Closed Bug 1324062 Opened 8 years ago Closed 7 years ago

Add "Report Site Issue" button to hamburger menu by default, for Nightly channel only.

Categories

(Web Compatibility :: Tooling & Investigations, defect)

Product:
Web Compatibility
Component:
Tooling & Investigations
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: miketaylr, Assigned: miketaylr)

References

Details

Attachments

(4 files, 9 obsolete files)

Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1324062. Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1324062. Part 3 - Add webcompat-reporter.xpi to xperf whitelist.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1324062. Part 4 - Add webcompat-reporter.xpi to addon-watch whitelist.
59 bytes, text/x-review-board-request
Yoric
: review+
Details 
Dunno if this should be in the Firefox Product or what. Anyone can feel free to move it!

This got UX sign-off from Sevaan (after we made a pile of changes to the add-on and to webcompat.com).

The idea is to have a button that says "Report Site Issue" in the hamburger menu by default, for the Nightly channel only. We may want to ship this further (DevEdition? Beta?) in the future, but want plenty of time to bake first.

How it should work:

1) Button should be disabled for all non-http or https URIs (i.e., we don't want bug reports for about:home, etc.)

2) When clicked, take a screenshot, open a new tab at https://webcompat.com/issues/new and pass along the screenshot and URI. If the screenshot failed, for some reason, it should still pass along the URI.

3) When a new issue is reported, it should send an "X-Reported-With" header to webcompat.com so we can measure what channels are being used to report bugs (and only send it then).
Blocks: 1306114
Attachment #8819375 - Attachment is obsolete: true
All the opt builds explode on this:

