Closed Bug 1481286 Opened 2 years ago Closed 2 years ago

Move command dispatcher from XULDocument to Document

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(2 files, 5 obsolete files)

The commandDispatcher attribute needs to be moved from XULDocument.webidl to Document.webidl so that it works for chrome HTML pages.
Priority: -- → P3
The "commandDispatcher" and "addBroadcastListenerFor" attributes are going
to be removed from content privilege XUL.

MozReview-Commit-ID: DL8qxToeNMm
Attachment #8998266 - Attachment is obsolete: true
The "commandDispatcher" and "addBroadcastListenerFor" attributes are going
to be removed from content privilege XUL.

MozReview-Commit-ID: DL8qxToeNMm
Attachment #8998275 - Attachment is obsolete: true
The "commandDispatcher" and "addBroadcastListenerFor" attributes are going
to be removed from content privilege XUL.

MozReview-Commit-ID: DL8qxToeNMm
Attachment #8998298 - Flags: review?(bzbarsky)
Create a "commandset" custom element that performs the job of adding and
removing command updaters that XULDocument use to do. Previously, the
"commandupdater" attribute was allowed on any element, but in tree it is
only every used on "commandset" elements.

MozReview-Commit-ID: HUXMG9kx4ft
Attachment #8998299 - Flags: review?(bzbarsky)
Allows non-XUL chrome privilege documents to also use the command
dispatcher. The command dispatcher is created lazily since it will not
always be used.

MozReview-Commit-ID: HUXMG9kx4ft
Attachment #8998300 - Flags: review?(bzbarsky)
Comment on attachment 8998298 [details] [diff] [review]
Update test to reflect removal of content XUL attributes.

This should be merged with the relevant removal changesets, right?
Attachment #8998298 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> Comment on attachment 8998298 [details] [diff] [review]
> Update test to reflect removal of content XUL attributes.
> 
> This should be merged with the relevant removal changesets, right?

I got a little ahead of myself. I'll remove the test for 'addBroadcastListenerFor' in another bug and squash this commit into the third commit.
Comment on attachment 8998299 [details] [diff] [review]
Use a custom element to add/remove command updater.

>+    if (this.getAttribute("commandupdater")) {

This doesn't match the old code (which compared the value to "true").  Do we want to preserve the old behavior, or just test for the attribute's presence?  If the latter, use hasAttribute() instead.  This applies both places we do this check.

>+      if (events === "") {

If the events attr is not set, you will have null === "" here (modulo the XULElement weirdness around getAttribute, but I hope that will die sometime), which will test false.  This should use ==, not ===, with a comment explaining why.  Or just:

  events = events || "*";

>+      if (targets === "") {

Same thing.

r=me
Attachment #8998299 - Flags: review?(bzbarsky) → review+
Comment on attachment 8998300 [details] [diff] [review]
Move command dispatcher from XULDocument to Document.

>+  readonly attribute XULCommandDispatcher? commandDispatcher;

Please document when this returns null.

r=me
Attachment #8998300 - Flags: review?(bzbarsky) → review+
Create a "commandset" custom element that performs the job of adding and
removing command updaters that XULDocument use to do. Previously, the
"commandupdater" attribute was allowed on any element, but in tree it is
only every used on "commandset" elements.

MozReview-Commit-ID: HUXMG9kx4ft
Attachment #8998558 - Flags: review?(bzbarsky)
Attachment #8998299 - Attachment is obsolete: true
Allows non-XUL chrome privilege documents to also use the command
dispatcher. The command dispatcher is created lazily since it will not
always be used.

Update test to reflect removal of the XUL attribute "commandDispatcher"
from content privilege XUL.

MozReview-Commit-ID: HUXMG9kx4ft
Attachment #8998559 - Flags: review?(bzbarsky)
Attachment #8998298 - Attachment is obsolete: true
Attachment #8998300 - Attachment is obsolete: true
Comment on attachment 8998558 [details] [diff] [review]
Use a custom element to add/remove command updater.

Carrying r+ forward.
Attachment #8998558 - Flags: review?(bzbarsky) → review+
Comment on attachment 8998559 [details] [diff] [review]
Move command dispatcher from XULDocument to Document.

Carrying r+ forward.
Attachment #8998559 - Flags: review?(bzbarsky) → review+
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0dcbca21b5
Use a custom element to add/remove command updater. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/271ad78ecd80
Move command dispatcher from XULDocument to Document. r=bz
https://hg.mozilla.org/mozilla-central/rev/fc0dcbca21b5
https://hg.mozilla.org/mozilla-central/rev/271ad78ecd80
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
See Also: → 1478434
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.