Common up the HTMLConstructor implementations

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

We have a bunch of HTMLConstructor implementations that are all very similar.  The infrastructure from bug 1444286 lets us common them up into a single function.  This saves about 80KB of codesize, if I am measuring right.
The codegen changes are mostly a backout of the changes made in bug 1274159.

The HTMLConstructor implementation is mostly copied from one of the
code-generated ones, with a few modifications:

* Derive the interface name from the proto id instead of hardcoding it.
* Use the proto/constructor ids to get constructor and prototype objects.
* Use ErrorResult instead of FastErrorResult; we don't want the precedent of
  using FastErrorResult in non-generated code.

There will be further changes to combine HTMLConstructor and
CreateXULOrHTMLElement, in a separate changeset.
Attachment #8959675 - Flags: review?(peterv)
This fixes an observable bug we had due to doing the steps in a different order
from the spec: the 'prototype' get can have side-effects so needs to happen
after some of the other sanity checks.
Attachment #8959676 - Flags: review?(peterv)
Priority: -- → P2
Attachment #8959675 - Flags: review?(peterv) → review+
Blocks: 1447454
Comment on attachment 8959676 [details] [diff] [review]
part 2.  Combine HTMLConstructor and CreateXULOrHTMLElement into a single function

Review of attachment 8959676 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/BindingUtils.cpp
@@ +3748,5 @@
> +      }
> +    }
> +
> +    // Step 12.
> +    constructionStack.LastElement() = nullptr;

It might make things more understandable to use ALREADY_CONSTRUCTED_MARKER here?
Attachment #8959676 - Flags: review?(peterv) → review+
> It might make things more understandable to use ALREADY_CONSTRUCTED_MARKER here?

Yes, fixed.

Comment 6

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b470762b4676
part 1.  Use a single handwritten HTMLConstructor implementation, instead of code-generating lots of very similar implementations. r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed0718ede32
part 2.  Combine HTMLConstructor and CreateXULOrHTMLElement into a single function.  r=peterv

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b470762b4676
https://hg.mozilla.org/mozilla-central/rev/9ed0718ede32
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10311 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/w3c/web-platform-tests/pull/10311
* continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/362266178?utm_source=github_status&utm_medium=notification)
Upstream PR merged
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.