12:27:02     INFO -  /builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox/_virtualenv/bin/python /builds/slave/try-l64-0000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py /builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox/dist/l10n-stage/firefox ../../dist/xpi-stage/locale-x-test \
12:27:02     INFO -  	 \
12:27:04     INFO -  Error: Locale doesn't contain browser/features/webcompat-reporter@mozilla.org/
12:27:07     INFO -  Traceback (most recent call last):
12:27:07     INFO -    File "/builds/slave/try-l64-0000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py", line 60, in <module>
12:27:07     INFO -      main()
12:27:07     INFO -    File "/builds/slave/try-l64-0000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py", line 56, in main
12:27:07     INFO -      non_resources=args.non_resource, non_chrome=NON_CHROME)
12:27:07     INFO -    File "/builds/slave/try-l64-0000000000000000000000/build/src/python/mozbuild/mozpack/packager/l10n.py", line 257, in repack
12:27:07     INFO -      _repack(app_finder, l10n_finder, copier, formatter, non_chrome)
12:27:07     INFO -    File "/tools/python27/lib/python2.7/contextlib.py", line 24, in __exit__
12:27:07     INFO -      self.gen.next()
12:27:07     INFO -    File "/builds/slave/try-l64-0000000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 131, in accumulate
12:27:07     INFO -      raise AccumulatedErrors()
12:27:07     INFO -  mozpack.errors.AccumulatedErrors
12:27:07     INFO -  gmake[7]: *** [repackage-zip] Error 1
12:27:07     INFO -  gmake[7]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox/browser/locales'
12:27:07     INFO -  gmake[6]: *** [repackage-zip-x-test] Error 2
12:27:07     INFO -  gmake[6]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox/browser/locales'
12:27:07     INFO -  gmake[5]: *** [l10n-check] Error 2
12:27:07     INFO -  gmake[5]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox/browser/locales'
12:27:07     INFO -  gmake[4]: *** [l10n-check] Error 2
12:27:07     INFO -  gmake[4]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox'
12:27:07     INFO -  gmake[3]: *** [automation/l10n-check] Error 2
12:27:07     INFO -  gmake[3]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src/obj-firefox'
12:27:07     INFO -  gmake[2]: *** [automation/build] Error 2
12:27:07     INFO -  gmake[2]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src'
12:27:07     INFO -  gmake[1]: *** [realbuild] Error 2
12:27:07     INFO -  gmake[1]: Leaving directory `/builds/slave/try-l64-0000000000000000000000/build/src'
12:27:07     INFO -  gmake: *** [build] Error 2
12:27:07     INFO -  1160 compiler warnings present.
12:27:07     INFO -  Notification center failed: Install the python dbus module to get a notification when the build finishes.
12:27:07     INFO -  WARNING: Command failed. See logs for output.
12:27:07     INFO -   # umount -n /builds/mock_mozilla/mozilla-centos6-x86_64/root/builds/slave
12:27:07     INFO -  WARNING: Forcibly unmounting '/builds/mock_mozilla/mozilla-centos6-x86_64/root/builds/slave' from chroot.
12:27:07    ERROR - Return code: 2
12:27:07  WARNING - setting return code to 2
12:27:07    FATAL - 'mach build' did not run successfully. Please check log for errors.
12:27:07    FATAL - Running post_fatal callback...
12:27:07    FATAL - Exiting -1
12:27:07     INFO - [mozharness: 2016-12-16 20:27:07.449720Z] Finished build step (failed)
12:27:07     INFO - Running post-run listener: _summarize
12:27:07    ERROR - # TBPL FAILURE #
12:27:07     INFO - [mozharness: 2016-12-16 20:27:07.450401Z] FxDesktopBuild summary:
12:27:07    ERROR - # TBPL FAILURE #
12:27:07     INFO - Running post-run listener: copy_logs_to_upload_dir
12:27:07     INFO - Copying logs to upload dir...
12:27:07     INFO - mkdir: /builds/slave/try-l64-0000000000000000000000/build/upload/logs
program finished with exit code 255
Attachment #8819385 - Attachment is obsolete: true
Attachment #8819955 - Attachment is obsolete: true
Attachment #8819376 - Attachment is obsolete: true
Attachment #8819965 - Attachment is obsolete: true
Ah, forgot to make edits in browser/locales/Makefile.in. Green @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e00b892611b18f25f95a83bec1169d3ff30dc09
Comment on attachment 8819964 [details] [diff] [review]
Add a "Report Site Issue" button to the panel menu. r=?

Gijs, would you mind reviewing?
Attachment #8819964 - Flags: review?(gijskruitbosch+bugs)
Attachment #8820005 - Flags: review?(gijskruitbosch+bugs)
Attachment #8819377 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8819377 [details] [diff] [review]
Disable webcompat reporter during tests. r=?

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

We shouldn't disable it for all tests. This is the kind of thing that will lead to UI breakage (e.g. "restore defaults" being enabled in a clean profile).
Attachment #8819377 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8820005 [details] [diff] [review]
Enable webcompat reporter add-on for NIGHTLY_BUILD only. r=?

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

Please fold with the previous patch.
Attachment #8820005 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8819964 [details] [diff] [review]
Add a "Report Site Issue" button to the panel menu. r=?

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

Please use mozreview for future requests so that interdiffs work, and I don't have to review the entire 20k patch several times.

This needs tests.

This will break some existing tests.

::: browser/extensions/webcompat-reporter/bootstrap.js
@@ +8,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +  "resource:///modules/CustomizableUI.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm");

Please factor the guts of this code out into a separate file instead of having everything in bootstrap.js. Use a JSM and make sure you call Cu.unload() when the add-on is being unloaded, so that updates/upgrades work.

Nit: no point lazy-loading Services.jsm, just Cu.import it.

@@ +15,5 @@
> +  return Services.strings.createBundle("chrome://webcompat-reporter/locale/webcompat.properties");
> +});
> +
> +const PREF_WC_REPORTER_ENABLED = "extensions.webcompat-reporter.enabled";
> +const WEBCOMPAT_DOMAIN = "webcompat.com";

To test this, you'll have to have a pref to configure this, so we can add test hostnames that don't leave the network.

@@ +29,5 @@
> +
> +  addListeners() {
> +    this.browser.addTabsProgressListener(this);
> +    this.browser.tabContainer.addEventListener("TabSelect", (e) =>
> +      this.setButtonState(e));

Just pass the same type of argument as onLocationChange?

@@ +35,5 @@
> +
> +  removeListeners() {
> +    this.browser.removeTabsProgressListener(this);
> +    this.browser.tabContainer.removeEventListener("TabSelect",
> +      this.setButtonState);

This doesn't work because you're not removing what you added.

Convention is to implement handleEvent on the object and add/remove the object.

@@ +42,5 @@
> +  onLocationChange(browser, webProgress, request, uri, flags) {
> +    this.setButtonState(uri.scheme);
> +  }
> +
> +  static isValidScheme(scheme) {

nit: rename to something that more accurately describes what this does (e.g. "isReportableScheme").

@@ +48,5 @@
> +  }
> +
> +  setButtonState(arg) {
> +    // Bail early if the button is in the palette.
> +    if (CustomizableUI.getPlacementOfWidget(WIDGET_ID) === null) {

Style guide for Firefox code says: if (!...)

@@ +57,5 @@
> +    // or via onLocationChange (arg: string)?
> +    let scheme = typeof arg === "string" ? arg :
> +                 arg.target.linkedBrowser.currentURI.scheme;
> +    CustomizableUI.getWidget(WIDGET_ID).disabled =
> +      !TabListener.isValidScheme(scheme);

Please make sure we just pass the scheme, or the URI - or something consistent.

@@ +71,5 @@
> +  }
> +
> +  get observerService() {
> +    return Cc["@mozilla.org/observer-service;1"].
> +      getService(Ci.nsIObserverService);

Don't do this, use Services.obs. instead.

@@ +79,5 @@
> +    if (topic === this.TOPIC) {
> +      let httpChannel = subject.QueryInterface(Ci.nsIHttpChannel);
> +      if (httpChannel.originalURI.host === WEBCOMPAT_DOMAIN &&
> +        httpChannel.originalURI.path.startsWith("/issues/new")) {
> +        httpChannel.setRequestHeader("X-Reported-With", "desktop-reporter", false);

Why not just a custom URI parameter or redirect endpoint? That would be a lot simpler to implement and doesn't involve the heavyweight watching of every single http request.

@@ +105,5 @@
> +      onCommand: (e) => this.reportIssue(e.target.ownerDocument),
> +      onBeforeCreated(aDocument) {
> +        this._sheetURI = Services.io.newURI("chrome://webcompat-reporter/skin/lightbulb.css", null, null);
> +        aDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).
> +          getInterface(Ci.nsIDOMWindowUtils).loadSheet(this._sheetURI, 1);

Please just insert a DOM stylesheet instead of doing this.

@@ +123,5 @@
> +  },
> +
> +  // When we come out of Customize Mode, we need to make sure the
> +  // Report Site Issue button has the right state.
> +  // cf. https://dxr.mozilla.org/mozilla-central/rev/63b447888a6469b9f6ae8f76ac5f0d7c6ea239da/browser/base/content/browser-social.js#128

Have you tested if this is actually (still) necessary? You also have an onLocationChange and TabSelect handler, and at least one of those should be firing.

@@ +141,5 @@
> +
> +  onOpenWindow(window) {
> +    let win = window.QueryInterface(Ci.nsIInterfaceRequestor).
> +      getInterface(Ci.nsIDOMWindow);
> +    WebCompatReporter.loadIntoWindow(win);

Don't do this, use CustomizableUI's onWindowOpened instead.

@@ +198,5 @@
> +
> +    return new Promise((resolve) => {
> +      let mm = browser.selectedBrowser.messageManager;
> +
> +      mm.loadFrameScript(FRAMESCRIPT, true);

Rather than inserting a separate frame script each time, uniquely identifying it with a timestamp, and then telling the single browser message manager to keep loading that script for all eternity (ie second param true), why don't we permanently load a process script that just responds to a message with the right data? Have the sent message indicate which browser to take a screenshot of and we should be all good.

@@ +223,5 @@
> +      let loadedListener = {
> +        QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
> +          "nsISupportsWeakReference"]),
> +        onStateChange: function(webProgress, request, flag, status) {
> +          if (flag & Ci.nsIWebProgressListener.STATE_STOP) {

Is this guaranteed to fire? If not, the add-on will leak its world.

@@ +224,5 @@
> +        QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
> +          "nsISupportsWeakReference"]),
> +        onStateChange: function(webProgress, request, flag, status) {
> +          if (flag & Ci.nsIWebProgressListener.STATE_STOP) {
> +            if (request.originalURI && request.originalURI.host === WEBCOMPAT_DOMAIN) {

request.originalURI.host can throw. Check asciiHost instead. Also, don't you need to QI request?

@@ +230,5 @@
> +              browser.removeProgressListener(this);
> +
> +              // did we successfully get a screenshot?
> +              if (tabData && tabData.canvasData) {
> +                mm.loadFrameScript(FRAMESCRIPT, true);

again, it would make more sense to have a process script that you just tell about this stuff.

@@ +236,5 @@
> +                  screenshot: tabData.canvasData
> +                });
> +
> +                mm.addMessageListener(ACK_MESSAGE, function ackFn(message) {
> +                  mm.removeMessageListener(ACK_MESSAGE, ackFn);

Nothing cares about this promise and its resolution. Why is it returning a promise, and why the extra 'ack' message that doesn't otherwise serve any purpose?

@@ +248,5 @@
> +          }
> +        }
> +      };
> +
> +      httpRequestObserver.addObserver();

This and the progress listener will now fire for every tab in this window. Can you add it to just the new tab's browser instead?

@@ +257,5 @@
> +    });
> +  },
> +
> +  reportIssue(xulDoc) {
> +    Promise.resolve(xulDoc.defaultView.gBrowser).then(this.getScreenshot)

Why the Promise.resolve hoopjump? Just: this.getScreenshot(xulDoc.defaultView.gBrowser) seems like it should work fine.

@@ +290,5 @@
> +  Services.prefs.removeObserver(PREF_WC_REPORTER_ENABLED, prefObserver);
> +
> +  let enabled = Services.prefs.getBoolPref(PREF_WC_REPORTER_ENABLED);
> +  if (enabled) {
> +    WebCompatReporter.uninit();

We shouldn't run this code if we're shutting down the browser anyway.

::: browser/extensions/webcompat-reporter/content/tab-frame.js
@@ +17,5 @@
> +    canvas.width = dpr * w;
> +    canvas.height = dpr * h;
> +    ctx.scale(dpr, dpr);
> +    ctx.drawWindow(win, x, y, w, h, "#fff");
> +    let canvasData = canvas.toDataURL();

Please use a blob URI instead of a data URI. Make sure to call revokeBlobURI when we have passed the data to the website.

@@ +30,5 @@
> +  }
> +};
> +
> +// we only care about top level docs
> +if (content.location.href === content.top.location.href) {

Frame scripts in the message manager sense only run for toplevel docs, so I believe this check is superfluous.

@@ +32,5 @@
> +
> +// we only care about top level docs
> +if (content.location.href === content.top.location.href) {
> +  let [url, canvasData] = getScreenshot(content);
> +  sendAsyncMessage(TABDATA_MESSAGE, {url: url, canvasData: canvasData});

We live in an ES-whatever-they-call-it-now world, so don't repeat object literal arguments where the name of the value is identical (so: {url, canvasData}, please).

Come to think of it, why can't getScreenshot return the object you need here directly? Then this can just be a 1-liner.

::: browser/extensions/webcompat-reporter/content/wc-frame.js
@@ +7,5 @@
> +addMessageListener(SCREENSHOT_MESSAGE, function handleMessage(message) {
> +  removeMessageListener(SCREENSHOT_MESSAGE, handleMessage);
> +  // waive x-rays so event.message.origin is non-empty on the receiving side.
> +  // see https://github.com/webcompat/webcompat.com/blob/0c5f5117fb14cd4777aa68a861cd8126d64c3c59/webcompat/static/js/lib/bugform.js#L81
> +  let win = Components.utils.waiveXrays(content);

r- just for this. It shouldn't be necessary to do this. This will effectively call postMessage (with chrome privileges) even if it's a user-defined function.

::: browser/extensions/webcompat-reporter/skin/lightbulb.css
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +
> +@-moz-document url("chrome://browser/content/browser.xul") {

Don't need this.

::: browser/extensions/webcompat-reporter/skin/lightbulb.svg
@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="32px" height="32px" viewBox="6.5 210 92 147">

Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines adn fix this SVG so as to remove the viewbox. I don't think you need/want the width/height specifier, either.

@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="32px" height="32px" viewBox="6.5 210 92 147">
> +  <g id="lightbulb">
> +    <g id="base">
> +      <path fill="#000000" fill-opacity=".6" d="M76.3,251.3v-8.1l-48.3,8.9v8L76.3,251.3z M76.3,232.9l-48.3,8.9v8l48.3-8.9V232.9z M73,231.2 c0-0.1,0-0.2,0-0.3c0-2-0.3-3.8-0.9-5.6l0,0v0c-0.1-0.2-0.2-0.4-0.2-0.6c-0.1-0.3-0.2-0.7-0.4-1c0-0.1-0.1-0.2-0.1-0.2l0,0         c-3.2-7-10.6-12-19.4-12c-9.5,0-17.5,5.8-20.1,13.8h29.4l-30.3,5.6l-3,0.5v8.1l4.7-0.9L73,231.2z  "/>

Nit: spacing.

Nit: you're going to want different coloring per platform.
Attachment #8819964 - Flags: review?(gijskruitbosch+bugs) → review-
Awesome feedback, thanks Gijs.
Attachment #8819377 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 8819964 [details] [diff] [review]
> Add a "Report Site Issue" button to the panel menu. r=?
> 
> Review of attachment 8819964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please use mozreview for future requests so that interdiffs work, and I
> don't have to review the entire 20k patch several times.

OK, will do.

> @@ +79,5 @@
> > +    if (topic === this.TOPIC) {
> > +      let httpChannel = subject.QueryInterface(Ci.nsIHttpChannel);
> > +      if (httpChannel.originalURI.host === WEBCOMPAT_DOMAIN &&
> > +        httpChannel.originalURI.path.startsWith("/issues/new")) {
> > +        httpChannel.setRequestHeader("X-Reported-With", "desktop-reporter", false);
> 
> Why not just a custom URI parameter or redirect endpoint? That would be a
> lot simpler to implement and doesn't involve the heavyweight watching of
> every single http request.

There's not a great reason other than the fact that I think GET params can be ugly. :| 

It *would* make it simpler to just add on a &reported-with=desktop-reporter... I filed https://github.com/webcompat/webcompat.com/issues/1249 to make the necessary changes on the webcompat.com server side.


> @@ +105,5 @@
> > +      onCommand: (e) => this.reportIssue(e.target.ownerDocument),
> > +      onBeforeCreated(aDocument) {
> > +        this._sheetURI = Services.io.newURI("chrome://webcompat-reporter/skin/lightbulb.css", null, null);
> > +        aDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).
> > +          getInterface(Ci.nsIDOMWindowUtils).loadSheet(this._sheetURI, 1);
> 
> Please just insert a DOM stylesheet instead of doing this.

Do you mean via nsIDOMWindowUtils.addSheet? 
https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/bootstrap.js#458-461

 
> @@ +123,5 @@
> > +  },
> > +
> > +  // When we come out of Customize Mode, we need to make sure the
> > +  // Report Site Issue button has the right state.
> > +  // cf. https://dxr.mozilla.org/mozilla-central/rev/63b447888a6469b9f6ae8f76ac5f0d7c6ea239da/browser/base/content/browser-social.js#128
> 
> Have you tested if this is actually (still) necessary? You also have an
> onLocationChange and TabSelect handler, and at least one of those should be
> firing.

Yeah, unfortunately this tripped me up for quite some time. TabHandler.setButtonState does fire as expected post-customize, but setting CustomizableUI.getWidget(FOO).disabled in the handler doesn't end up enabling/disabling the widget.

> @@ +223,5 @@
> > +      let loadedListener = {
> > +        QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
> > +          "nsISupportsWeakReference"]),
> > +        onStateChange: function(webProgress, request, flag, status) {
> > +          if (flag & Ci.nsIWebProgressListener.STATE_STOP) {
> 
> Is this guaranteed to fire? If not, the add-on will leak its world.

Not sure!


> @@ +236,5 @@
> > +                  screenshot: tabData.canvasData
> > +                });
> > +
> > +                mm.addMessageListener(ACK_MESSAGE, function ackFn(message) {
> > +                  mm.removeMessageListener(ACK_MESSAGE, ackFn);
> 
> Nothing cares about this promise and its resolution. Why is it returning a
> promise, and why the extra 'ack' message that doesn't otherwise serve any
> purpose?

Oh yeah, good catch. Leftovers from an earlier iteration.


> ::: browser/extensions/webcompat-reporter/content/tab-frame.js
> @@ +17,5 @@
> > +    canvas.width = dpr * w;
> > +    canvas.height = dpr * h;
> > +    ctx.scale(dpr, dpr);
> > +    ctx.drawWindow(win, x, y, w, h, "#fff");
> > +    let canvasData = canvas.toDataURL();
> 
> Please use a blob URI instead of a data URI. Make sure to call revokeBlobURI
> when we have passed the data to the website.

Roger that. Filed https://github.com/webcompat/webcompat.com/issues/1248 to handle from the webcompat.com backend/frontend.

> ::: browser/extensions/webcompat-reporter/content/wc-frame.js
> @@ +7,5 @@
> > +addMessageListener(SCREENSHOT_MESSAGE, function handleMessage(message) {
> > +  removeMessageListener(SCREENSHOT_MESSAGE, handleMessage);
> > +  // waive x-rays so event.message.origin is non-empty on the receiving side.
> > +  // see https://github.com/webcompat/webcompat.com/blob/0c5f5117fb14cd4777aa68a861cd8126d64c3c59/webcompat/static/js/lib/bugform.js#L81
> > +  let win = Components.utils.waiveXrays(content);
> 
> r- just for this. It shouldn't be necessary to do this. This will
> effectively call postMessage (with chrome privileges) even if it's a
> user-defined function.

I can hack around this on the webcompat.com side (and maybe it's not so necessary to be paranoid about postMessage'd image content, since there's a file picker anyways?). But without calling waiveXrays, event.origin is "", so I can't do clientside origin checks from my message event handler.

(Note, when doing the same thing as a WebExtension, event.origin === location.origin, aka https://webcompat.com: https://github.com/webcompat/webcompat-reporter-extensions/blob/master/firefox/background.js#L21).
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #15)
> ::: browser/extensions/webcompat-reporter/skin/lightbulb.svg
> @@ +1,5 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="32px" height="32px" viewBox="6.5 210 92 147">
> 
> Please see
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines adn fix this SVG so as to remove the viewbox. I don't think
> you need/want the width/height specifier, either.

I'll see what I can do, I'm terrible with SVG (I did some cleanup, but viewBox is intact).

> Nit: you're going to want different coloring per platform.

Blake, are there UX docs on conventions for cross-platform icons?
Flags: needinfo?(bwinton)
Not really, at least not that I know of.  There's http://firefoxux.github.io/StyleGuide/#/icons but it's not using the right colours, and doesn't give a lot of guidelines.  (It's more examples.)  Fortunately, the team Markus is on is handling that kind of thing, so let's ask him.  :)
Flags: needinfo?(bwinton) → needinfo?(mjaritz)
Handing to Emanuela who will get you the color values and update them in the styleguide.
Flags: needinfo?(mjaritz) → needinfo?(emanuela)
(In reply to Mike Taylor [:miketaylr] from comment #17)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Comment on attachment 8819964 [details] [diff] [review]
> > Add a "Report Site Issue" button to the panel menu. r=?
> > 
> > Review of attachment 8819964 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please use mozreview for future requests so that interdiffs work, and I
> > don't have to review the entire 20k patch several times.
> 
> OK, will do.
> 
> > @@ +79,5 @@
> > > +    if (topic === this.TOPIC) {
> > > +      let httpChannel = subject.QueryInterface(Ci.nsIHttpChannel);
> > > +      if (httpChannel.originalURI.host === WEBCOMPAT_DOMAIN &&
> > > +        httpChannel.originalURI.path.startsWith("/issues/new")) {
> > > +        httpChannel.setRequestHeader("X-Reported-With", "desktop-reporter", false);
> > 
> > Why not just a custom URI parameter or redirect endpoint? That would be a
> > lot simpler to implement and doesn't involve the heavyweight watching of
> > every single http request.
> 
> There's not a great reason other than the fact that I think GET params can
> be ugly. :| 
> 
> It *would* make it simpler to just add on a
> &reported-with=desktop-reporter... I filed
> https://github.com/webcompat/webcompat.com/issues/1249 to make the necessary
> changes on the webcompat.com server side.

Cool. Another option (to avoid evil get params :-) ) would be pointing to a dedicated endpoint, like webcompat.com/firefox-addon-report (please fix my terrible example slug) and redirecting that to issues/new, while setting a cookie or reading the Referer header from the endpoint page.

> > @@ +105,5 @@
> > > +      onCommand: (e) => this.reportIssue(e.target.ownerDocument),
> > > +      onBeforeCreated(aDocument) {
> > > +        this._sheetURI = Services.io.newURI("chrome://webcompat-reporter/skin/lightbulb.css", null, null);
> > > +        aDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).
> > > +          getInterface(Ci.nsIDOMWindowUtils).loadSheet(this._sheetURI, 1);
> > 
> > Please just insert a DOM stylesheet instead of doing this.
> 
> Do you mean via nsIDOMWindowUtils.addSheet? 
> https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/
> bootstrap.js#458-461

No, I meant:

let processingData = `type="text/css" href="${kSheetURL}"`;
let pi = document.createProcessingInstruction("xml-stylesheet", processingData);
document.insertBefore(pi, document.documentElement);


> > @@ +123,5 @@
> > > +  },
> > > +
> > > +  // When we come out of Customize Mode, we need to make sure the
> > > +  // Report Site Issue button has the right state.
> > > +  // cf. https://dxr.mozilla.org/mozilla-central/rev/63b447888a6469b9f6ae8f76ac5f0d7c6ea239da/browser/base/content/browser-social.js#128
> > 
> > Have you tested if this is actually (still) necessary? You also have an
> > onLocationChange and TabSelect handler, and at least one of those should be
> > firing.
> 
> Yeah, unfortunately this tripped me up for quite some time.
> TabHandler.setButtonState does fire as expected post-customize, but setting
> CustomizableUI.getWidget(FOO).disabled in the handler doesn't end up
> enabling/disabling the widget.

Have you tried setting the "disabled" attribute explicitly instead of via the property? Have you debugged why it's failing? We disable stuff in customize mode generally (so you can't accidentally click buttons while customizing) but I would kind of expect the events here to fire after we've left customize mode. I guess it's possible they fire before we've fully left, which is unfortunate and probably not easy to fix...

> > ::: browser/extensions/webcompat-reporter/content/tab-frame.js
> > @@ +17,5 @@
> > > +    canvas.width = dpr * w;
> > > +    canvas.height = dpr * h;
> > > +    ctx.scale(dpr, dpr);
> > > +    ctx.drawWindow(win, x, y, w, h, "#fff");
> > > +    let canvasData = canvas.toDataURL();
> > 
> > Please use a blob URI instead of a data URI. Make sure to call revokeBlobURI
> > when we have passed the data to the website.
> 
> Roger that. Filed https://github.com/webcompat/webcompat.com/issues/1248 to
> handle from the webcompat.com backend/frontend.

Ah, so, you might still need to (in the chrome/add-on code) go back from a Blob URI to a File, or marshal to a new Blob URI, depending on what origin the Blob URI gets created as in the current setup, in order to get the web page to have access to the Blob/File.

> > ::: browser/extensions/webcompat-reporter/content/wc-frame.js
> > @@ +7,5 @@
> > > +addMessageListener(SCREENSHOT_MESSAGE, function handleMessage(message) {
> > > +  removeMessageListener(SCREENSHOT_MESSAGE, handleMessage);
> > > +  // waive x-rays so event.message.origin is non-empty on the receiving side.
> > > +  // see https://github.com/webcompat/webcompat.com/blob/0c5f5117fb14cd4777aa68a861cd8126d64c3c59/webcompat/static/js/lib/bugform.js#L81
> > > +  let win = Components.utils.waiveXrays(content);
> > 
> > r- just for this. It shouldn't be necessary to do this. This will
> > effectively call postMessage (with chrome privileges) even if it's a
> > user-defined function.
> 
> I can hack around this on the webcompat.com side (and maybe it's not so
> necessary to be paranoid about postMessage'd image content, since there's a
> file picker anyways?). But without calling waiveXrays, event.origin is "",
> so I can't do clientside origin checks from my message event handler.

Hm. What happens if you do something like this:

let sb = new Cu.Sandbox(content.document.nodePrincipal);
sb.win = content;
sb.myData = imageData;
Cu.evalInSandbox("win.postMessage(myData);", sb);

or this:

content.__myImageData = imageData;
content.eval("window.postMessage(__myImageData)");

?

> (Note, when doing the same thing as a WebExtension, event.origin ===
> location.origin, aka https://webcompat.com:
> https://github.com/webcompat/webcompat-reporter-extensions/blob/master/
> firefox/background.js#L21).

OK, so maybe switching to do this entirely to a webextension would make sense? What are you missing in order to do that? Alternatively, maybe we can channel this stuff through an/the embedded webextension?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Mike Taylor [:miketaylr] from comment #17)
> > > > +  let win = Components.utils.waiveXrays(content);
> > > 
> > > r- just for this. It shouldn't be necessary to do this. This will
> > > effectively call postMessage (with chrome privileges) even if it's a
> > > user-defined function.
> > 
> > I can hack around this on the webcompat.com side (and maybe it's not so
> > necessary to be paranoid about postMessage'd image content, since there's a
> > file picker anyways?). But without calling waiveXrays, event.origin is "",
> > so I can't do clientside origin checks from my message event handler.
> 
> Hm. What happens if you do something like this:
> 
> let sb = new Cu.Sandbox(content.document.nodePrincipal);
> sb.win = content;
> sb.myData = imageData;
> Cu.evalInSandbox("win.postMessage(myData);", sb);
> 
> or this:
> 
> content.__myImageData = imageData;
> content.eval("window.postMessage(__myImageData)");
> 
> ?

Eh, for the eval case you might need to:

content.__myImageData = Cu.cloneInto(imageData, content);

explicitly, in order to pass the param. I believe the sandbox expando stuff should take care of that automatically.
(In reply to :Gijs Kruitbosch from comment #21)
> let processingData = `type="text/css" href="${kSheetURL}"`;
> let pi = document.createProcessingInstruction("xml-stylesheet",
> processingData);
> document.insertBefore(pi, document.documentElement);

Thanks!

> Have you tried setting the "disabled" attribute explicitly instead of via
> the property? Have you debugged why it's failing? We disable stuff in
> customize mode generally (so you can't accidentally click buttons while
> customizing) but I would kind of expect the events here to fire after we've
> left customize mode. I guess it's possible they fire before we've fully
> left, which is unfortunate and probably not easy to fix...

I'm pretty sure I did and failed, but I can re-test. Debugging didn't get me very far, but probably because I'm so new to the Firefox front-end
 
> Ah, so, you might still need to (in the chrome/add-on code) go back from a
> Blob URI to a File, or marshal to a new Blob URI, depending on what origin
> the Blob URI gets created as in the current setup, in order to get the web
> page to have access to the Blob/File.

k, noted.

> OK, so maybe switching to do this entirely to a webextension would make
> sense? What are you missing in order to do that? Alternatively, maybe we can
> channel this stuff through an/the embedded webextension?

Bug 1313459 is the blocker. UX wants this to be in the menu by default and there's no APIs for that. Let me see how much work it is to implement the rest of the changes before learning how to write embedded webextensions or webextension experiments. :)
Attachment #8822250 - Flags: review?(gijskruitbosch+bugs)
Attachment #8822251 - Flags: review?(gijskruitbosch+bugs)
OK, feedback address and tests added.

(In reply to :Gijs (gone until 3 jan) from comment #15)
> @@ +198,5 @@
> > +
> > +    return new Promise((resolve) => {
> > +      let mm = browser.selectedBrowser.messageManager;
> > +
> > +      mm.loadFrameScript(FRAMESCRIPT, true);
> 
> Rather than inserting a separate frame script each time, uniquely
> identifying it with a timestamp, and then telling the single browser message
> manager to keep loading that script for all eternity (ie second param true),
> why don't we permanently load a process script that just responds to a
> message with the right data? Have the sent message indicate which browser to
> take a screenshot of and we should be all good.

Talking this over with blassey, he recommended against a process script. We don't expect people to click on this button every time they use the browser, so it creates unneeded process overhead. I did change this to mm.loadFrameScript(FRAMESCRIPT, false) so we're not injecting it on every new frame (and removed the timestamp -- that was a workaround to a caching bug during dev).

> Nit: you're going to want different coloring per platform.

Hopefully someone from UX can point me in the right direction here, I'm not sure what platform requirements are (apart from what I see in Pocket, for example).
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review101730

::: browser/extensions/webcompat-reporter/bootstrap.js:11
(Diff revision 1)
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "WebCompatReporter",
> + "chrome://webcompat-reporter/content/WebCompatReporter.jsm");

drive by nit: repeated on line 11 and 40. 

Ref: https://dxr.mozilla.org/mozilla-central/source/browser/extensions/presentation/bootstrap.js#10

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:26
(Diff revision 1)
> +const WEBCOMPAT_ENDPOINT = Services.urlFormatter.formatURLPref(
> +  "extensions.webcompat-reporter.newIssueEndpoint");
> +
> +let WebCompatReporter = {
> +  init() {
> +    Cu.import("chrome://webcompat-reporter/content/TabListener.jsm");

`"chrome://webcompat-reporter/content/TabListener.jsm"` repeated on line 26 and 72

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:37
(Diff revision 1)
> +      defaultArea: CustomizableUI.AREA_PANEL,
> +      disabled: true,
> +      onCommand: (e) => this.reportIssue(e.target.ownerDocument),
> +      onBeforeCreated(document) {
> +        let ssURI = "chrome://webcompat-reporter/skin/lightbulb.css";
> +        let data = `type="text/css" href="${ssURI}"`;

drive by nit: does ssURI need to be inside `onBeforeCreated` ?
Attachment #8822251 - Attachment is obsolete: true
Attachment #8822251 - Flags: review?(gijskruitbosch+bugs)
I'm gonna squash this all into a single commit, it doesn't make much sense to split it up, the more that I think about it.

(also, try run that actually builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d167aa807b953f8d99fe612822bc27a22006c1bf)
Attachment #8819964 - Attachment is obsolete: true
Attachment #8820005 - Attachment is obsolete: true
See Also: → 1229236
Attachment #8822533 - Flags: review?(gijskruitbosch+bugs)
Don't quite understand the Linux64 opt failure yet: TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js | Should be in the default state -, it passes for me on my Ubuntu64 VM.

However, it did reveal this funkiness: https://cloudup.com/c4LSp6kbS4C
```
Report Site Is-
Sue
```
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review102082

::: browser/extensions/webcompat-reporter/bootstrap.js:11
(Diff revision 1)
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "WebCompatReporter",
> + "chrome://webcompat-reporter/content/WebCompatReporter.jsm");

Good point!

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:26
(Diff revision 1)
> +const WEBCOMPAT_ENDPOINT = Services.urlFormatter.formatURLPref(
> +  "extensions.webcompat-reporter.newIssueEndpoint");
> +
> +let WebCompatReporter = {
> +  init() {
> +    Cu.import("chrome://webcompat-reporter/content/TabListener.jsm");

Thanks!

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:37
(Diff revision 1)
> +      defaultArea: CustomizableUI.AREA_PANEL,
> +      disabled: true,
> +      onCommand: (e) => this.reportIssue(e.target.ownerDocument),
> +      onBeforeCreated(document) {
> +        let ssURI = "chrome://webcompat-reporter/skin/lightbulb.css";
> +        let data = `type="text/css" href="${ssURI}"`;

Yeah, it could live outside.
Comment on attachment 8822533 [details]
Bug 1324062. Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button.

https://reviewboard.mozilla.org/r/101410/#review102086

::: browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js:16
(Diff revision 4)
>  
>  // When we drag an item onto a customizable area, and not over a specific target, we
>  // should assume that we're appending them to the area. If doing so, we should scan
>  // backwards over any hidden items and insert the item before those hidden items.
>  add_task(function*() {
> +  // This can fail if CustomizableUI.PROVIDER_API widgets are added as defaults,

So, this is kind of a theory. I can't get the failures to repro on my local Ubuntu builds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=741bdb021b36e063da583fdabd770a7ce4ef67ce to test the theory.
Comment on attachment 8822533 [details]
Bug 1324062. Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button.

https://reviewboard.mozilla.org/r/101410/#review102086

> So, this is kind of a theory. I can't get the failures to repro on my local Ubuntu builds.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=741bdb021b36e063da583fdabd770a7ce4ef67ce to test the theory.

Eh, nevermind. It still failed. Tempted to just make that test ignore the button.:(
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review102416

Apologies for the delay here while I was away and now while I am digging through bugmail etc.

This is pretty close already, but considering the number of issues, I guess it's a good idea for me to look at it again before it lands.

::: browser/extensions/webcompat-reporter/content/TabListener.jsm:62
(Diff revision 7)
> +    CustomizableUI.getWidget(WIDGET_ID).disabled =
> +      !TabListener.isReportableScheme(scheme);

This will enable/disable the button in every browser window, so it'll do the wrong thing if one window should have the button enabled and another shouldn't.

Use the

```js
CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).disabled
```

setter instead.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:21
(Diff revision 7)
> +const WEBCOMPAT_ENDPOINT = Services.urlFormatter.formatURLPref(
> +  "extensions.webcompat-reporter.newIssueEndpoint");

I think this would be better off as a getter on the web compat reporter that always reads the pref anew. This way, you can set the pref to a test value and test it. Right now, it'll just take whatever you put in the default preferences and changing the pref afterwards will not affect the value we use.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:38
(Diff revision 7)
> +        let data = `type="text/css" href="${LIGHTBULB_CSS}"`;
> +        WebCompatReporter.ss = document.createProcessingInstruction(
> +          "xml-stylesheet", data);
> +        document.insertBefore(WebCompatReporter.ss, document.documentElement);

I think onWindowOpened is a slightly better place to put this.

Also, right now, if you were to enable the add-on, disable it, and enable it again, we add the stylesheet twice. Can we avoid doing that?

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:59
(Diff revision 7)
> +      win._webcompatReporterTabListener = new TabListener(win);
> +    }
> +  },
> +
> +  onWindowClosed(win) {
> +    if (win.gBrowser && win._webcompatReporterTabListener) {

CUI should only be giving you browser windows, so here and in onWindowOpened you shouldn't need to check for `gBrowser`.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:76
(Diff revision 7)
> +
> +    CustomizableUI.removeListener(this);
> +    Cu.unload(TABLISTENER_JSM);
> +  },
> +
> +  // This method injects a framescript that should send back a dataURL

Nit: this comment still says data URL.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:79
(Diff revision 7)
> +  },
> +
> +  // This method injects a framescript that should send back a dataURL
> +  // screenshot of the top-level window of the currently selected tab,
> +  // resolved as a Promise.
> +  getScreenshot(browser) {

Nit: Can you make the arg be 'gBrowser' or 'tabbrowser' to make it clear it's a tabbrowser? Ditto for `openWebCompatTab`.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:100
(Diff revision 7)
> +  // 1) set up listeners for a new webcompat.com tab, and open it, passing
> +  //    along the current URI
> +  // 2) if we successfully got a screenshot from getScreenshot,
> +  //    inject a frame script that will postMessage it to webcompat.com
> +  //    so it can show a preview to the user and include it in FormData
> +  openWebCompatTab(args) {

yay es6:

```js
openWebCompatTab([browser, tabData]) {
```

Nit: please also add a comment above this function saying that arguments are in an array (rather than 'normal') because they are the result of a promise resolution.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:104
(Diff revision 7)
> +  //    so it can show a preview to the user and include it in FormData
> +  openWebCompatTab(args) {
> +    const SCREENSHOT_MESSAGE = "WebCompat:SendScreenshot";
> +    const FRAMESCRIPT = "chrome://webcompat-reporter/content/wc-frame.js";
> +    let [browser, tabData] = args;
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");

In theory, I can click the 'report' button and then close my browser window (on OS X, the process and all this code will stay alive). Because fetching the screenshot is async, it might race.

Right now, I think this will just throw. Making it do something better can be a followup - I seem to have forgotten how to deal with that (short of rolling your own openWindow call - but it seems there should be something better than that). :-(

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:111
(Diff revision 7)
> +
> +    let loadedListener = {
> +      QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
> +        "nsISupportsWeakReference"]),
> +      onStateChange: function(webProgress, request, flag, status) {
> +        if (flag & Ci.nsIWebProgressListener.STATE_STOP) {

I think you should also check the webProgress's isTopLevel property, to avoid this code triggering on asset files or frames that you might request from the web compat domain (I realize you probably use CDNs, and are unlikely to use frames, but the code right now would, AIUI, fire for any old file, and I presume we really care about the STATE_STOP of the main document).

You might also want to check for either the network or document flag. We'll need some way of ensuring that our sending the screenshot data doesn't race with the web compat web code having set up a listener for such data. When/how is the web compat web code doing that?

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:118
(Diff revision 7)
> +          try {
> +            location = request.QueryInterface(Ci.nsIChannel).URI;
> +          } catch (ex) {}
> +
> +          if (location && location.prePath === WEBCOMPAT_ORIGIN) {
> +            let mm = browser.selectedBrowser.messageManager;

Nit: move this right before the mm.loadFrameScript call.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:122
(Diff revision 7)
> +          if (location && location.prePath === WEBCOMPAT_ORIGIN) {
> +            let mm = browser.selectedBrowser.messageManager;
> +            browser.removeProgressListener(this);
> +
> +            // did we successfully get a screenshot?
> +            if (tabData && tabData.blob) {

We can do this check a lot earlier, can't we, and avoid adding the progress listener at all? That seems like it'd be simpler.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:133
(Diff revision 7)
> +          }
> +        }
> +      }
> +    };
> +
> +    browser.addProgressListener(loadedListener);

This still adds the progress listener to the tabbrowser (gBrowser). Can you add it to the newly opened tab instead? `loadOneTab` returns a tab, and you can use browser.getBrowserForTab(tab) to get the right single browser.

::: browser/extensions/webcompat-reporter/locale/jar.mn:7
(Diff revision 7)
> +% locale webcompat-reporter @AB_CD@ %locale/@AB_CD@/
> +  locale/@AB_CD@/                    (en-US/*)

This looks like we'll always package en-US for whatever language we use... is this just a placeholder for when we add more localizations? If you haven't yet, can you check with flod how to deal with this?

::: browser/extensions/webcompat-reporter/skin/lightbulb.svg:5
(Diff revision 7)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 85 128">

You can check with shorlander to get versions of this  SVG that are correctly coloured for each OS (version), plus an inverted one for dark lightweight themes / devedition.

::: browser/extensions/webcompat-reporter/test/browser/browser_disabled_cleanup.js:4
(Diff revision 7)
> +registerCleanupFunction(function() {
> +  SpecialPowers.clearUserPref(PREF_WC_REPORTER_ENABLED);
> +});

You can omit this, and use:

```js
yield SpecialPowers.pushPrefEnv({set: [[PREF_WC_REPORTER_ENABLED, false]]});
```

in the test instead.

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:9
(Diff revision 7)
> +/* Test that clicking on the Report Site Issue button opens a new tab
> +   and sends a postMessaged blob at it.
> +   testing/profiles/prefs_general.js sets the value for
> +   "extensions.webcompat-reporter.newIssueEndpoint" */
> +add_task(function* test_screenshot() {
> +  const TEST_PAGE = "http://example.com/browser/browser/extensions/webcompat-reporter/test/browser/test.html";

Nit: please use:

```js
const TEST_ROOT = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");
const TEST_PAGE = TEST_ROOT + "test.html";
```

(ditto to get the URL of the webcompat.html page)

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:18
(Diff revision 7)
> +  yield BrowserTestUtils.waitForCondition(function() {
> +    return gBrowser.currentURI.path.includes("webcompat.html") &&
> +      gBrowser.selectedBrowser.contentWindow.document.readyState === "complete";
> +  }, "Wait until the new issue endpoint is opened and loaded.");

waitForCondition is frowned upon, unless unavoidable, because on our test machines (and particularly on ASAN or debug builds) setTimeout is unreliable, so repeatedly waiting for a timeout leads to test timeouts and thereby intermittent/random orange.

In this case, I think you basically want:

```js
let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser);

