874371 - (Australis) failing panorama tests checking for toolbar item presence on UX builds

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[:Gijs (he/him)]

Failures look like this:

21:41:56  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug626791.js | name-group: panorama button should be in the toolbar - Didn't expect -1, but got it
21:41:57  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug626791.js | drag-to-create-group: panorama button should be in the toolbar - Didn't expect -1, but got it
21:41:59  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug626791.js | create-orphan: panorama button should be in the toolbar - Didn't expect -1, but got it
21:42:00  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug626791.js | drag-to-create-orphan: panorama button should be in the toolbar - Didn't expect -1, but got it
21:42:01  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug626791.js | re-adding-after-removal: panorama button should be in the toolbar - Didn't expect -1, but got it

The problem is that browser-tabview.js ( http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabview.js#412 ) modifies currentset rather than using insertItem to insert its toolbar button.

I see two options:
1. Implement a setter for toolbar currentsets.
2. Change the panorama code to use insertItem instead.

I personally favour option 2, but I don't know if we're going to have to do 1 anyway for add-on compat, and some part of me feels a little uneasy changing other code to deal with our things. On top of that, without Australis, using insertItem is per-window, which is presumably what the currentset code was trying to avoid...

Comment 1

[:Gijs (he/him)]

Attachment: Patch

This switches to insertItem (and CustomizableUI for removing, because actually calling .remove() makes insertItem balk when we try to reinsert it).

Mike, do you think we can take this road, or should we cave and implement currentset setting? Is that even an option without accidentally overriding the pref when restoring a document with persisted setting stuff?

Comment 2

[:Gijs (he/him)]

Attachment: Patch with insertItem/CustomizableUI

Oops, forgot to check the alltabs button hasn't been taken out / moved...

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 752168 [details] [diff] [review]
Patch with insertItem/CustomizableUI

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

I think this is the preferred route. I think we want to get away from currentset and replace it with something better, and not re-implement it.

::: browser/base/content/browser-tabview.js
@@ +425,5 @@
>        return;
>  
> +    let allTabsBtn = document.getElementById("alltabs-button");
> +    let nextItem = allTabsBtn.nextSibling;
> +    toolbar.insertItem("tabview-button", nextItem);

We should use buttonId instead of "tabview-button".

::: browser/components/tabview/test/browser_tabview_bug626791.js
@@ +30,5 @@
>      let buttonId = "tabview-button";
>      let pos = currentSet.indexOf(buttonId);
>  
>      if (-1 < pos) {
> +      Cu.import("resource:///modules/CustomizableUI.jsm");

We're probably OK importing this at the top of the file.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 752168 [details] [diff] [review]
Patch with insertItem/CustomizableUI

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

What Mike said :) Thanks!

Comment 5

[:Gijs (he/him)]

Thanks Mike & Tim!

http://hg.mozilla.org/projects/ux/rev/1f1144b5a14f

Comment 6

[:Gijs (he/him)]

These have gone green, hurray!

Comment 7

[:Gijs (he/him)]

... except then it wasn't. Totally jinxed it. Will try to see what's wrong with the last failing test (seems to be somewhat intermittent?).

Comment 8

[:Gijs (he/him)]

The last failure looks like it was fixed by bug 874584...

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/1f1144b5a14f