Add HTMLSlotElement

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla58
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Attachment #8912216 - Flags: review?(hsivonen)
(Assignee)

Comment 2

2 years ago
Posted patch slot_dom.diff (obsolete) — Splinter Review
The element isn't enabled yet
Attachment #8912217 - Flags: review?(hsivonen)
(Assignee)

Comment 3

2 years ago
I just want to land the code to make it easier to land other Shadow DOM stuff
(Assignee)

Comment 4

2 years ago
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/4282679f353799022b5476b3f6840878830edc34
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4282679f353799022b5476b3f6840878830edc34
(Assignee)

Comment 5

2 years ago
Attachment #8912217 - Attachment is obsolete: true
Attachment #8912217 - Flags: review?(hsivonen)
Attachment #8912321 - Flags: review?(hsivonen)
Comment on attachment 8912321 [details] [diff] [review]
slot_dom.diff (fixed some nits)

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

Not saying r+ quite yet pending answer to the question about AssignedNodeOptions getting ignored. (Also, please change the comment about clang-format.)

::: dom/html/HTMLSlotElement.cpp
@@ +52,5 @@
> +
> +void
> +HTMLSlotElement::AssignedNodes(const AssignedNodesOptions& aOptions,
> +                               nsTArray<RefPtr<nsINode>>& aNodes)
> +{

Why is aOptions ignored instead of having an effect per spec?

::: parser/html/java/README.txt
@@ +86,5 @@
>  # Save.
>  cd ../.. # Back to parser/html/java/
>  make translate
>  cd ../../..
> +# XXXsmaug clang-format doesn't deal well with some macros, so you may not want to use it.

Better to guide readers to exclude specific files from clang-format than to suggest not running it at all.
Attachment #8912216 - Flags: review?(hsivonen) → review+
(In reply to Henri Sivonen (:hsivonen) from comment #6)
> Better to guide readers to exclude specific files from clang-format than to
> suggest not running it at all.

Or sections of files, rather. With // clang-format off and // clang-format on
(Assignee)

Comment 8

2 years ago
This is just adding the basic class. It is not doing anything yet, that is why 
HTMLSlotElement::AssignedNodes is very dummy.
Note, with this patch there isn't a way to create HTMLSlotElement yet, since the constructor call is explicitly commented out.

I can remove the XXXsmaug  comment, but it has been very painful to notice after wards that clang-format totally destroyed some files.
(Assignee)

Comment 9

2 years ago
s/destroyed/destroyed formatting of/

Updated

2 years ago
Blocks: 1404842
Comment on attachment 8912321 [details] [diff] [review]
slot_dom.diff (fixed some nits)

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

(In reply to Olli Pettay [:smaug] from comment #8)
> This is just adding the basic class. It is not doing anything yet, that is
> why 
> HTMLSlotElement::AssignedNodes is very dummy.
> Note, with this patch there isn't a way to create HTMLSlotElement yet, since
> the constructor call is explicitly commented out.

OK.

> I can remove the XXXsmaug  comment, but it has been very painful to notice
> after wards that clang-format totally destroyed some files.

I think it's good to warn readers to make a local backup commit before running ./mach clang-format, but I think it's not OK to suggest not running it at all.
Attachment #8912321 - Flags: review?(hsivonen) → review+
(Assignee)

Comment 11

2 years ago
Looks like I need to add the interface to the interface list on non-stylo, since we enable webcomponents in such cases in test profiles.

Comment 12

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3704f4a298dc
Add HTMLSlotElement (disabled for now), r=hsivonen

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3704f4a298dc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

2 years ago
Blocks: 1405934

Updated

2 years ago
No longer blocks: 1205323
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.