Move command dispatcher from XULDocument to Document

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
2 months ago

People

(Reporter: bdahl, Assigned: bdahl)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

10 months ago
The commandDispatcher attribute needs to be moved from XULDocument.webidl to Document.webidl so that it works for chrome HTML pages.
Priority: -- → P3
Assignee

Comment 1

10 months ago
The "commandDispatcher" and "addBroadcastListenerFor" attributes are going
to be removed from content privilege XUL.

MozReview-Commit-ID: DL8qxToeNMm
Assignee

Updated

10 months ago
Attachment #8998266 - Attachment is obsolete: true
Assignee

Comment 2

10 months ago
The "commandDispatcher" and "addBroadcastListenerFor" attributes are going
to be removed from content privilege XUL.

MozReview-Commit-ID: DL8qxToeNMm
Assignee

Updated

10 months ago
Attachment #8998275 - Attachment is obsolete: true
Assignee

Comment 3

10 months ago
The "commandDispatcher" and "addBroadcastListenerFor" attributes are going
to be removed from content privilege XUL.

MozReview-Commit-ID: DL8qxToeNMm
Attachment #8998298 - Flags: review?(bzbarsky)
Assignee

Comment 4

10 months ago
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)
Assignee

Comment 5

10 months ago
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+
Assignee

Comment 7

10 months ago
(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+
Assignee

Comment 10

10 months ago
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)
Assignee

Updated

10 months ago
Attachment #8998299 - Attachment is obsolete: true
Assignee

Comment 11

10 months ago
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)
Assignee

Updated

10 months ago
Attachment #8998298 - Attachment is obsolete: true
Attachment #8998300 - Attachment is obsolete: true
Assignee

Comment 12

10 months ago
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+
Assignee

Comment 13

10 months ago
Comment on attachment 8998559 [details] [diff] [review]
Move command dispatcher from XULDocument to Document.

Carrying r+ forward.
Attachment #8998559 - Flags: review?(bzbarsky) → review+

Comment 14

10 months ago
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

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc0dcbca21b5
https://hg.mozilla.org/mozilla-central/rev/271ad78ecd80
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
See Also: → 1478434
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.