// click button here.

let tab = yield newTabPromise;
yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
```

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:23
(Diff revision 7)
> +  let testPageDoc = gBrowser.selectedBrowser.contentWindow.document;
> +
> +  yield BrowserTestUtils.waitForCondition(function() {
> +    return testPageDoc.getElementById("url") !== "";
> +  }, "Wait until the test script has populated the url div.");

This is not a good idea in e10s, where ideally we shouldn't be touching the content document directly. Use something like this instead:

```js
yield BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "ReceivedScreenshot");
```

and dispatch a `new CustomEvent("ReceivedScreenshot")` from the content page when you receive the message you're waiting for.

This can probably replace waiting for the content to load (ie the browserLoaded bit in my previous suggestion), and if you fire the event after the read of the blob has succeeded, you can also remove the other waitForCondition below.

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:29
(Diff revision 7)
> +  let urlParam = testPageDoc.getElementById("url").innerText;
> +  is(urlParam, TEST_PAGE, "Reported page is added to url param");
> +
> +  let preview = testPageDoc.getElementById("screenshot-preview");
> +
> +  yield BrowserTestUtils.waitForCondition(function() {
> +    return preview.style.background !== "";
> +  }, "Wait until the blob is converted to base64 and set as background of preview div.");
> +
> +  is(preview.innerText, "Pass", "A Blob object was successfully transferred to the test page.")
> +  isnot(preview.innerText, "Fail", "A Blob object was successfully transferred to the test page.")
> +  ok(preview.style.background.includes("data:image/png;base64,iVBOR"), "A green screenshot was successfully postMessaged");

So instead of getting (CPOW) references to the content process stuff here, you can use a content task:

```js
yield ContentTask.spawn(tab.linkedBrowser, null, function() {
  let doc = content.document;
  let urlParam = doc.getElementById("url").innerText;
  is(urlParam, ...);
  
  // etc.
});
```

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:39
(Diff revision 7)
> +  yield BrowserTestUtils.waitForCondition(function() {
> +    return preview.style.background !== "";
> +  }, "Wait until the blob is converted to base64 and set as background of preview div.");
> +
> +  is(preview.innerText, "Pass", "A Blob object was successfully transferred to the test page.")
> +  isnot(preview.innerText, "Fail", "A Blob object was successfully transferred to the test page.")

If `is(foo, "Pass")` succeeded, clearly `isnot(foo, "something else")` will also succeed, so please omit this line. :-)

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:40
(Diff revision 7)
> +    return preview.style.background !== "";
> +  }, "Wait until the blob is converted to base64 and set as background of preview div.");
> +
> +  is(preview.innerText, "Pass", "A Blob object was successfully transferred to the test page.")
> +  isnot(preview.innerText, "Fail", "A Blob object was successfully transferred to the test page.")
> +  ok(preview.style.background.includes("data:image/png;base64,iVBOR"), "A green screenshot was successfully postMessaged");

Nitty nit: startsWith rather than includes.

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:42
(Diff revision 7)
> +
> +  is(preview.innerText, "Pass", "A Blob object was successfully transferred to the test page.")
> +  isnot(preview.innerText, "Fail", "A Blob object was successfully transferred to the test page.")
> +  ok(preview.style.background.includes("data:image/png;base64,iVBOR"), "A green screenshot was successfully postMessaged");
> +
> +  gBrowser.removeCurrentTab();

Nit: remove with BTU.removeTab().

::: browser/extensions/webcompat-reporter/test/browser/head.js:1
(Diff revision 7)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

This is the default for new test files, so you don't strictly need this disclaimer ( https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ ).

::: browser/extensions/webcompat-reporter/test/browser/head.js:7
(Diff revision 7)
> +function panelShown(window) {
> +  return new Promise(resolve => {
> +    PanelUI.panel.addEventListener("popupshown", function onPopUpShown() {
> +      PanelUI.panel.removeEventListener("popupshown", onPopUpShown);
> +        resolve();
> +    });
> +    PanelUI.show();
> +  });
> +}

```js
return PanelUI.show();
```

does this already (it returns a promise that resolves when the panel has been shown).

::: browser/extensions/webcompat-reporter/test/browser/head.js:21
(Diff revision 7)
> +
> +function isButtonDisabled() {
> +  return document.getElementById("webcompat-reporter-button").disabled;
> +}
> +
> +function waitForCondition(aConditionFn, aMaxTries = 50, aCheckInterval = 100) {

Nit: You don't use this (as you're using BTU's version).

::: browser/extensions/webcompat-reporter/test/browser/webcompat.html:23
(Diff revision 7)
> +  reader.onload = e => preview.style.background = `url(${e.target.result})`;
> +  reader.readAsDataURL(event.data);
> +});
> +
> +
> + let params = new URLSearchParams(location.search.slice(1));

Nit:

```js
let params = new URL(location.href).searchParams;
```

avoids the slice() stuff. :-)

::: testing/profiles/prefs_general.js:364
(Diff revision 7)
>  // Disable password capture, so that mochitests that include forms aren't
>  // influenced by the presence of the persistent doorhanger notification.
>  user_pref("signon.rememberSignons", false);
> +
> +// Make sure "Report Site Issue" button won't hit the network.
> +user_pref("extensions.webcompat-reporter.newIssueEndpoint", "http://example.com/browser/browser/extensions/webcompat-reporter/test/browser/webcompat.html");

So, with the change to make this a bit lazier, you can just set this pref in the individual tests using:

```js
yield SpecialPowers.pushPrefEnv({set: [["prefname", "prefvalue"]]});
```

to ensure that we don't hit this in other tests (because we shouldn't) and to keep the value here local to the tests (so we can also change it if at some point we want another endpoint for a specific test).
Attachment #8822250 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8822533 [details]
Bug 1324062. Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button.

https://reviewboard.mozilla.org/r/101410/#review102438

My deepest apologies for the clusterf... that is these tests. I should really find some time to fix them, after I've put out the fires in my request queue. :-\

r=me, though we'll need to come up with a way of fixing that remaining failure. I can hopefully find time tomorrow or Friday (assuming I can deal with reviews today) to help, if you're still stuck by then - please do ping me on IRC in that case. In the meantime I hope the suggestions below help with reproducing things.

::: browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js:47
(Diff revision 4)
> -  expectedPlaceholders = isInDevEdition() ? 1 : 3;
> +  expectedPlaceholders = 3;
> +  if (isInDevEdition()) {
> +    expectedPlaceholders = 1;
> +  } else if (isInNightly()) {
> +    expectedPlaceholders = 2;
> +  }

Making this slightly smarter while we're here:

```js
// We always start with a positive number of placeholders (checked above).
// We're removing 1 placeholder by replacing it with a 'real' button.
// So there should be 1 fewer than before, but if there would then be 0,
// there should now be 3
expectedPlaceHolders = (expectedPlaceholders - 1) || 3;
```

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js:428
(Diff revision 4)
>    let panel = document.getElementById(CustomizableUI.AREA_PANEL);
>    let numPlaceholders = 2;
>    for (let i = 0; i < numPlaceholders; i++) {
>      // This test relies on there being a specific number of widgets in the
> -    // panel. The addition of sync-button screwed this up, so we remove it
> -    // here. We should either fix the tests to not rely on the specific layout,
> +    // panel. The addition of sync-button and webcompat-reporter-button screwed
> +    // this up, so we remove it here. We should either fix the tests to not rely

Nit: remove /them/

::: browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js:16
(Diff revision 4)
>  
>  // When we drag an item onto a customizable area, and not over a specific target, we
>  // should assume that we're appending them to the area. If doing so, we should scan
>  // backwards over any hidden items and insert the item before those hidden items.
>  add_task(function*() {
> +  // This can fail if CustomizableUI.PROVIDER_API widgets are added as defaults,

Maybe the/a previous test has not have left things in the right state? That might also help with reproducing locally - if you're just running this test, that wouldn't show any issues. You can run the entire directory to see if that reproduces it, and then you can use the mochitest --start-at param to bisect the tests (though I somewhat suspect it's one of the immediately preceding tests, as they /should/ all be cleaning up after themselves anyway... if the sync button test, which comes immediately before this, removes its button and re-adds it, it might accidentally be relying on being the last button, which won't be true anymore...).

Though it's also weird that this isn't failing on OS X... :-\

::: browser/extensions/webcompat-reporter/test/browser/browser_button_state.js:12
(Diff revision 4)
>  
>  /* Test that the Report Site Issue button is enabled for http and https tabs,
>     on page load, or TabSelect, and disabled for everything else. */
>  add_task(function* test_button_state_disabled() {
>    let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, REPORTABLE_PAGE);
> -  yield panelShown(window);
> +  yield PanelUI.show();

Oh, oops, you already fixed that nit of mine in this patch. Sorry, I reviewed them one cset at a time...
Attachment #8822533 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks for the review comments so far!
Comment on attachment 8822533 [details]
Bug 1324062. Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button.

https://reviewboard.mozilla.org/r/101410/#review103266

::: browser/components/customizableui/test/browser_967000_button_sync.js
(Diff revision 5)
>  // The test expects the about:preferences#sync page to open in the current tab
>  function* openPrefsFromMenuPanel(expectedPanelId, entryPoint) {
>    info("Check Sync button functionality");
>    Services.prefs.setCharPref("identity.fxaccounts.remote.signup.uri", "http://example.com/");
>  
> -  // add the Sync button to the panel

I don't really understand why removing these fixes the issue on Linux (still can't reproduce locally, even running whole directory of CUI tests), but it appears to do the job. 

(besides, `sync-button` is a panel default (possibly wasn't when this test was written?): https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#203
Comment on attachment 8822533 [details]
Bug 1324062. Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button.

https://reviewboard.mozilla.org/r/101410/#review103270
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review102416

> This will enable/disable the button in every browser window, so it'll do the wrong thing if one window should have the button enabled and another shouldn't.
> 
> Use the
> 
> ```js
> CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).disabled
> ```
> 
> setter instead.

So, for whatever reason, when I do this the nodes don't actually update (they always appear disabled, but logging the getter shows it should have the correct state).

If I set/remove `disabled` myself though, it all works (which means I can get rid of the `focus` listener, that's how I was dealing with window-swapping and not ending up with a button in the wrong state):


```
if (TabListener.isReportableScheme(scheme)) {
  CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).node.removeAttribute("disabled");
} else {
  CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).node.setAttribute("disabled", true);
}
```

> I think onWindowOpened is a slightly better place to put this.
> 
> Also, right now, if you were to enable the add-on, disable it, and enable it again, we add the stylesheet twice. Can we avoid doing that?

Ah yeah, good point. Moved to onWindowOpened.

> yay es6:
> 
> ```js
> openWebCompatTab([browser, tabData]) {
> ```
> 
> Nit: please also add a comment above this function saying that arguments are in an array (rather than 'normal') because they are the result of a promise resolution.

Ah, nice!

> In theory, I can click the 'report' button and then close my browser window (on OS X, the process and all this code will stay alive). Because fetching the screenshot is async, it might race.
> 
> Right now, I think this will just throw. Making it do something better can be a followup - I seem to have forgotten how to deal with that (short of rolling your own openWindow call - but it seems there should be something better than that). :-(

Hmm, OK. I'll file a follow up bug here.

> I think you should also check the webProgress's isTopLevel property, to avoid this code triggering on asset files or frames that you might request from the web compat domain (I realize you probably use CDNs, and are unlikely to use frames, but the code right now would, AIUI, fire for any old file, and I presume we really care about the STATE_STOP of the main document).
> 
> You might also want to check for either the network or document flag. We'll need some way of ensuring that our sending the screenshot data doesn't race with the web compat web code having set up a listener for such data. When/how is the web compat web code doing that?

webcompat's bugform code sets up a message listener on DOMContentLoaded. I'll toss in the network flag as well, thx!

> We can do this check a lot earlier, can't we, and avoid adding the progress listener at all? That seems like it'd be simpler.

Great point.

> This looks like we'll always package en-US for whatever language we use... is this just a placeholder for when we add more localizations? If you haven't yet, can you check with flod how to deal with this?

Yeah, for now just a "send English to everyone" thing until there are more localizations. I'll shoot flod an email, and I guess file a follow up bug for adding more localizations (even though the site is awkwardly english-only for now).

> You can check with shorlander to get versions of this  SVG that are correctly coloured for each OS (version), plus an inverted one for dark lightweight themes / devedition.

Thanks! I've sent him an email.

> This is not a good idea in e10s, where ideally we shouldn't be touching the content document directly. Use something like this instead:
> 
> ```js
> yield BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "ReceivedScreenshot");
> ```
> 
> and dispatch a `new CustomEvent("ReceivedScreenshot")` from the content page when you receive the message you're waiting for.
> 
> This can probably replace waiting for the content to load (ie the browserLoaded bit in my previous suggestion), and if you fire the event after the read of the blob has succeeded, you can also remove the other waitForCondition below.

For whatever reason, I can't get this to actually work. It never yields. :/ Will ping on IRC tomorrow.

(and yeah, with this, there's no need to wait for browserLoaded, so that's a win).

> This is the default for new test files, so you don't strictly need this disclaimer ( https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ ).

Oh, that's cool. I had missed that.
(eh, ignore that last empty interdiff... just rebased and pushed for no reason other than to see how reviewboard would handle it).
(In reply to Mike Taylor [:miketaylr] from comment #55)
> Comment on attachment 8822250 [details]
> Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for
> NIGHTLY_BUILDs.
> 
> https://reviewboard.mozilla.org/r/101220/#review102416
> 
> > This will enable/disable the button in every browser window, so it'll do the wrong thing if one window should have the button enabled and another shouldn't.
> > 
> > Use the
> > 
> > ```js
> > CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).disabled
> > ```
> > 
> > setter instead.
> 
> So, for whatever reason, when I do this the nodes don't actually update
> (they always appear disabled, but logging the getter shows it should have
> the correct state).
> 
> If I set/remove `disabled` myself though, it all works (which means I can
> get rid of the `focus` listener, that's how I was dealing with
> window-swapping and not ending up with a button in the wrong state):
> 
> 
> ```
> if (TabListener.isReportableScheme(scheme)) {
>  
> CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).node.
> removeAttribute("disabled");
> } else {
>  
> CustomizableUI.getWidget(WIDGET_ID).forWindow(this.win).node.
> setAttribute("disabled", true);
> }
> ```

This is fine, though I'm curious why the getter/setter doesn't work. Do you see the same problem if you use the node.disabled getter/setter? If so this is likely a XBL binding 'quirk' - which we should work around in CUI. So if my hypothesis is right, please file a separate bug so I (or someone else) can fix the CustomizableUI code. Thanks for looking at this!
I'm trying to run Try Build just for a final UI-r but keep getting an error saying it's damaged: http://i.sevaan.com/1B1l2S2L1N0n

I've tried both the Nightly and NightlyDebug versions and the error is the same.

Just want to give it a final look over in action. If the build problem is me, could someone please provide some screenshots?
Here you go Sevaan: https://cloudup.com/cg4xQhYmYZb
Thank you Mike. +r from me.
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review103272

> This test can't get past this part, and I don't understand why. :|
> 
> Will ping on IRC tomorrow.

Turns out I needed `wantsUntrusted=true`, and also needed `{bubbles: true}` on my custom event (thanks mconley and gijs!)
shorlander, what's the best way to request help with themed SVG icons? Should I file a bug somewhere?
Flags: needinfo?(emanuela) → needinfo?(shorlander)
See Also: → 1329226
Blocks: 1329226
See Also: 1329226 → 
OK, Part 1 should be ready for review now, all test failures are fixed (and my own tests are running):

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

(Mac wouldn't build so I'll run bc tests on there for completeness)
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review103564

Just 2 notes below, though one's pretty important. With that, I think this looks good enough to land in nightly considering it's nightly-only for now. Up to you whether you want to sort out the icon before/after landing.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:53
(Diff revisions 7 - 10)
>  
>    onWindowOpened(win) {
> -    if (win.gBrowser) {
> +      // Attach stylesheet for the button icon.
> +      let data = `type="text/css" href="${LIGHTBULB_CSS}"`;
> +      let node = win.document.createProcessingInstruction("xml-stylesheet", data);
> +      win.document.insertBefore(node, win.document.documentElement);

I think we still don't remove this when disabled, so we'll duplicate it when re-enabled, right? Or do I misunderstand the code flow here? If not, the simplest way to allow removing this easily is probably to give the processing instruction an ID and use `win.document.getElementById(...).remove()` in the onWindowClosed code.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:120
(Diff revisions 7 - 10)
> -        if (flag & Ci.nsIWebProgressListener.STATE_STOP) {
> +          if (flag & Ci.nsIWebProgressListener.STATE_STOP |
> +            Ci.nsIWebProgressListener.IS_NETWORK && webProgress.isTopLevel) {

Nit: indentation here.

Also, this isn't doing what you think it's doing, viz:

```js
let foo = 1 & 1 | 2;
// foo = 3
```

I would put the expected flags in a temp var before the if statement. Note also that even with that change:

```js
if (5 & 3) {
  console.log("I get displayed because 5 & 3 == 1 which is truthy");
}
```

so we should check the result of the '&' operation matches the expected flags, not just that it's truthy.

Did you copy/paste this from somewhere, and if so, does that have the same bug? We should probably fix that...
Attachment #8822250 - Flags: review?(gijskruitbosch+bugs) → review+
Let's go ahead and land this (with comments addressed, in the next day or so (traveling atm)) and handle icons in Bug 1329704. shorlander said he would find help for this. Thanks!
Flags: needinfo?(shorlander)
Comment on attachment 8822250 [details]
Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs.

https://reviewboard.mozilla.org/r/101220/#review103564

> I think we still don't remove this when disabled, so we'll duplicate it when re-enabled, right? Or do I misunderstand the code flow here? If not, the simplest way to allow removing this easily is probably to give the processing instruction an ID and use `win.document.getElementById(...).remove()` in the onWindowClosed code.

Nah, you're right. Will add to `onWindowClosed`.

Seems like `doc.getElementById` and `xml-stylesheet`s don't get along (also maybe , so I'll store a reference to the stylesheet node on `WebCompatReporter` and `remove()` it that way.

edit: according to https://www.w3.org/TR/xml-stylesheet/ 

> Documents must not specify other pseudo-attributes on xml-stylesheet processing instructions.

And `id` is not on the list of legal pseudo-attributes. (I guess, XML isn't my strong suit.)

> Nit: indentation here.
> 
> Also, this isn't doing what you think it's doing, viz:
> 
> ```js
> let foo = 1 & 1 | 2;
> // foo = 3
> ```
> 
> I would put the expected flags in a temp var before the if statement. Note also that even with that change:
> 
> ```js
> if (5 & 3) {
>   console.log("I get displayed because 5 & 3 == 1 which is truthy");
> }
> ```
> 
> so we should check the result of the '&' operation matches the expected flags, not just that it's truthy.
> 
> Did you copy/paste this from somewhere, and if so, does that have the same bug? We should probably fix that...

Gonna change to:

```
let isStopped = flags & Ci.nsIWebProgressListener.STATE_STOP;
let isNetwork = flags & Ci.nsIWebProgressListener.STATE_IS_NETWORK;
if (isStopped && isNetwork && webProgress.isTopLevel) {
...
```
(In reply to Mike Taylor [:miketaylr] from comment #70)
> Comment on attachment 8822250 [details]
> Bug 1324062. Part 1 - Add a "Report Site Issue" button to the panel menu for
> NIGHTLY_BUILDs.
> 
> https://reviewboard.mozilla.org/r/101220/#review103564
> 
> > I think we still don't remove this when disabled, so we'll duplicate it when re-enabled, right? Or do I misunderstand the code flow here? If not, the simplest way to allow removing this easily is probably to give the processing instruction an ID and use `win.document.getElementById(...).remove()` in the onWindowClosed code.
> 
> Nah, you're right. Will add to `onWindowClosed`.
> 
> Seems like `doc.getElementById` and `xml-stylesheet`s don't get along (also
> maybe , so I'll store a reference to the stylesheet node on
> `WebCompatReporter` and `remove()` it that way.
> 
> edit: according to https://www.w3.org/TR/xml-stylesheet/ 
> 
> > Documents must not specify other pseudo-attributes on xml-stylesheet processing instructions.
> 
> And `id` is not on the list of legal pseudo-attributes. (I guess, XML isn't
> my strong suit.)

Oh oops. Sorry. Storing directly on WebCompatReporter has issues though, because you'll get 1 stylesheet per window, but WebCompatReporter is a singleton. So you'd need to keep track of them per window. Could potentially do:

let styleSheetRefs = new WeakMap(); // from windows to stylesheet els

...
onWindowOpened(win) {
  styleSheetRefs.set(win, stylesheet);
}
...
cleanupForWindow(win) {
  let stylesheet = styleSheetRefs.get(win);
  if (stylesheet) {
    stylesheet.remove();
  }
}

or something like that. You don't need to remove the stylesheet unless we're getting disabled while the windows are staying up (ie generally, if a window is genuinely closing you don't need to remove the stylesheet)
Ah yeah, you're totally right -- thanks for catching that!

I pushed that @ https://reviewboard.mozilla.org/r/101220/diff/11-12/

Try looks happy as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4e275fb466&selectedJob=67703389
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/028db619f166
Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1df935d51464
Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button. r=Gijs
this was backed out for an xperf failure- why this is pgo only, I am really confused.

TEST-UNEXPECTED-FAIL : xperf: File '{firefox}\browser\features\webcompat-reporter@mozilla.org.xpi' was accessed and we were not expecting it. DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 262144, DiskWriteBytes: 0


we have a whitelist (expected file access) for webcompat@mozilla.org.xpi, but not webcompat-recorder:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/xtalos/xperf_whitelist.json?q=path%3Axperf_whitelist.json&redirect_type=single#16

Ignoring the fact that this is pgo only, can we confirm that we want to put in another addon by default it makes sense to do this?  If so, please go ahead and add an entry to xperf_whitelist.json.
Rob, am I right in thinking that the 'webcompat' thing is about actually changing stuff (e.g. user agent) for specific sites for compatibility reasons, so we need a new entry? Or should we be removing the webcompat "stub" that got added in bug 1268197?
Flags: needinfo?(rhelmer)
Also https://treeherder.mozilla.org/logviewer.html#?job_id=68395913&repo=autoland about  toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js thinking you are misbehaving.
(In reply to :Gijs from comment #80)
> Rob, am I right in thinking that the 'webcompat' thing is about actually
> changing stuff (e.g. user agent) for specific sites for compatibility
> reasons, so we need a new entry? Or should we be removing the webcompat
> "stub" that got added in bug 1268197?

Mike, I notice this one is called webcompat-reporter - do you plan on replacing browser/extensions/webcompat/ also or should we remove it in favor of this one?
Flags: needinfo?(rhelmer) → needinfo?(miket)
(In reply to Joel Maher ( :jmaher) from comment #79)
> this was backed out for an xperf failure- why this is pgo only, I am really
> confused.

*womp womp* Sorry about that. :(
 
> Ignoring the fact that this is pgo only, can we confirm that we want to put
> in another addon by default it makes sense to do this?  If so, please go
> ahead and add an entry to xperf_whitelist.json.

Yeah, we want to do this. Will update.

(In reply to Phil Ringnalda (:philor) from comment #81)
> Also
> https://treeherder.mozilla.org/logviewer.html#?job_id=68395913&repo=autoland
> about 
> toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js
> thinking you are misbehaving.

Got it!

(In reply to Robert Helmer [:rhelmer] from comment #82)
> (In reply to :Gijs from comment #80)
> > Rob, am I right in thinking that the 'webcompat' thing is about actually
> > changing stuff (e.g. user agent) for specific sites for compatibility
> > reasons, so we need a new entry? Or should we be removing the webcompat
> > "stub" that got added in bug 1268197?
> 
> Mike, I notice this one is called webcompat-reporter - do you plan on
> replacing browser/extensions/webcompat/ also or should we remove it in favor
> of this one?

Nope, this is something different. The "webcompat" system add-on (stub currently, but non-stub landing in Bug 1308271, blocked on a test fix in Bug 1330076) is intended to be rapid fixes for top sites while we work with sites to fix things on their end (which is less rapid). This add-on adds a button for users to report broken sites.

The similar naming is kind of confusing, if someone feels strongly I could rename this one to report-site-issue.xpi (or something).
Flags: needinfo?(miket)
(See https://wiki.mozilla.org/Compatibility/Go_Faster_Addon/Initial_Planning#Requirements for more info on webcompat.xpi, for those interested)
Comment on attachment 8827628 [details]
Bug 1324062. Part 3 - Add webcompat-reporter.xpi to xperf whitelist.

https://reviewboard.mozilla.org/r/105244/#review106070

this looks good, thanks for adding it to the file.
Attachment #8827628 - Flags: review?(jmaher) → review+
Thanks Joel. 

I don't understand the failure in browser_AddonWatcher.js from Comment #81...

```
8570 14:04:20 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js | The add-on watcher has detected the misbehaving addon - "webcompat-reporter@mozilla.org" == "addonwatcher-test@mozilla.com" - JS frame :: chrome://mochitests/content/browser/toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js :: observer :: line 82
8578 14:04:20 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js | The add-on watcher has detected the misbehaving addon - "webcompat-reporter@mozilla.org" == "addonwatcher-test@mozilla.com" - JS frame :: chrome://mochitests/content/browser/toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js :: observer :: line 82

Any ideas of how this might fail David? It passes for me locally.
Flags: needinfo?(dteller)
Managed to get the browser_AddonWatcher.js failure on Linux-x86 debug (but not on 32 bit, which is where the original failure was):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce2f0e92c08b82cf731f23972d33a9c7ec2b13d&duplicate_jobs=visible&selectedJob=70017901

https://bug98304.bugzilla.mozilla.org/show_bug.cgi?id=1227138 had a similar failure for Loop.
Note that the test we're adding is also intermittently failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce2f0e92c08b82cf731f23972d33a9c7ec2b13d&selectedJob=70017921 ... We should probably figure

Unlike the SHIELD code, you're adding UI... I wonder if the CUI code initializing sooner / at a different time, or your adding stylesheets when the window is opened is what's at fault here. Don't really have a great idea for how to debug that, unfortunately... but you could try not touching CUI and seeing if the addon watcher's report is reliably gone? IIRC when we hit something similar with Pocket, we tried preloading the CSS or something... but it seems hard to believe that that single stylesheet should make such a big difference. :-\
(In reply to :Gijs from comment #95)
> Note that the test we're adding is also intermittently failing:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dce2f0e92c08b82cf731f23972d33a9c7ec2b13d&selectedJob=7
> 0017921 ... We should probably figure

Yeah, funky. That makes it look like it times out @ 
`yield BrowserTestUtils.waitForContentEvent(tab2.linkedBrowser, "ScreenshotReceived", false, null, true);`

I just re-wrote the test to be more promisey (because that's like adding magic, I'm told). Let's see if it fails in the next try run when I have... some kind of plan on how to fix the browser_AddonWatcher.js test failure.

> Unlike the SHIELD code, you're adding UI... I wonder if the CUI code
> initializing sooner / at a different time, or your adding stylesheets when
> the window is opened is what's at fault here. Don't really have a great idea
> for how to debug that, unfortunately... but you could try not touching CUI
> and seeing if the addon watcher's report is reliably gone? IIRC when we hit
> something similar with Pocket, we tried preloading the CSS or something...
> but it seems hard to believe that that single stylesheet should make such a
> big difference. :-\

I just pushed a try build where I commented out adding the stylesheet in CUI.onWindowOpened and didn't get a "misbehaving" add-on failure for linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13bf988a72f8. Yay?
The fix that landed for Loop in https://bug98304.bugzilla.mozilla.org/show_bug.cgi?id=1227138 was to just make sure the test is only looking for its own addon. But that was removed in: https://hg.mozilla.org/mozilla-central/diff/30227d71bff3/toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js...

I guess I can try what Pocket does here: https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/bootstrap.js#317-320 and see if that makes a difference.
(In reply to Mike Taylor [:miketaylr] from comment #89)
> Thanks Joel. 
> 
> I don't understand the failure in browser_AddonWatcher.js from Comment #81...
> 
> ```
> 8570 14:04:20 INFO - TEST-UNEXPECTED-FAIL |
> toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js |
> The add-on watcher has detected the misbehaving addon -
> "webcompat-reporter@mozilla.org" == "addonwatcher-test@mozilla.com" - JS
> frame ::
> chrome://mochitests/content/browser/toolkit/components/perfmonitoring/tests/
> browser/browser_AddonWatcher.js :: observer :: line 82
> 8578 14:04:20 INFO - TEST-UNEXPECTED-FAIL |
> toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js |
> The add-on watcher has detected the misbehaving addon -
> "webcompat-reporter@mozilla.org" == "addonwatcher-test@mozilla.com" - JS
> frame ::
> chrome://mochitests/content/browser/toolkit/components/perfmonitoring/tests/
> browser/browser_AddonWatcher.js :: observer :: line 82
> 
> Any ideas of how this might fail David? It passes for me locally.

Well, apparently there is an addon ("webcompat-reporter@mozilla.org") that's detected as misbehaving while running on the server. Most likely, it was installed by another test.

The simplest way would be to patch "browser_AddonWatcher.js" to ignore this specific addon. Unfortunately, I won't have time to do it before mid-February. Mike, by any chance, do you have time to do this?
Flags: needinfo?(dteller) → needinfo?(miket)
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #102)
> (In reply to Mike Taylor [:miketaylr] from comment #89)
> > Thanks Joel. 
> > 
> > I don't understand the failure in browser_AddonWatcher.js from Comment #81...
> > 
> > ```
> > 8570 14:04:20 INFO - TEST-UNEXPECTED-FAIL |
> > toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js |
> > The add-on watcher has detected the misbehaving addon -
> > "webcompat-reporter@mozilla.org" == "addonwatcher-test@mozilla.com" - JS
> > frame ::
> > chrome://mochitests/content/browser/toolkit/components/perfmonitoring/tests/
> > browser/browser_AddonWatcher.js :: observer :: line 82
> > 8578 14:04:20 INFO - TEST-UNEXPECTED-FAIL |
> > toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js |
> > The add-on watcher has detected the misbehaving addon -
> > "webcompat-reporter@mozilla.org" == "addonwatcher-test@mozilla.com" - JS
> > frame ::
> > chrome://mochitests/content/browser/toolkit/components/perfmonitoring/tests/
> > browser/browser_AddonWatcher.js :: observer :: line 82
> > 
> > Any ideas of how this might fail David? It passes for me locally.
> 
> Well, apparently there is an addon ("webcompat-reporter@mozilla.org") that's
> detected as misbehaving while running on the server. Most likely, it was
> installed by another test.
> 
> The simplest way would be to patch "browser_AddonWatcher.js" to ignore this
> specific addon. Unfortunately, I won't have time to do it before
> mid-February. Mike, by any chance, do you have time to do this?

No, we're adding that add-on in this bug. It's a system add-on. And again, there's not really a lot of actionable information to work out why this add-on is being detected as "slow" on infra, given that apparently the issue doesn't reproduce locally. :-\
Ah, my bad. My best guess is that it's caused by bug 1305031.
(In reply to Mike Taylor [:miketaylr] from comment #97)
> The fix that landed for Loop in
> https://bug98304.bugzilla.mozilla.org/show_bug.cgi?id=1227138 was to just
> make sure the test is only looking for its own addon. But that was removed
> in:
> https://hg.mozilla.org/mozilla-central/diff/30227d71bff3/toolkit/components/
> perfmonitoring/tests/browser/browser_AddonWatcher.js...
> 
> I guess I can try what Pocket does here:
> https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/
> bootstrap.js#317-320 and see if that makes a difference.

The initial runs were green. Retriggered to make sure it's not intermittent. :-\
The retriggers are green too, so I think this is good to reland now. Thanks for your persistence, Mike!
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #102)
> The simplest way would be to patch "browser_AddonWatcher.js" to ignore this
> specific addon. Unfortunately, I won't have time to do it before
> mid-February. Mike, by any chance, do you have time to do this?

Thanks for taking a look David! I made tweaks to how we're adding a stylesheet to new windows (thx to Gijs and Pocket code...), and it looks like it's a non-issue for now. 

(In reply to :Gijs from comment #106)
> The retriggers are green too, so I think this is good to reland now. Thanks
> for your persistence, Mike!

Bombs away... and thanks everyone for help thus far.
Flags: needinfo?(miket)
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1658b20cb69c
Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/054cfcf60c75
Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d01f82be01f1
Part 3 - Add webcompat-reporter.xpi to xperf whitelist. r=jmaher
Well, so the issue detected above hasn't been resolved. I guess you should just whitelist the add-on.
Son of a gun. OK, let's whitelist.
Flags: needinfo?(miket)
Comment on attachment 8828944 [details]
Bug 1324062. Part 4 - Add webcompat-reporter.xpi to addon-watch whitelist.

https://reviewboard.mozilla.org/r/106164/#review107156

Let's try that.
Attachment #8828944 - Flags: review?(dteller) → review+
Thanks for the quick review Yoric. One last try before re-landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7da113236677
Try looks good (famous last words...). Will fix a small commit message inconsistency and then push to autoland again.
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf749e7246d7
Part 1 - Add a "Report Site Issue" button to the panel menu for NIGHTLY_BUILDs. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/8debe3af8515
Part 2 - Fix CustomizableUI test failures due to NIGHTLY-only Report Site Issue button. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/0a0afd09f8b1
Part 3 - Add webcompat-reporter.xpi to xperf whitelist. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/6ee916b9fa07
Part 4 - Add webcompat-reporter.xpi to addon-watch whitelist. r=Yoric
Depends on: 1332856
Depends on: 1333305
a perf win was seen when this landed:
== Change summary for alert #4916 (as of January 20 2017 23:47 UTC) ==

Improvements:

  2%  sessionrestore_no_auto_restore windows8-64 opt     868.92 -> 850.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4916
Depends on: 1362102
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: