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

WebIDL HTMLConstructor support #17224

Merged
merged 8 commits into from Jun 16, 2017

Conversation

@cbrewster
Copy link
Member

commented Jun 8, 2017

spec: https://html.spec.whatwg.org/multipage/#htmlconstructor


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17194 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jun 8, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/webidls/HTMLDataListElement.webidl, components/script/dom/webidls/HTMLBaseElement.webidl, components/script/dom/webidls/HTMLSpanElement.webidl, components/script/dom/bindings/codegen/parser/tests/test_constructor.py and 67 more
  • @KiChjang: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/webidls/HTMLDataListElement.webidl, components/script/dom/webidls/HTMLBaseElement.webidl, components/script/dom/webidls/HTMLSpanElement.webidl, components/script/dom/bindings/codegen/parser/tests/test_constructor.py and 67 more
@highfive

This comment has been minimized.

Copy link

commented Jun 8, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@cbrewster

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2017

This is getting close to done. I am new to a lot of the jsapi stuff and codegen, so any feedback there would be very helpful!

cc @jdm

@jdm jdm assigned jdm and unassigned pcwalton Jun 8, 2017

return Err(Error::Type("Active function object is not HTMLElement".to_owned()));
}
} else {
// TODO: Step 5

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jun 8, 2017

Author Member

I'm not quite sure how to do step 5. I think there are two potential options:

  1. Get a list of local names from the callee. I am not even sure if this is possible.
  2. Get a ConstructorObject from the definition's local name and compare it to the callee. But we don't generate GetConstructorObject for most of the HTML interfaces.

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

Given the commit that adds HTMLConstructor to the HTML interfaces, do we not generate GetConstructorObjects for them now?

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jun 12, 2017

Author Member

We only generate GetConstructorObject for interfaces that are a namespace, callback, or have descendants.
Configuration.py#L397-L401

This comment has been minimized.

Copy link
@jdm

This comment has been minimized.

Copy link
@nox

nox Jun 13, 2017

Member

Yeah, not generating GetConstructorObject for leaf interfaces was just me trimming a bit of unneeded codegen, but if it's not unneeded anymore, just make it generate the function again.

@cbrewster cbrewster force-pushed the cbrewster:html_constructor branch 2 times, most recently from 5ca7cca to ccd5acf Jun 9, 2017

@nox

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

Changes to WebIDL.py need to be versioned as standalone patch files in the repository. But in your case you don't need to do it, because the upstream parser in mozilla-central already supports [HTMLConstructor]. Will do a PR to update ours.

@nox

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2017

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

@jdm
Copy link
Member

left a comment

Good start!

let registry = window.CustomElements();
let document = window.Document();

// Step 2 in codegen

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

// Step 2 is checked in the generated caller code

// Step 2 https://html.spec.whatwg.org/multipage/#htmlconstructor
// The custom element definition cannot use an element interface as its constructor
rooted!(in(cx) let new_target = UnwrapObject(args.new_target().to_object(), 1));

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

Add this from Gecko's implementation:

// The new_target might be a cross-compartment wrapper. Get the underlying object
// so we can do the spec's object-identity checks.
throw_dom_exception(cx, global.upcast::<GlobalScope>(), Error::Type("new.target is null".to_owned()));
}
{

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

Add this from Gecko:

// Step 2 of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor.
// Enter the compartment of our underlying new_target object, so we end
// up comparing to the constructor object for our interface from that global.
prefix: Option<Prefix>,
document: &Document,
creator: ElementCreator)
-> Root<Element> {

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

nit: indentation

@@ -63,6 +63,12 @@ impl CustomElementRegistry {
self.when_defined.borrow_mut().clear()
}

pub fn lookup_definition_by_constructor(&self, constructor: HandleObject) -> Option<CustomElementDefinition> {

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

Let's return Option<&CustomElementDefinition>.

// Step 7
rooted!(in(cx) let global_object = CurrentGlobalOrNull(cx));
GetProtoObject(cx, global_object.handle(), prototype.handle_mut());
if prototype.is_null() {

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

We assert that our prototypes are not null instead.

}
if !proto_val.is_object() {
// Step 7

This comment has been minimized.

@@ -158,6 +167,26 @@ pub unsafe fn create_global_object(
JS_FireOnNewGlobalObject(cx, rval.handle());
}

// https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor
pub fn create_html_element(window: &Window, call_args: CallArgs) -> Fallible<Root<HTMLElement>> {

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

&CallArgs

@@ -228,27 +233,42 @@ impl CustomElementRegistryMethods for CustomElementRegistry {
}
}

#[derive(HeapSizeOf, JSTraceable, PartialEq, Clone)]
pub enum ConstructionEntry {
Element(Root<HTMLElement>),

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

This should be JS<HTMLElement>, and the type should be marked #[must_root].

Some(entry) => {
let result = match *entry {
ConstructionEntry::AlreadyConstructed => return Err(Error::InvalidState),
ConstructionEntry::Element(ref mut element) => {

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

No need for mut.

@jdm jdm added S-fails-tidy and removed S-awaiting-review labels Jun 12, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

failures:

    size_of::size_div

    size_of::size_element

    size_of::size_htmlelement

    size_of::size_span

@cbrewster cbrewster force-pushed the cbrewster:html_constructor branch from ccd5acf to 76878fd Jun 12, 2017

@cbrewster cbrewster force-pushed the cbrewster:html_constructor branch 2 times, most recently from 17d6364 to 5cfbc5f Jun 13, 2017

@cbrewster cbrewster force-pushed the cbrewster:html_constructor branch from 5eac359 to 114ea9b Jun 13, 2017

@cbrewster cbrewster changed the title [WIP] WebIDL HTMLConstructor support WebIDL HTMLConstructor support Jun 13, 2017

@cbrewster cbrewster force-pushed the cbrewster:html_constructor branch 3 times, most recently from e6c2025 to 6c0b6c3 Jun 13, 2017

@jdm
jdm approved these changes Jun 15, 2017
Copy link
Member

left a comment

r=me once the function is moved.


/// Returns the constructor object for the element associated with the given local name.
/// This list should only include elements marked with the [HTMLConstructor] extended attribute.
pub fn get_constructor_object_from_local_name(name: LocalName,

This comment has been minimized.

Copy link
@jdm

jdm Jun 15, 2017

Member

Let's move this to dom/bindings/interfaces.rs.

@cbrewster cbrewster force-pushed the cbrewster:html_constructor branch from 6c0b6c3 to e4ff934 Jun 16, 2017

@cbrewster

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2017

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2017

📌 Commit e4ff934 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2017

⌛️ Testing commit e4ff934 with merge c58bcc2...

bors-servo added a commit that referenced this pull request Jun 16, 2017
Auto merge of #17224 - cbrewster:html_constructor, r=jdm
WebIDL HTMLConstructor support

<!-- Please describe your changes on the following line: -->
spec: https://html.spec.whatwg.org/multipage/#htmlconstructor

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17194 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17224)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2017

@bors-servo bors-servo merged commit e4ff934 into servo:master Jun 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
jdm added a commit to web-platform-tests/wpt that referenced this pull request Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.