Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce trait Castable #8041

Merged
merged 5 commits into from Oct 21, 2015
Merged

Introduce trait Castable #8041

merged 5 commits into from Oct 21, 2015

Conversation

@nox
Copy link
Member

nox commented Oct 15, 2015

Removes all those messy FooCast structures in InheritTypes.rs.

Review on Reviewable

@highfive
Copy link

highfive commented Oct 15, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@jdm
Copy link
Member

jdm commented Oct 15, 2015

I got through the first two commits.


Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: 2 of 84 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

@eefriedman
Copy link
Contributor

eefriedman commented Oct 16, 2015

It's probably a good idea to always specify the type of downcasts; even if the compiler can technically infer the expected type, it's difficult to read. For example, in one case the type of a downcast is deduced based on the fact that LayoutHTMLInputElementHelpers is the only trait which currently has a method called "get_size_for_layout".

@nox
Copy link
Member Author

nox commented Oct 16, 2015

@eefriedman I don't think so. The case you found maybe is surprising, but not much more than some Deref coercions we already have all over the place. In other occurrences, removing the type really helps readability:

    fn get_html_element(&self) -> Option<Root<HTMLHtmlElement>> {
        self.GetDocumentElement().and_then(Root::downcast)
    }
@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

The latest upstream changes (presumably #8026) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox force-pushed the nox:castable branch from 523555e to 37eb89a Oct 16, 2015
@nox nox removed the S-needs-rebase label Oct 16, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2015

The latest upstream changes (presumably #7977) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox force-pushed the nox:castable branch from 37eb89a to 6e723d5 Oct 17, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 17, 2015

The latest upstream changes (presumably #7415) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox force-pushed the nox:castable branch from 6e723d5 to e3a12c0 Oct 19, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2015

The latest upstream changes (presumably #7935) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox force-pushed the nox:castable branch from e3a12c0 to 106aef2 Oct 19, 2015
@nox nox removed the S-needs-rebase label Oct 19, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 20, 2015

The latest upstream changes (presumably #8095) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm removed the S-awaiting-review label Oct 20, 2015
@nox
Copy link
Member Author

nox commented Oct 20, 2015

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


components/script/dom/bindings/conversions.rs, line 112 [r3] (raw file):
Well, these are conversions. I feel like we should make a round of patches to move around pieces of code, introducing new modules in the process, but that feels a bit out of scope.


components/script/dom/domimplementation.rs, line 118 [r4] (raw file):
What about an expect call? I don't really like assert that actually do important things. I'm pretty sure we have more unwrap calls than we have uses of assert in such cases.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Oct 20, 2015

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


components/script/dom/bindings/conversions.rs, line 112 [r3] (raw file):
File a followup issue, please. Everything else in this file is related to converting to/from JS values.


components/script/dom/domimplementation.rs, line 118 [r4] (raw file):
That's fair. I'd be ok with either

let result = doc_node.AppendChild(doc_type.upcast());
assert!(result.is_ok());

or expect.


Comments from the review on Reviewable.io

nox added 4 commits Oct 6, 2015
This method is given a DOMClass value and returns whether it derives from Self.

Interfaces with no descendants directly check whether the given DOMClass is the
same as their own.
This trait is used to hold onto the downcast and upcast functions of all
castable IDL interfaces. A castable IDL interface is one which either derives
from or is derived by other interfaces.

The deriving relation is represented by implementations of marker trait
DerivedFrom<T: Castable> generated in InheritTypes.

/^[ ]*use dom::bindings::codegen::InheritTypes::.*(Base|Cast|Derived)/ {
    /::[a-zA-Z]+(Base|Cast|Derived);/d
    s/([{ ])[a-zA-Z]+(Base|Cast|Derived), /\1/g
    s/([{ ])[a-zA-Z]+(Base|Cast|Derived), /\1/g
    s/, [a-zA-Z]+(Base|Cast|Derived)([},])/\2/g
    s/, [a-zA-Z]+(Base|Cast|Derived)([},])/\2/g
    /\{([a-zA-Z]+(Base|Cast|Derived))?\};$/d
    s/\{([a-zA-Z_]+)\};$/\1;/
}

s/([a-zA-Z]+)Cast::from_ref\(\&?\**([a-zA-Z_]+)(\.r\(\))?\)/\2.upcast::<\1>()/g
s/([a-zA-Z]+)Cast::from_ref\(\&?\**([a-zA-Z_]+)(\.[a-zA-Z_]+\(\))?\)/\2\3.upcast::<\1>()/g
s/\(([a-zA-Z]+)Cast::from_ref\)/\(Castable::upcast::<\1>\)/g

s/([a-zA-Z]+)Cast::from_root/Root::upcast::<\1>/g

s/([a-zA-Z]+)Cast::from_layout_js\(\&([a-zA-Z_.]+)\)/\2.upcast::<\1>()/g

s/([a-zA-Z]+)Cast::to_ref\(\&?\**([a-zA-Z_]+)(\.r\(\))?\)/\2.downcast::<\1>()/g
s/([a-zA-Z]+)Cast::to_ref\(\&?\**([a-zA-Z_]+)(\.[a-zA-Z_]+\(\))?\)/\2\3.downcast::<\1>()/g
s/\(([a-zA-Z]+)Cast::to_ref\)/\(Castable::downcast::<\1>\)/g

s/([a-zA-Z]+)Cast::to_root/Root::downcast::<\1>/g

s/([a-zA-Z]+)Cast::to_layout_js\(&?([a-zA-Z_.]+(\(\))?)\)/\2.downcast::<\1>()/g

s/\.is_document\(\)/.is::<Document>()/g
s/\.is_htmlanchorelement\(\)/.is::<HTMLAnchorElement>()/g
s/\.is_htmlappletelement\(\)/.is::<HTMLAppletElement>()/g
s/\.is_htmlareaelement\(\)/.is::<HTMLAreaElement>()/g
s/\.is_htmlbodyelement\(\)/.is::<HTMLBodyElement>()/g
s/\.is_htmlembedelement\(\)/.is::<HTMLEmbedElement>()/g
s/\.is_htmlfieldsetelement\(\)/.is::<HTMLFieldSetElement>()/g
s/\.is_htmlformelement\(\)/.is::<HTMLFormElement>()/g
s/\.is_htmlframesetelement\(\)/.is::<HTMLFrameSetElement>()/g
s/\.is_htmlhtmlelement\(\)/.is::<HTMLHtmlElement>()/g
s/\.is_htmlimageelement\(\)/.is::<HTMLImageElement>()/g
s/\.is_htmllegendelement\(\)/.is::<HTMLLegendElement>()/g
s/\.is_htmloptgroupelement\(\)/.is::<HTMLOptGroupElement>()/g
s/\.is_htmloptionelement\(\)/.is::<HTMLOptionElement>()/g
s/\.is_htmlscriptelement\(\)/.is::<HTMLScriptElement>()/g
s/\.is_htmltabledatacellelement\(\)/.is::<HTMLTableDataCellElement>()/g
s/\.is_htmltableheadercellelement\(\)/.is::<HTMLTableHeaderCellElement>()/g
s/\.is_htmltablerowelement\(\)/.is::<HTMLTableRowElement>()/g
s/\.is_htmltablesectionelement\(\)/.is::<HTMLTableSectionElement>()/g
s/\.is_htmltitleelement\(\)/.is::<HTMLTitleElement>()/g
@nox nox force-pushed the nox:castable branch from 106aef2 to 68014af Oct 21, 2015
@nox nox removed the S-needs-rebase label Oct 21, 2015
@nox
Copy link
Member Author

nox commented Oct 21, 2015

@jdm Rebased and addressed, I would appreciate it if I don't need to ever rebase that again. ;)

@nox nox force-pushed the nox:castable branch from 52dffc8 to d0e022b Oct 21, 2015
@nox
Copy link
Member Author

nox commented Oct 21, 2015

Added a commit to fix #8125.

@jdm
Copy link
Member

jdm commented Oct 21, 2015

@bors-servo: r+
https://www.youtube.com/watch?v=FnpJBkAMk44


Reviewed 35 of 35 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2015

📌 Commit d0e022b has been approved by jdm

@nox
Copy link
Member Author

nox commented Oct 21, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2015

Testing commit d0e022b with merge 674589c...

bors-servo pushed a commit that referenced this pull request Oct 21, 2015
bors-servo
Introduce trait Castable

Removes all those messy FooCast structures in InheritTypes.rs.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8041)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2015

@bors-servo bors-servo merged commit d0e022b into servo:master Oct 21, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:castable branch Oct 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.