Closed Bug 1059146 Opened 10 years ago Closed 8 years ago

menulist.xml uses Mutation Events, should switch to MutationObserver

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: Gijs, Assigned: Kwan)

References

Details

Attachments

(3 files, 5 obsolete files)

Bug 1059146 - Switch menulist.xml to use MutationObserver instead of Mutation Events
58 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1059146 - Remove broadcast listeners from menulist.xml and always use MutationObservers
58 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1059146 - Update test_menulist.xul and test_bug360220.xul to wait for MutationObservers to finish
58 bytes, text/x-review-board-request
Gijs
: review+
Details 
STR:

1. Open a feed preview page (e.g. for http://feeds.bbci.co.uk/news/rss.xml )

ER:

no errors

AR:

Use of Mutation Events is deprecated. Use MutationObserver instead. menulist.xml:240
Blocks: 395496
(In reply to :Gijs Kruitbosch from comment #0)
> Use of Mutation Events is deprecated. Use MutationObserver instead.
> menulist.xml:240


For completeness, this is:

http://hg.mozilla.org/mozilla-central/annotate/0753f7b93ab7/toolkit/content/widgets/menulist.xml#l240
Nothing wrong with using broadcast listeners though.
Summary: menulist.xml uses Mutation Events and broadcast listeners, should switch to MutationObserver → menulist.xml uses Mutation Events, should switch to MutationObserver
(In reply to neil@parkwaycc.co.uk from comment #2)
> Nothing wrong with using broadcast listeners though.

Can't we switch to mutationobservers wholesale? From a casual look, it seemed like that'd simplify the code.
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to neil@parkwaycc.co.uk from comment #2)
> > Nothing wrong with using broadcast listeners though.
> 
> Can't we switch to mutationobservers wholesale? From a casual look, it
> seemed like that'd simplify the code.
Looks like.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28e072bdafc9
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Comment on attachment 8574717 [details] [diff] [review]
Part 1 - Switch menulist.xml to use MutationObserver instead of Mutation Events

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

::: toolkit/content/widgets/menulist.xml
@@ +239,5 @@
>                  document.addBroadcastListenerFor(val, this, "description");
>                }
> +              else {
> +                this.mAttributeObserver = new MutationObserver(this.handleMutation);
> +                this.mAttributeObserver.observe(val, { attributes: true,

One of my favourite parts of the spec is that attributeFilter implies attributes, so you can omit it here. See the description of attributeFilter here: https://dom.spec.whatwg.org/#mutationobserver .

Please also just stick the list of attributes in a temp var, and then pass that as the value, so we don't need 3 extra lines. :-)

@@ +272,5 @@
>          <body>
>            <![CDATA[
> +            for (let record of aRecords) {
> +              if (record.type == "attributes" &&
> +                  record.target == this.mSelectedInternal) {

Meh, don't need to check record.type here considering it's the only one we listen for, right?
Attachment #8574717 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8574718 [details] [diff] [review]
Part 2 - Remove broadcast listeners from menulist.xml and always use MutationObservers

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

Yay code removal!
Attachment #8574718 - Flags: review?(gijskruitbosch+bugs) → review+
- remove implements="nsIDOMEventListener", since it doesn't any more.

Why was this + the second patch causing M-oth failures?  Because the value of "this" in a mutation handler is the observer!

+ .bind(this) so it equals the menulist.

Why is this + the second patch now causing element.menupopup to fail in a test?  I don't know!

(In reply to :Gijs Kruitbosch from comment #7)
> One of my favourite parts of the spec is that attributeFilter implies
> attributes, so you can omit it here. See the description of attributeFilter
> here: https://dom.spec.whatwg.org/#mutationobserver .
Duh, now I feel silly.  Done.

> Please also just stick the list of attributes in a temp var, and then pass
> that as the value, so we don't need 3 extra lines. :-)
Heh, I wondered if that might come up.  Done.

> Meh, don't need to check record.type here considering it's the only one we
> listen for, right?
Yeah, I was just being excessively paranoid. Removed.
Attachment #8574717 - Attachment is obsolete: true
Now with less accidental-deletion-of-needed-code.
Attachment #8574718 - Attachment is obsolete: true
(In reply to Ian Moody [:Kwan] from comment #9)
> Created attachment 8575131 [details] [diff] [review]
> Part 1 - Switch menulist.xml to use MutationObserver instead of Mutation
> Events
> 
> - remove implements="nsIDOMEventListener", since it doesn't any more.
> 
> Why was this + the second patch causing M-oth failures?  Because the value
> of "this" in a mutation handler is the observer!
> 
> + .bind(this) so it equals the menulist.
> 
> Why is this + the second patch now causing element.menupopup to fail in a
> test?  I don't know!
Urgh, because I failed at rebasing the second patch on top of the .bind(this) change and left a loose brace in, which broke everything.

So the M-oth tests are still failing, but now I'm pretty certain it's purely because they are checking before the mutation handler has had a chance to do its thing, since editing menuitems live with the DOMI certainly works fine at updating the menulist. Will need to look at making the tests wait later this afternoon.
Attachment #8575132 - Attachment is obsolete: true
(In reply to Ian Moody [:Kwan] from comment #11)
> So the M-oth tests are still failing, but now I'm pretty certain it's purely
> because they are checking before the mutation handler has had a chance to do
> its thing, since editing menuitems live with the DOMI certainly works fine
> at updating the menulist. Will need to look at making the tests wait later
> this afternoon.

You could make them use a mutation observer on the menulist element itself to listen for the attributes changing? :-)
Review commit: https://reviewboard.mozilla.org/r/59160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59160/
Attachment #8762516 - Flags: review?(gijskruitbosch+bugs)
Attachment #8762517 - Flags: review?(gijskruitbosch+bugs)
Attachment #8762518 - Flags: review?(gijskruitbosch+bugs)
Attachment #8575131 - Attachment is obsolete: true
Attachment #8575163 - Attachment is obsolete: true
Attachment #8762518 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8762518 [details]
Bug 1059146 - Update test_menulist.xul and test_bug360220.xul to wait for MutationObservers to finish

https://reviewboard.mozilla.org/r/59164/#review56152

::: toolkit/content/tests/chrome/test_menulist.xul:162
(Diff revision 1)
>    // editable menulists don't use the image or description properties currently
>    if (editable) {
>      element.selectedIndex = 1;
>      is(element.image, "", testprefix + " image set to selected");
>      is(element.description, "", testprefix + " description set to selected");
> +    test_nsIDOMXULMenuListElement_Finish(element, testprefix, editable);

Nit: please lowercase 'finish' (and 'properties' and 'unselected' later on)

::: toolkit/content/tests/chrome/test_menulist.xul:177
(Diff revision 1)
> -    is(element.label, "Item Number Three", testprefix + " label modified");
> -    thirditem.value = "item-three";
> +                      {attr: "value", value: "item-three"}, 
> +                      {attr: "image", value: "smile.png"}, 

Nit: please get rid of trailing whitespace
Comment on attachment 8762517 [details]
Bug 1059146 - Remove broadcast listeners from menulist.xml and always use MutationObservers

https://reviewboard.mozilla.org/r/59162/#review56156
Attachment #8762517 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8762516 [details]
Bug 1059146 - Switch menulist.xml to use MutationObserver instead of Mutation Events

https://reviewboard.mozilla.org/r/59160/#review56158
Attachment #8762516 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8762518 [details]
Bug 1059146 - Update test_menulist.xul and test_bug360220.xul to wait for MutationObservers to finish

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59164/diff/1-2/
Thanks Gijs for the speedy review!  Both nits addressed.  Sorry it took so long to finish this off, my old computer lost the ability to work on Firefox shortly after I started updating the tests.

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b69f06127646
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/dac7420ef19d
Switch menulist.xml to use MutationObserver instead of Mutation Events. r=gijs
https://hg.mozilla.org/integration/fx-team/rev/0ec389f578c4
Remove broadcast listeners from menulist.xml and always use MutationObservers. r=gijs
https://hg.mozilla.org/integration/fx-team/rev/cb5eccf28d08
Update test_menulist.xul and test_bug360220.xul to wait for MutationObservers to finish r=gijs
Keywords: checkin-needed
Had to back this out for reftest failures in 508908-1.xul on Windows 7 opt:

https://hg.mozilla.org/integration/fx-team/rev/270ba15c5f00b917a230a08c46e61672f834a3aa
https://hg.mozilla.org/integration/fx-team/rev/0e3e27ab48fb2d2603d097be645258d9437722c7
https://hg.mozilla.org/integration/fx-team/rev/f83c6ae171402f19a537a1432595ffbd36bb27eb

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=cb5eccf28d080455a62d3b09f8b698c6bc306774
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=9962588&repo=fx-team

REFTEST TEST-UNEXPECTED-FAIL | file:///c:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/508908-1.xul == file:///c:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/508908-1-ref.xul | (LOAD ONLY), max difference: 236, number of differing pixels: 16591
Flags: needinfo?(moz-ian)
Reftest failure, that was unexpected.

So what's going on here?  The testfile weirdly seems to involve a <html:embed> inside a <menulist>. The very helpful screencap data URI images in the log show a menulist showing "null" in the failure, and an empty menulist in the ref.

I'm guessing a get/setAttribute() is involved somehow.

And it's one inside the "if (val) {" of .selectedItem that sets the label. Since there isn't one, there's the null from the getAttribute().  This would have happened in the old code too, but the broadcast listener seems to immediately sync it up with the lack of label on the element.  Removing just the label broadcast listener is enough to get the old code to fail the same way.

So two options 1) keep the broadcast listeners around 2) add some hasAttribute() checks

This also caused me to realise that I've caused a behaviour change.  The old code used the mutation event's newValue property to set the value on the menulist, which in the case of attribute deletion is the empty string. However, the mutation record doesn't have a newValue property, only an oldValue, so the getAttribute() I've used instead causes attr="null" on attribute deletion, instead of the old attr="".

Which I've just checked is a difference of behaviour between being in an XULDocument or not, since the broadcast listener fully syncs the attribute, deletion and all.  So I can either go with the broadcast behaviour of deleting the attr (slightly more code with the removeAttribute() [0]), or go with the non-XULDocument behaviour of attr deletion being matched with an empty attr.  I'm inclined to go with copying the broadcast behaviour everywhere, since menulist in XUL is surely far more common that not.  The other option would be to keep the broadcast listeners around.

[0]Could pull that "if (has) {set(get)} else {remove}" out into a method on its own, but with all the XUL that ends up being more lines anyway, though the two call sites are nicer to read.
Flags: needinfo?(moz-ian)
Comment on attachment 8762516 [details]
Bug 1059146 - Switch menulist.xml to use MutationObserver instead of Mutation Events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59160/diff/1-2/
Comment on attachment 8762517 [details]
Bug 1059146 - Remove broadcast listeners from menulist.xml and always use MutationObservers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59162/diff/1-2/
Comment on attachment 8762518 [details]
Bug 1059146 - Update test_menulist.xul and test_bug360220.xul to wait for MutationObservers to finish

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59164/diff/2-3/
Attachment #8762516 - Flags: review+ → review?(gijskruitbosch+bugs)
Attachment #8762517 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8762516 [details]
Bug 1059146 - Switch menulist.xml to use MutationObserver instead of Mutation Events

Looks good, thanks for diving in. You can run the reftest locally with mach as well to verify the fix worked. :-)
Attachment #8762516 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8762517 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks Gijs!

>You can run the reftest locally with mach as well to verify the fix worked. :-)
I did do this to investigate the problem, and verify the fix, but I was wondering if there is anyway to inspect with devtools or attach the jsconsole to the test?  Wanted to do that when investigating but couldn't, so just ended up including "foo" in the setAttribute()s to find the problem. And then doing some investigating of how broadcast listeners work, as they seem to be sadly lacking in documentation.

Green try including the previously failing R3 that caused the backout:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3231e0c34076
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7b2c25149bc1
Switch menulist.xml to use MutationObserver instead of Mutation Events. r=gijs
https://hg.mozilla.org/integration/fx-team/rev/1e387aca0b24
Remove broadcast listeners from menulist.xml and always use MutationObservers. r=gijs
https://hg.mozilla.org/integration/fx-team/rev/c914e1312fd6
Update test_menulist.xul and test_bug360220.xul to wait for MutationObservers to finish. r=gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b2c25149bc1
https://hg.mozilla.org/mozilla-central/rev/1e387aca0b24
https://hg.mozilla.org/mozilla-central/rev/c914e1312fd6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: