1482610 - Don't use broadcaster elements to store sidebar metadata

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[:Paolo Amadini]

This is part of the removal of broadcasters we're doing for bug 1479908.

The SidebarUI code currently abuses "broadcaster" elements to store metadata whose propagation to the observers is unnecessary. This metadata can be more simply stored in a JavaScript object, and the relevant attributes for the menu items can be set individually.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1482610 - Part 1 - Move the sidebar title and URL from the broadcasters to a JavaScript object. r=jaws

The immediate goal is only to remove the broadcasters, so we still require the labels to be set manually on the "toolbarbutton" and "menuitem" elements. Generating these elements programmatically from the new SidebarUI.sidebars object, both for built-in sidebars and extensions, can be a future improvement.

The autoCheck attribute is also unnecessary since it is only intended for the menu items, and they are already properly updated after their command is invoked. Since the attribute was written with the wrong capitalization, it already had no effect.

The persistence of the label of the sidebar selector is also unnecessary since it is already set on startup. Removing this does not seem to cause any additional flickering.

Comment 2

[:Paolo Amadini]

Attachment: Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

Comment 3

[:Paolo Amadini]

I expect some failing tests due to the attributes being moved, let's see:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f7d37cee4f4be8fa4f7c9bdc8a33f2e46b20260

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8999343 [details]
Bug 1482610 - Part 1 - Move the sidebar title and URL from the broadcasters to a JavaScript object. r=jaws

Jared Wein [:jaws] (please needinfo? me) has approved the revision.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8999365 [details]
Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

Jared Wein [:jaws] (please needinfo? me) has approved the revision.

Comment 6

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f21c86d9b4f14124061fa228866bd0b9e3b0ffe

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8999365 [details]
Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 8

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=911ac6439aa7109e0b124c2da7741d3338279465

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5873a719ec0
Part 1 - Move the sidebar title and URL from the broadcasters to a JavaScript object. r=jaws,mixedpuppy
https://hg.mozilla.org/integration/mozilla-inbound/rev/79a59b25e535
Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws,mixedpuppy

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4dba41c7a8
Backed out 2 changesets for mochitest failures on test_keycodes.xul. CLOSED TREE

Comment 11

[:Paolo Amadini]

I guess the rewritten front-end code may now be triggering some debug-only assertions that show up in other tests.

For now, running a new tryserver build including mochitests to see if the issue is confirmed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a19361ff01b69037fa5eae0e6ea3b5d0165a99bc

Comment 12

[:Paolo Amadini]

There were a few missing command replacements, and the assertion is for the "key" element pointing to an invalid command.

I added a browser-chrome test for the key bindings:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec716c60184a8e231864794a422c41e3d116b4a4

On Mac, the key bindings work by invoking the menu commands, so I didn't see the issue when testing manually. On other platforms, the command that is executed is the one on the "key" element. Eventually, as part of XUL removal we want to replace this command and key system with something else, but this is what we have for now.

On Windows, there are two different key bindings to open the Bookmarks sidebar, and only one is effectively advertised in the menu items. I don't know if we're considering removing the other one at some point, but for the moment I just added a test for it as well.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8999365 [details]
Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

Shane Caraveo (:mixedpuppy) has been removed from the revision.

Comment 14

[:Paolo Amadini]

Comment on attachment 8999365 [details]
Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

I requested review from Jared, but I just noticed after setting the flags on Bugzilla manually that he's away. Shane, maybe you can review the keys test and other changes per comment 12?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8999365 [details]
Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8999365 [details]
Bug 1482610 - Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws

this review flag seems backwards, removing it.  already gave r+

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ceb8813cc1d
Part 1 - Move the sidebar title and URL from the broadcasters to a JavaScript object. r=jaws,mixedpuppy
https://hg.mozilla.org/integration/mozilla-inbound/rev/be8aca082faf
Part 2 - Move the remaining attributes and remove the sidebar broadcasters. r=jaws,mixedpuppy

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8ceb8813cc1d
https://hg.mozilla.org/mozilla-central/rev/be8aca082faf