1358097 - browser_BrowserUITelemetry_defaults.js fails when screenshots is enabled in tests

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[Mark Banner (:standard8)]

We're looking at enabling Screenshots in mochitests by default. In browser_BrowserUITelemetry_defaults.js we're seeing this test failure:

1879 INFO TEST-UNEXPECTED-FAIL | browser/modules/test/browser/browser_BrowserUITelemetry_defaults.js | No other test should have left CUI in a dirty state. - false == true - JS frame :: chrome://mochitests/content/browser/browser/modules/test/browser/browser_BrowserUITelemetry_defaults.js :: test :: line 15
Stack trace:
chrome://mochitests/content/browser/browser/modules/test/browser/browser_BrowserUITelemetry_defaults.js:test:15
chrome://mochikit/content/browser-test.js:Tester_execTest:771
chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest</<:672
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:791
Buffered messages finished

Comment 1

[Mark Banner (:standard8)]

I have a patch for this bug, however I think it depends on bug 1356256 as we're currently seeing inconsistent start-ups for Screenshots in tests.

Comment 2

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #1)
> I have a patch for this bug, however I think it depends on bug 1356256 as
> we're currently seeing inconsistent start-ups for Screenshots in tests.

Ok, I was wrong about that, I think we should land this now to avoid issues with restore defaults (Screenshots is currently disabled by default in tests, we'll get it enabled soon).

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1358097 - Add Screenshots to the navbar placements for Customizable UI when Screenshots is enabled.

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

Comment 4

[:Gijs (he/him)]

Comment on attachment 8860145 [details]
Bug 1358097 - Add Screenshots to the navbar placements for Customizable UI when Screenshots is enabled.

https://reviewboard.mozilla.org/r/132176/#review135272

::: browser/components/customizableui/CustomizableUI.jsm:260
(Diff revision 1)
> +    // Firefox Screenshots is a WebExtension, and WebExtensions place buttons
> +    // at the end of the toolbar. Apparently this happens after pocket's
> +    // createWidget call (see previous comment).

This sounds like a race condition waiting to happen, especially in the "migration" case for pocket (for profiles that had the add-on before).

We should stop hacking around this and fix bug 1247045 instead. Now that we're deprecating non-webextension buttons we can just change the API to no longer prevent opting in to "real" default buttons.

Comment 5

[Mark Banner (:standard8)]

I guess somewhere along the line this got fixed/worked around, since screenshots is now enabled by default.