1391082 - Page action panel ordering can get messed up

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[Drew Willcoxon :adw]

I noticed that when I'm restoring all my windows on startup, in some of the windows, the web compat item is first in the panel, followed by a separator and then all the built-in items.  Also, in new windows, Pocket is now getting placed above the bookmark item, and I'm not sure when that started happening but I don't think it was always broken.

At least part of the problem is that BrowserPageActions.init incorrectly calls placeAction with only two arguments instead of three, and the second argument is wrong: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#51

I'll have a patch soon.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1391082 - Page action panel ordering can get messed up.

Review commit: https://reviewboard.mozilla.org/r/169370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169370/

Comment 2

[Drew Willcoxon :adw]

Work in progress.  Main part's done, working on a test.

Comment 3

[Drew Willcoxon :adw]

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

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8898068 [details]
Bug 1391082 - Page action panel ordering can get messed up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169370/diff/1-2/

Comment 5

[Drew Willcoxon :adw]

I think there are a couple of problems:

* BrowserPageActions.init incorrectly calls placeAction with only two arguments instead of three, and the second argument is wrong.

* An action added by an extension can get placed in a browser window before that window's BrowserPageActions.init is called -- before that window places all the built-in actions.  Which the code does not expect and I think ends up placing the extension's action before all the built-in actions in the panel.  I think that's what I'm seeing both when restoring all my windows on startup and with Pocket.  I'm not 100% sure, but it's plausible.

The fix for the first problem is pretty easy.  I added another method to PageActions to get the appropriate place to insert actions in the panel.

The second problem points to the fact that PageActions isn't handling the ordering of actions and insertion and removal as robustly as it should be.  So most of the changes in this patch address that, and so does the new test.  Those changes include:

* Stop handling separators specially -- don't assume they should always be appended, don't allow them not to have IDs, etc.

* Don't assume there are built-in actions.  (Shouldn't happen normally of course, but the test checks this.)

Comment 6

[:Gijs (he/him)]

Comment on attachment 8898068 [details]
Bug 1391082 - Page action panel ordering can get messed up.

https://reviewboard.mozilla.org/r/169370/#review175230

Very nice. Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a08bb8b25b9b
Page action panel ordering can get messed up. r=Gijs

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a08bb8b25b9b