Don't use broadcaster elements to store sidebar metadata

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

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

Comment 1

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

Comment 3

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

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

Updated

9 months ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
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.
Attachment #8999343 - Flags: review+
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.
Attachment #8999365 - Flags: review+
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.
Attachment #8999365 - Flags: review+

Comment 9

9 months ago
Pushed by paolo.mozmail@amadzone.org:
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

9 months ago
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4dba41c7a8
Backed out 2 changesets for mochitest failures on test_keycodes.xul. CLOSED TREE
Assignee

Comment 11

9 months ago
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
Assignee

Comment 12

9 months ago
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 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.
Attachment #8999365 - Flags: review+
Assignee

Comment 14

9 months ago
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?
Attachment #8999365 - Flags: review+ → review?(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.
Attachment #8999365 - Flags: review+
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+
Attachment #8999365 - Flags: review?(mixedpuppy)

Comment 17

9 months ago
Pushed by paolo.mozmail@amadzone.org:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ceb8813cc1d
https://hg.mozilla.org/mozilla-central/rev/be8aca082faf
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.