Closed Bug 1373650 Opened 4 years ago Closed 4 years ago

Add Report Site Issue option to Photon Page Action menu for Nightly + DevEdition

Categories

(Web Compatibility :: Tooling & Investigations, enhancement, P1)

Product:
Web Compatibility
Component:
Tooling & Investigations
Type:
enhancement

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: miketaylr, Assigned: miketaylr)

References

(Depends on 1 open bug)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(6 files, 1 obsolete file)

Bug 1373650. Move Report Site Issue button from CUI to PageActions API.
59 bytes, text/x-review-board-request
adw
: review+
Details
Bug 1373650. Update webcompat-reporter tests.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1373650. Remove TabListener.jsm from browser_startup.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1373650. Disable Report Site Issue page action before running page action tests.
59 bytes, text/x-review-board-request
adw
: review+
Details
report-issues-16.svg
682 bytes, image/svg+xml
Details 
As of June 16 this is preffed on in Nightly. Need to see what it would take to add our button there.
(Note: it's still in the CUI panel if you click customize.)
Assignee: nobody → miket
Hey Stephen, can I get some guidance from UX? It's super valuable for us to be able to have this button in the UI for Nightly (and Dev Edition). 

Would it be OK to stick it in the new photon menu in between Web Developer and Help? Or should this go in the page actions menu? Again, this won't ship beyond DevEdition or Nightly. Thanks!
Flags: needinfo?(shorlander)
Blocks: 1306114
Well if you go by the intent of the Photon reorganization things that relate to the page should be placed in the Page Action Menu (•••).

Append it to the end of the Page Action Menu, under a horizontal rule, (for Nightly Only).
Flags: needinfo?(shorlander)
Thanks Bryan, I'll go for that. Note, it will be *Nightly and DevEdition* only (and not ride to Beta or Release).
I think bug 1374477 will help with implementing this.
Blocks: 1374477
Whiteboard: [reserve-photon-structure]
Oh, excellent. Thanks for the pointer!
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: Report Site Issue button missing from new Photon hamburger menu → Add Report Site Issue option to Photon Page Action menu for Nightly + DevEdition
(In reply to bbell from comment #3)
> Append it to the end of the Page Action Menu, under a horizontal rule, (for
> Nightly Only).

These patches don't have it under a new horizontal rule, but I can do that if someone feels strongly.

(reasoning being I'm using __insertBeforeActionID, because I consider this to be a semi-built-in action (for nightly/devedition))

http://searchfox.org/mozilla-central/source/browser/modules/PageActions.jsm#146-149
No longer blocks: 1374477
Depends on: 1374477
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170040

::: browser/extensions/webcompat-reporter/skin/lightbulb.svg:7
(Diff revision 1)
> +<!-- 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"
> +     width="32" height="32" class="fieldtext">
> +#include ../../../../browser/themes/shared/icon-colors.inc.svg

nit: Remove this line and use fill="context-fill" instead of class="fieldtext".
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170108

Stealing this one from Gijs...  This isn't (shouldn't be) necessary.  onPlacedInPanel is defined on Action.prototype.  It should never be null/undefined.  Is that what you're seeing?  If so, there's some other problem we need to figure out.
Attachment #8893569 - Flags: review-
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170110

I'll steal this one too while I'm here...

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:30
(Diff revision 1)
>        "extensions.webcompat-reporter.newIssueEndpoint");
>    },
>  
>    init() {
> -    let styleSheetService = Cc["@mozilla.org/content/style-sheet-service;1"]
> -      .getService(Ci.nsIStyleSheetService);
> +    PageActions.addAction({
> +      __insertBeforeActionID: true,

This isn't the right way to use this property, and you don't need it anyway.  Leaving it out means that this action will go under the separator, which is what Bryan says we want in comment 3.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:33
(Diff revision 1)
>    init() {
> -    let styleSheetService = Cc["@mozilla.org/content/style-sheet-service;1"]
> -      .getService(Ci.nsIStyleSheetService);
> -    this._sheetType = styleSheetService.AUTHOR_SHEET;
> -    this._cachedSheet = styleSheetService.preloadSheet(wcStyleURI,
> -                                                       this._sheetType);
> +    PageActions.addAction({
> +      __insertBeforeActionID: true,
> +      id: "webcompat-reporter-button",
> +      title: wcStrings.GetStringFromName("wc-reporter.label"),
> +      iconURL: "chrome://webcompat-reporter/skin/lightbulb.svg",

Does the webcompat extension have CSS files for styling purposes?  If so, please use them rather than this property, just because this property is really only intended to be used for WebExtensions, where CSS isn't really an option.  If you don't already have CSS files though, then this is OK.

The selectors are #pageAction-panel-${actionID} for the button in the panel and #pageAction-urlbar-${actionID} for the button in the urlbar.  You can see Pocket's example here: https://hg.mozilla.org/mozilla-central/annotate/tip/browser/extensions/pocket/skin/shared/pocket.css#l252

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:35
(Diff revision 1)
> -      .getService(Ci.nsIStyleSheetService);
> -    this._sheetType = styleSheetService.AUTHOR_SHEET;
> -    this._cachedSheet = styleSheetService.preloadSheet(wcStyleURI,
> -                                                       this._sheetType);
> -
> -    XPCOMUtils.defineLazyModuleGetter(this, "TabListener", TABLISTENER_JSM);
> +      __insertBeforeActionID: true,
> +      id: "webcompat-reporter-button",
> +      title: wcStrings.GetStringFromName("wc-reporter.label"),
> +      iconURL: "chrome://webcompat-reporter/skin/lightbulb.svg",
> +      onCommand: (e) => this.reportIssue(e.target.ownerGlobal),
> +      onShowingInPanel: this.onShowingInPanel

You should probably do this.onShowingInPanel.bind(this) or make this an arrow function.  Your onShowingInPanel doesn't use `this` currently, but it might in the future.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:42
(Diff revision 1)
>    },
>  
>    uninit() {
> -    CustomizableUI.destroyWidget(WIDGET_ID);
> -
> -    for (let win of CustomizableUI.windows) {
> +    let action = PageActions.actionForID("webcompat-reporter-button");
> +    if (action) {
> +      PageActions.onActionRemoved(action);

Replace this with: action.remove()
Attachment #8893570 - Flags: review-
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170110

> This isn't the right way to use this property, and you don't need it anyway.  Leaving it out means that this action will go under the separator, which is what Bryan says we want in comment 3.

okey doke.

> Does the webcompat extension have CSS files for styling purposes?  If so, please use them rather than this property, just because this property is really only intended to be used for WebExtensions, where CSS isn't really an option.  If you don't already have CSS files though, then this is OK.
> 
> The selectors are #pageAction-panel-${actionID} for the button in the panel and #pageAction-urlbar-${actionID} for the button in the urlbar.  You can see Pocket's example here: https://hg.mozilla.org/mozilla-central/annotate/tip/browser/extensions/pocket/skin/shared/pocket.css#l252

Aw man, I totally missed this comment (I was just reading the code...):

http://searchfox.org/mozilla-central/source/browser/modules/PageActions.jsm#381

Will switch back to CSS.
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170108

Yeah... admittedly I didn't dig in and just wanted to get it working. Here's the error:

TypeError: action.onPlacedInPanel is not a function[Learn More]  browser-pageActions.js:102:7
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170110

> Replace this with: action.remove()

So something is funky with the Action prototype methods (and props) not actually being on the action instance.

TypeError: action.remove is not a function[Learn More]  WebCompatReporter.jsm:40:5
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170110

> So something is funky with the Action prototype methods (and props) not actually being on the action instance.
> 
> TypeError: action.remove is not a function[Learn More]  WebCompatReporter.jsm:40:5

... I just realized I passed in an object literal to PageActions.addAction, rather than a new PageActions.Action. New patches coming up.
Attachment #8893569 - Flags: review?(gijskruitbosch+bugs)
Attachment #8893570 - Flags: review?(gijskruitbosch+bugs)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #19)
> ... I just realized I passed in an object literal to PageActions.addAction,
> rather than a new PageActions.Action. New patches coming up.

Ah, sorry I didn't notice that.
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170040

> nit: Remove this line and use fill="context-fill" instead of class="fieldtext".

cool, thanks.
Well, git-cinnabar does not like me right now: https://github.com/glandium/git-cinnabar/issues/136. Will sort that out in the morning. Thanks for the help and reviews so far Drew and Tim!
Attachment #8893573 - Attachment is obsolete: true
Attachment #8893573 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170332

Driveby comments from me because I wanted to understand the rest of the patches so I looked at this one.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:69
(Diff revision 2)
> +  },
>  
> -    if (Cu.isModuleLoaded(TABLISTENER_JSM)) {
> -      Cu.unload(TABLISTENER_JSM);
> +  onShowingInPanel(buttonNode) {
> +    let browser = buttonNode.ownerGlobal.gBrowser;
> +    let scheme = browser.currentURI.scheme;
> +    if (["http", "https"].some((prefix) => scheme.startsWith(prefix))) {

This would also enable it for "http-custom" or whatever, so please don't do a prefix check but just: 

```js
["http", "https"].includes(scheme)
```

::: browser/extensions/webcompat-reporter/skin/lightbulb.css:5
(Diff revision 2)
> +/* 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/. */
> +#pageAction-panel-webcompat-reporter-button {
> +  list-style-image: url("chrome://webcompat-reporter/skin/lightbulb.svg");

I defer to Drew, but my understanding is that if you pass an iconURL you shouldn't need this, which will reduce complexity here some more because we won't need to insert the stylesheet (yay!).
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170332

> This would also enable it for "http-custom" or whatever, so please don't do a prefix check but just: 
> 
> ```js
> ["http", "https"].includes(scheme)
> ```

Good point, will change.

> I defer to Drew, but my understanding is that if you pass an iconURL you shouldn't need this, which will reduce complexity here some more because we won't need to insert the stylesheet (yay!).

(Yeah, my first version did that but Drew requested I move back to CSS, since the iconURL is intended for WebExtensions that can't insert stylesheets.)
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170338

::: browser/extensions/webcompat-reporter/test/browser/browser_button_state.js:10
(Diff revision 2)
>     on page load, or TabSelect, and disabled for everything else. */
>  add_task(async function test_button_state_disabled() {
> -  CustomizableUI.addWidgetToArea("webcompat-reporter-button", "nav-bar");
>    let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, REPORTABLE_PAGE);
> +  openPageActions();
>    is(isButtonDisabled(), false, "Check that button is enabled for reportable schemes on tab load");

`openPageActions` shows the popup, so I think you need to call hidePopup() after each of these.

::: browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:15
(Diff revision 2)
>  
> -  let webcompatButton = document.getElementById("webcompat-reporter-button");
> +  let webcompatButton = document.getElementById(WC_PAGE_ACTION_ID);
>    ok(webcompatButton, "Report Site Issue button exists.");
>  
>    let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser);
> +  openPageActions();

Same here.
Attachment #8893570 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8893571 [details]
Bug 1373650. Remove TabListener.jsm from browser_startup.js.

https://reviewboard.mozilla.org/r/164644/#review170344
Attachment #8893571 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8893572 [details]
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.

https://reviewboard.mozilla.org/r/164646/#review170346

Can you rebase on top of bug 1354117? I think some of these tests have just gone away with that change.
Attachment #8893572 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8893572 [details]
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.

https://reviewboard.mozilla.org/r/164646/#review170346

Roger that!
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170354

::: browser/extensions/webcompat-reporter/skin/lightbulb.svg:6
(Diff revision 3)
> +<?xml version="1.0"?>
> +<!-- 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"
> +     width="32" height="32" class="context-fill">

Should be fill="context-fill"
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170354

> Should be fill="context-fill"

*womp womp*
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170364

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:43
(Diff revision 4)
>  
> -    XPCOMUtils.defineLazyModuleGetter(this, "TabListener", TABLISTENER_JSM);
> -
> -    CustomizableUI.createWidget({
> -      id: WIDGET_ID,
> -      label: wcStrings.GetStringFromName("wc-reporter.label"),
> +    PageActions.addAction(new PageActions.Action({
> +      id: "webcompat-reporter-button",
> +      title: wcStrings.GetStringFromName("wc-reporter.label"),
> +      iconURL: "chrome://webcompat-reporter/skin/lightbulb.svg",
> +      onCommand: (e) => this.reportIssue(e.target.ownerGlobal),

Should I be calling `BrowserPageActions.panelNode.hidePopup();` here as well?
(ni? for question in Comment 43)
Flags: needinfo?(adw)
Comment on attachment 8893572 [details]
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.

https://reviewboard.mozilla.org/r/164646/#review170368

::: browser/components/customizableui/CustomizableUI.jsm
(Diff revision 4)
> -    if (AppConstants.MOZ_DEV_EDITION || AppConstants.NIGHTLY_BUILD) {
> -      if (Services.prefs.getBoolPref("extensions.webcompat-reporter.enabled")) {
> -        panelPlacements.push("webcompat-reporter-button");
> -      }
> -    }

This is gone now, but there's a corresponding block in the migration code.

::: browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
(Diff revision 4)
>    let panel = document.getElementById(CustomizableUI.AREA_PANEL);
>    // The value of expectedPlaceholders depends on the default palette layout.
>    // Bug 1229236 is for these tests to be smarter so the test doesn't need to
>    // change when the default placements change.
> -  let expectedPlaceholders = 1;
> +  let expectedPlaceholders = 1 + (isInDevEdition() ? 1 : 0);
> -  if (isInNightly()) {

This is also gone.

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
(Diff revision 4)
>                               "find-button",
>                               "preferences-button",
>                               "add-ons-button",
>                               "developer-button",
>                               "sync-button",
> -                             "webcompat-reporter-button"

I don't think this rebase worked, because this file is definitely gone on autoland tip.

::: browser/components/customizableui/test/head.js
(Diff revision 4)
>  
>  function isInDevEdition() {
>    return AppConstants.MOZ_DEV_EDITION;
>  }
>  
> -function isInNightly() {

Yay, cleanup!
Attachment #8893572 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #45)
> I don't think this rebase worked, because this file is definitely gone on
> autoland tip.

Oh yeah, I haven't rebased just yet -- sorry that it re-pinged you for review when I pushed a fixup to a previous commit. :/

(Just noticed Bug 1354117 landed on m-c, so I'll work on the rebase now.)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #43)
> ::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:43
> (Diff revision 4)
> > +      onCommand: (e) => this.reportIssue(e.target.ownerGlobal),
> 
> Should I be calling `BrowserPageActions.panelNode.hidePopup();` here as well?

I think maybe not.... but a sanity check would be appreciated! 

(And means I probably don't need to update the tests to manually close the panel):

PageActions.addAction -> 
BrowserPlaceAction.placeAction -> 
BrowserPlaceAction.placeActionInPanel -> 
BrowserPlaceAction._makePanelButtonNodeForAction,
which sets up a "command" event handler that calls BrowserPlaceAction.panelNode.hidePopup() before calling the actions onCommand method.
(*BrowserPageAction, that is...)
Comment on attachment 8893569 [details]
Bug 1373650. Move Report Site Issue button from CUI to PageActions API.

https://reviewboard.mozilla.org/r/164640/#review170450

LGTM except it looks like you have both an iconURL and CSS styling now.  You should only need one or the other.

And about that -- sorry, I didn't realize you were adding a stylesheet dynamically only for this icon.  I didn't mean for you to jump through hoops to avoid using iconURL.  So please choose whichever method is easier for you at this point: iconURL or the stylesheet.
Attachment #8893569 - Flags: review?(adw) → review+
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #47)
> > Should I be calling `BrowserPageActions.panelNode.hidePopup();` here as well?
> 
> I think maybe not.... but a sanity check would be appreciated! 
> 
> (And means I probably don't need to update the tests to manually close the
> panel):
> 
> PageActions.addAction -> 
> BrowserPlaceAction.placeAction -> 
> BrowserPlaceAction.placeActionInPanel -> 
> BrowserPlaceAction._makePanelButtonNodeForAction,
> which sets up a "command" event handler that calls
> BrowserPlaceAction.panelNode.hidePopup() before calling the actions
> onCommand method.

You're correct, you don't need to hide the panel yourself, it should happen automatically in the code path you describe.
Flags: needinfo?(adw)
Comment on attachment 8893572 [details]
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.

https://reviewboard.mozilla.org/r/164646/#review170522

::: browser/components/customizableui/CustomizableUI.jsm
(Diff revision 5)
> -      if (AppConstants.MOZ_DEV_EDITION || AppConstants.NIGHTLY_BUILD) {
> -        if (Services.prefs.getBoolPref("extensions.webcompat-reporter.enabled")) {
> -          defaultPlacements.push("webcompat-reporter-button");
> -        }

So, I know I said we should update this, but thinking about this, this will mean that if you use nightly or devedition, and you hit this migration code, we would try to re-add the webcompat-reporter to the overflow panel.

More generally, I guess we should tidy up after ourselves...

Can you increment `kVersion` and then add a separate block that looks for `webcompat-reporter` in the saved state and removes it if found?
Attachment #8893572 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Drew Willcoxon :adw from comment #53)
> And about that -- sorry, I didn't realize you were adding a stylesheet
> dynamically only for this icon.  I didn't mean for you to jump through hoops
> to avoid using iconURL.  So please choose whichever method is easier for you
> at this point: iconURL or the stylesheet.

No worries. Locally I'm using iconURL again -- seems nicer to remove avoid complexity involved in adding/removing stylesheets.
Comment on attachment 8893570 [details]
Bug 1373650. Update webcompat-reporter tests.

https://reviewboard.mozilla.org/r/164642/#review170338

> `openPageActions` shows the popup, so I think you need to call hidePopup() after each of these.

(Comments #47 and #54 explain why we don't need to do this -- hidePopup() gets called eventually when the command handler is fired.)
Comment on attachment 8893572 [details]
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.

(not sure if reviewboard sends updates for an r+'d patch...)

Gijs, something like this?
Attachment #8893572 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8893572 [details]
Bug 1373650. Unteach CustomizableUI about the Report Site Issue button.

https://reviewboard.mozilla.org/r/164646/#review171632

Thanks for making me check - this is close, but not quite right yet. With the below, r=me (and apologies for not being more explicit in my original feedback!)

::: browser/components/customizableui/CustomizableUI.jsm:393
(Diff revision 6)
> +        gSavedState.placements["PanelUI-contents"]) {
> +      let savedPanelPlacements = gSavedState.placements["PanelUI-contents"];
> +      if (savedPanelPlacements.includes("webcompat-reporter-button")) {
> +        savedPanelPlacements.splice(savedPanelPlacements.indexOf("webcompat-reporter-button"), 1);
> +      }

This only looks for it in the panel, but we should remove it everywhere, so I think something like this:

```js
if (currentVersion < 9 && gSavedState.placements) {
  for (let placements of Object.values(gSavedState.placements)) {
    if (placements.includes("webcompat-reporter-button")) {
      placements.splice(placements.indexOf("webcompat-reporter-button"), 1);
      break;
    }
  }
}
```
Attachment #8893572 - Flags: feedback?(gijskruitbosch+bugs)
Thanks Gijs.

Just need to update PageAction tests, then should be good to go.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=154e72675cc7&selectedJob=121746628
Attachment #8895492 - Flags: review?(adw)
I am being a terrible person and bitrotting the CustomizableUI migration thing in bug 1383009 - when you rebase, please increment kVersion at the top of the file as well as in the migration code. :-)

Sorry! I had thought this would land first...
Comment on attachment 8895492 [details]
Bug 1373650. Disable Report Site Issue page action before running page action tests.

https://reviewboard.mozilla.org/r/166688/#review172464

What's supposed to happen when extensions.webcompat-reporter.enabled is set to false?  What does "disable" mean?  I looked through the other patches in this bug and didn't see anything, so AFAICT this doesn't actually do anything.
Comment on attachment 8895492 [details]
Bug 1373650. Disable Report Site Issue page action before running page action tests.

https://reviewboard.mozilla.org/r/166688/#review172472

Sorry!  I see.  WebCompatReporter.uninit() gets called, and the action is removed.
Attachment #8895492 - Flags: review?(adw) → review+
(In reply to :Gijs from comment #68)
> I am being a terrible person and bitrotting the CustomizableUI migration
> thing in bug 1383009 - when you rebase, please increment kVersion at the top
> of the file as well as in the migration code. :-)
> 
> Sorry! I had thought this would land first...

Ha, no worries. My bad for being away from code for a few days while traveling to TO office (and then not running PageAction tests until way too late).

Thanks again for help Drew and Gijs.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 87a4e81f4f93 -d fd97f835597a: rebasing 413030:87a4e81f4f93 "Bug 1373650. Move Report Site Issue button from CUI to PageActions API. r=adw"
rebasing 413031:1788a2e499ed "Bug 1373650. Update webcompat-reporter tests. r=Gijs"
rebasing 413032:bca85cd9a394 "Bug 1373650. Remove TabListener.jsm from browser_startup.js. r=Gijs"
rebasing 413033:49673423e00e "Bug 1373650. Unteach CustomizableUI about the Report Site Issue button. r=Gijs"
merging browser/components/customizableui/CustomizableUI.jsm
warning: conflicts while merging browser/components/customizableui/CustomizableUI.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
wat.
OK, I rebased against inbound which doesn't have the patches from 1383009 (yet...). I need to rebase against central. Sorry Autoland bot, it's not your fault.
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0b6db321f82
Move Report Site Issue button from CUI to PageActions API. r=adw
https://hg.mozilla.org/integration/autoland/rev/0f9b774eac91
Update webcompat-reporter tests. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/4976f279c96f
Remove TabListener.jsm from browser_startup.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/792f4bf3fed7
Unteach CustomizableUI about the Report Site Issue button. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/449c8d0f859e
Disable Report Site Issue page action before running page action tests. r=adw
this shows a performance improvement:
== Change summary for alert #8770 (as of August 11 2017 18:29 UTC) ==

Improvements:

  6%  sessionrestore_no_auto_restore windows7-32 pgo e10s     605.42 -> 568.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8770
Attached image report-issues-16.svg — 
Please replace "Report Site Issues" icon with attached.
Flags: needinfo?(miket)
Filed Bug 1390519 as a follow up (since this bug is FIXED...)
Flags: needinfo?(miket)
Can the person that added a release note for this please change the note so that it doesn't read like a request?

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #90)
> Can the person that added a release note for this please change the note so
> that it doesn't read like a request?
> 
> Sebastian

What specifically are you referring to? I don't think the 57 release notes have been written yet...
Flags: needinfo?(sebastianzartner)
(In reply to :Gijs from comment #91)
> (In reply to Sebastian Zartner [:sebo] from comment #90)
> > Can the person that added a release note for this please change the note so
> > that it doesn't read like a request?
> > 
> > Sebastian
> 
> What specifically are you referring to? I don't think the 57 release notes
> have been written yet...

They are already (partly) written, see https://www.mozilla.org/en-US/firefox/57.0a1/releasenotes/.
And I am referring to the sentence "Add Report Site Issue option to Page Action menu for Firefox Nightly and Firefox Dev Edition", which should rather be written as "Added an option to the Page Action menu to report a website issue".

Sebastian
Flags: needinfo?(sebastianzartner)
(In reply to Sebastian Zartner [:sebo] from comment #92)
> (In reply to :Gijs from comment #91)
> > (In reply to Sebastian Zartner [:sebo] from comment #90)
> > > Can the person that added a release note for this please change the note so
> > > that it doesn't read like a request?

> They are already (partly) written, see
> https://www.mozilla.org/en-US/firefox/57.0a1/releasenotes/.
> And I am referring to the sentence "Add Report Site Issue option to Page
> Action menu for Firefox Nightly and Firefox Dev Edition", which should
> rather be written as "Added an option to the Page Action menu to report a
> website issue".

Ritu or Julien, can one of you address this? Thank you!
Flags: needinfo?(rkothari)
Flags: needinfo?(jcristau)
(In reply to :Gijs from comment #93)

> Ritu or Julien, can one of you address this? Thank you!

Done
Flags: needinfo?(rkothari)
Flags: needinfo?(jcristau)
Perfect! Thank you for the quick fix, Pascal!

Sebastian
Depends on: 1400263
Verified on Windows 10 64bit, Mac OS X 10.11, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) as of this date.
Did you intend to mark this as verified? :-)
Flags: needinfo?(gwimberly)
Verifying per previous discussion on call with Gijs.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gwimberly)
Depends on: 1417273
You need to log in before you can comment on or make changes to this bug.