Closed Bug 1209344 Opened 4 years ago Closed 4 years ago

remove "debug" buttons on about:addons and refer to about:debugging

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox44 --- affected
firefox46 --- fixed
firefox51 --- verified

People

(Reporter: mossop, Assigned: rhelmer)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

Developers want to be able to load an add-on temporarily for testing so we need a UI to let them provide a directory to load an add-on from and run.
Depends on: 1185460
We were thinking that there would be another option in the add-ons menu gear drop down (see below) that said, something like "Temporarily install unsigned Add-on from File...". Although it might work for a directory or .xpi, but I don't know if that needs differentiating.

https://www.dropbox.com/s/i2hxx91buzsliov/Screenshot%202015-10-26%2010.16.03.png?dl=0

I'm not sure about that message though Markus, any advice?
Flags: needinfo?(mjaritz)
In my opinion, we can do either one of these four things. They all rely on the fact that our devs are able to toggle options that may be obscure to every day user, or use different Firefox channels.

1. Add another link to the drop down menu: “Temporarily install unsigned Add-on from File…”. This is the solution proposed above.
  * This seems to be the most obvious and accessible place to put this link
  * But it may be too accessible to every day user, leading to misuse?

2. Modify the existing “Install Add-on from File…” functionality, so it can install both signed and unsigned add-ons
  * However, this ability only exists in Dev Edition or Beta

3. On the file upload dialogue, have a checkbox that says “Only install signed add-ons” that is checked by default. If unchecked, then unsigned add-ons can be installed

4. Put the option to install unsigned add-ons behind a pref. If this pref is on, then user can select any unsigned add-on using this dialogue, and it will be installed.
  * This may be the most obscure way of triggering this feature, but our devs should be familiar with about:preferences

Personally, I’m in favour of approach #2 or #4. What do you think?

(Clearing Markus’ needinfo, but feel free to put it back on if the answer doesn’t satisfy).
Flags: needinfo?(mjaritz)
(In reply to Bram Pitoyo [:bram] from comment #2)
> 1. Add another link to the drop down menu: “Temporarily install unsigned
> Add-on from File…”. This is the solution proposed above.
>   * This seems to be the most obvious and accessible place to put this link
>   * But it may be too accessible to every day user, leading to misuse?

I like that way. It seams fairly well hidden so that normal users might not use it. (Close to none of the users we tested so far has ever been to the add-ons manager in their respective browser. They install and use add-ons, but no more.)

Do we need a permanent indicator while a temporary add-on is active? 
(Is it possible to automatically install a temporary add-on?)

(In reply to Andy McKay [:andym] from comment #1)
> We were thinking that there would be another option in the add-ons menu gear
> drop down (see below) that said, something like "Temporarily install
> unsigned Add-on from File...". Although it might work for a directory or
> .xpi, but I don't know if that needs differentiating.

Regarding the message I am not sure if we need the "unsigned" in it, 
but somehow could give a hint that this feature is aimed at devs.
Would be great to get feedback on the wording from :Matej

Options:
"Temporarily install unsigned Add-on from File..."
"Test Add-on temporarily (Install from File)..."
"Install Add-on temporarily from File..."
"Install Add-on temporarily to debug..."
...


> 2. Modify the existing “Install Add-on from File…” functionality, so it can
> install both signed and unsigned add-ons
>   * However, this ability only exists in Dev Edition or Beta

Not possible, as the idea behind that feature is allowing devs to test in release.
 
> 3. On the file upload dialogue, have a checkbox that says “Only install
> signed add-ons” that is checked by default. If unchecked, then unsigned
> add-ons can be installed

In terms of discover-ability this seams very close to option 1. But would change a systems dialog, which probably makes it more complicated to implement.

> 4. Put the option to install unsigned add-ons behind a pref. If this pref is
> on, then user can select any unsigned add-on using this dialogue, and it
> will be installed.
>   * This may be the most obscure way of triggering this feature, but our
> devs should be familiar with about:preferences

I guess if we plan to have more dev-features implemented in release we might consider adding a button to turn on such a developer mode. For one additional line in one sub-menu on add-on manager, I am not sure if need need that.
 
> Personally, I’m in favour of approach #2 or #4. What do you think?

I would go with 1, and if we have multiple dev-features in release, add a pref, or button in settings to enable dev-mode.

> (Clearing Markus’ needinfo, but feel free to put it back on if the answer
> doesn’t satisfy).
Flags: needinfo?(matej)
(In reply to Markus Jaritz [:maritz] (UX) from comment #3)
> (In reply to Bram Pitoyo [:bram] from comment #2)
> (In reply to Andy McKay [:andym] from comment #1)
> > We were thinking that there would be another option in the add-ons menu gear
> > drop down (see below) that said, something like "Temporarily install
> > unsigned Add-on from File...". Although it might work for a directory or
> > .xpi, but I don't know if that needs differentiating.
> 
> Regarding the message I am not sure if we need the "unsigned" in it, 
> but somehow could give a hint that this feature is aimed at devs.
> Would be great to get feedback on the wording from :Matej
> 
> Options:
> "Temporarily install unsigned Add-on from File..."
> "Test Add-on temporarily (Install from File)..."
> "Install Add-on temporarily from File..."
> "Install Add-on temporarily to debug..."
> ...

I agree that you probably don't need "unsigned" here. I like the second and third options above, though the third is probably the clearest:

"Install Add-on temporarily from File..."

Also, is it standard to have "Add-on" capitalized in cases like this? In marketing copy we only capitalize it when it directly follows Firefox, which is why I'm asking.
Flags: needinfo?(matej)
(In reply to Matej Novak [:matej] from comment #4)
> Also, is it standard to have "Add-on" capitalized in cases like this? In
> marketing copy we only capitalize it when it directly follows Firefox, which
> is why I'm asking.

In the add-ons manager it is capitalized most of the time. see about:addons
Conforming to your convention would require to have it not capitalized in this dropdown menu as it is not about the brand. If this is what you suggest, we should file another bug to update that.
(In reply to Markus Jaritz [:maritz] (UX) from comment #5)
> (In reply to Matej Novak [:matej] from comment #4)
> > Also, is it standard to have "Add-on" capitalized in cases like this? In
> > marketing copy we only capitalize it when it directly follows Firefox, which
> > is why I'm asking.
> 
> In the add-ons manager it is capitalized most of the time. see about:addons
> Conforming to your convention would require to have it not capitalized in
> this dropdown menu as it is not about the brand. If this is what you
> suggest, we should file another bug to update that.

Nope, all good. Just wanted to make sure we were being consistent there. Thanks.
(In reply to Markus Jaritz [:maritz] (UX) from comment #5)
> (In reply to Matej Novak [:matej] from comment #4)
> > Also, is it standard to have "Add-on" capitalized in cases like this? In
> > marketing copy we only capitalize it when it directly follows Firefox, which
> > is why I'm asking.
> 
> In the add-ons manager it is capitalized most of the time. see about:addons
> Conforming to your convention would require to have it not capitalized in
> this dropdown menu as it is not about the brand. If this is what you
> suggest, we should file another bug to update that.

In the drop-down menu all menuitems which include the word have it capitalized: "Add-on" or "Add-ons"; OTOH the label in the search box just right of it has "Search all add-ons" in lowercase. I suppose developers have more urgent things to worry about than this, though.
So as I stated in bug 1185460, I'm willing to move forward with this and contribute with patches if that helps.

I already highlighted the new "about:debugging" page in Firefox.
This new page, instroduced by Devtools is meant to be a usual page for developers that need to debug stuff that aren't just a specific webpage:
 - shared/service workers,
 - addons,
 - apps,
All also allow to debug all these contexts, plus tabs for remote targets like:
 - firefox devices,
 - FxOS simulator addon (=FxOS desktop runtime)
 - fennec / firefox for android
 - and also chrome/ios tabs via "Valence" addon

We can add a UI in about:addons while trying to ensure it won't mislead regular Firefox users. I think Markus is right in comment 3, most users don't even go to about:addons. So adding something slightly hidden in a sub menu should work.
But I think we also have to add a UI to about:debugging as it fully targets developers. I opened bug 1221141 for this.
See Also: → 1221141
I don't know the timeline or plans for about:debugging is there anything outlining that? We are looking for something that can land and be usable as soon as possible, so adding something to the about:addons menu seems a good way to go. Thanks for filing that other bug.
Assignee: nobody → rhelmer
Blocks: 1226743
Status: NEW → ASSIGNED
(In reply to Matej Novak [:matej] from comment #4)
> "Install Add-on temporarily from File..."

1) since it is possible to install an add-on temporarily from both a directory and a file, is it worth trying any distinction here or should we stick with "File" in the label above?

2) do we want to display any kind of confirmation dialog as we do with the existing "Install Add-on from File..."?
(In reply to Robert Helmer [:rhelmer] from comment #10)
> (In reply to Matej Novak [:matej] from comment #4)
> > "Install Add-on temporarily from File..."
> 
> 1) since it is possible to install an add-on temporarily from both a
> directory and a file, is it worth trying any distinction here or should we
> stick with "File" in the label above?
> 
> 2) do we want to display any kind of confirmation dialog as we do with the
> existing "Install Add-on from File..."?

3) Related to #1 - our filepicker implementation does not seem to be able to select a directory/folder or a file, in the same dialog.

I think we'll need to have both "Install Add-on temporarily from File..." and "Install Add-on temporarily from Folder...". The former allows selecting multiple files but the latter does not allow selecting multiple folders.
Bug 1209344 - about:addons UI for temporarily-installed add-ons f?mossop
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

Wondering what you think of this approach - the main thing that's confusing here is that our platform-specific nsIFilePicker implementations (e.g. https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsFilePicker.mm but same for GTK and Windows) do not allow for selecting both files and folders in the same file picker. MDN used to imply that they did, I just corrected https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFilePicker and asked for technical review, just in case I am missing something here.

Furthermore it's only possible to multi-select files, but not folders.

So - this patch has a separate menu item for installing from File or from Folder. Note that the code is largely identical, I can refactor that to be shared if you agree with this approach.

Also just a quick question on tests - the only test I can find for the current "Install Add-on from File..." is https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_bug567127.js - should I make a new test, or should I rename that one and test this here? I expect there to be a lot of shared code between the two, not sure it's worth having a whole separate file, but I could rename it to something more generic without the bug # perhaps?
Attachment #8695626 - Flags: feedback?(dtownsend)
(In reply to Robert Helmer [:rhelmer] from comment #13)
> Comment on attachment 8695626 [details]
> MozReview Request: Bug 1209344 - about:addons UI for temporarily-installed
> add-ons f?mossop
> 
> Wondering what you think of this approach - the main thing that's confusing
> here is that our platform-specific nsIFilePicker implementations (e.g.
> https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsFilePicker.mm
> but same for GTK and Windows) do not allow for selecting both files and
> folders in the same file picker. MDN used to imply that they did, I just
> corrected
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIFilePicker and asked for technical review, just in case I am
> missing something here.
> 
> Furthermore it's only possible to multi-select files, but not folders.
> 
> So - this patch has a separate menu item for installing from File or from
> Folder. Note that the code is largely identical, I can refactor that to be
> shared if you agree with this approach.
> 
> Also just a quick question on tests - the only test I can find for the
> current "Install Add-on from File..." is
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/browser/browser_bug567127.js - should I make a new test, or should I
> rename that one and test this here? I expect there to be a lot of shared
> code between the two, not sure it's worth having a whole separate file, but
> I could rename it to something more generic without the bug # perhaps?

Oh I should also mention - there is no confirmation dialog currently, I am not sure it's desired but if so then I noticed that the current "Install Add-on from File..." code re-uses webInstaller.onWebInstallRequested() which we'll need to adjust (it requires AddonManager Install objects, which we don't have).

If a confirmation dialog is desired then I am sure we can figure out the right way to expose that, I just didn't want to do the work if it's not necessary :)
Flags: needinfo?(dtownsend)
This is a screenshot of the dialog on OS X 10.11.1 with attachment 8695626 [details] (fx-team repo).
I don't think this depends on bug 1185460, please re-instate if I'm mistaken.
No longer depends on: 1185460
Andy and I have been discussing whether we need this, now that there is about:debugging and it supports this. It has some of the issues that I brought up earlier in this bug, so I filed bug 1231129 which will allow it to load from unpacked dirs as well as files.

Do we still want this feature in about:addons, given that about:debugging exists?
I think it's a question of how discoverable about:debugging is, and how likely add-on developers are to look for this feature there. So, my questions are:

How are developers going to find out about about:debugging?

Are they mostly going to be looking for this feature when they're debugging (or when they're thinking about what they're doing as debugging)?

Are we going to continue to surface other debugging features in about:addons without this one? If so, are they going to know/remember they need to go somewhere else to get to this functionality?

Are most developers going to automatically open the add-on manager when they want to install an extension, and get frustrated when this functionality isn't there? If so, what are we going to do to guide them to the correct solution?
Also, I strongly suspect that, when loading from a folder, most developers aren't going to want the add-on to be temporary. They're probably going to be loading it that way because they work on it regularly, and want changes to be picked up with minimal extra effort. Only offering this for temporary add-ons is probably going to be frustrating.
(In reply to Kris Maglione [:kmag] from comment #19)
> Also, I strongly suspect that, when loading from a folder, most developers
> aren't going to want the add-on to be temporary. They're probably going to
> be loading it that way because they work on it regularly, and want changes
> to be picked up with minimal extra effort. Only offering this for temporary
> add-ons is probably going to be frustrating.

This is a good point - I think it is covered by bug 1185460.
(In reply to Robert Helmer [:rhelmer] from comment #20)
> (In reply to Kris Maglione [:kmag] from comment #19)
> > Also, I strongly suspect that, when loading from a folder, most developers
> > aren't going to want the add-on to be temporary. They're probably going to
> > be loading it that way because they work on it regularly, and want changes
> > to be picked up with minimal extra effort. Only offering this for temporary
> > add-ons is probably going to be frustrating.
> 
> This is a good point - I think it is covered by bug 1185460.

Hm actually looks like the title on that changed - I am not sure it's about being able to load an unpacked directory with signing enabled anymore, given that.

We should file a separate bug for this issue IMHO.
(In reply to Kris Maglione [:kmag] from comment #18)
> I think it's a question of how discoverable about:debugging is, and how
> likely add-on developers are to look for this feature there. So, my
> questions are:
> 
> How are developers going to find out about about:debugging?
> 
> Are they mostly going to be looking for this feature when they're debugging
> (or when they're thinking about what they're doing as debugging)?
> 
> Are we going to continue to surface other debugging features in about:addons
> without this one? If so, are they going to know/remember they need to go
> somewhere else to get to this functionality?
> 
> Are most developers going to automatically open the add-on manager when they
> want to install an extension, and get frustrated when this functionality
> isn't there? If so, what are we going to do to guide them to the correct
> solution?

I don't know the answers to these questions, but a few folks have mentioned that it would be nice to move developer-focused features out of about:addons to devtools (e.g. about:debugging).

I don't mind doing this in about:addons and the current patch I put up for feedback WFM, but if we're just going to move it out of about:addons later then it'd be less confusing for add-on developers to decide that now.
(In reply to Robert Helmer [:rhelmer] from comment #22)
> I don't know the answers to these questions, but a few folks have mentioned
> that it would be nice to move developer-focused features out of about:addons
> to devtools (e.g. about:debugging).
> 
> I don't mind doing this in about:addons and the current patch I put up for
> feedback WFM, but if we're just going to move it out of about:addons later
> then it'd be less confusing for add-on developers to decide that now.

Yeah, that's fair. Either way, I'd rather be consistent, so if we don't add this to about:addons, it would be nice to remove the Debug buttons, and add something to point developers to about:debugging, as soon as possible.
(In reply to Robert Helmer [:rhelmer] from comment #14)
> (In reply to Robert Helmer [:rhelmer] from comment #13)
> > Comment on attachment 8695626 [details]
> > MozReview Request: Bug 1209344 - about:addons UI for temporarily-installed
> > add-ons f?mossop
> > 
> > Wondering what you think of this approach - the main thing that's confusing
> > here is that our platform-specific nsIFilePicker implementations (e.g.
> > https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsFilePicker.mm
> > but same for GTK and Windows) do not allow for selecting both files and
> > folders in the same file picker. MDN used to imply that they did, I just
> > corrected
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > Interface/nsIFilePicker and asked for technical review, just in case I am
> > missing something here.
> > 
> > Furthermore it's only possible to multi-select files, but not folders.
> > 
> > So - this patch has a separate menu item for installing from File or from
> > Folder. Note that the code is largely identical, I can refactor that to be
> > shared if you agree with this approach.
> > 
> > Also just a quick question on tests - the only test I can find for the
> > current "Install Add-on from File..." is
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > test/browser/browser_bug567127.js - should I make a new test, or should I
> > rename that one and test this here? I expect there to be a lot of shared
> > code between the two, not sure it's worth having a whole separate file, but
> > I could rename it to something more generic without the bug # perhaps?
> 
> Oh I should also mention - there is no confirmation dialog currently, I am
> not sure it's desired but if so then I noticed that the current "Install
> Add-on from File..." code re-uses webInstaller.onWebInstallRequested() which
> we'll need to adjust (it requires AddonManager Install objects, which we
> don't have).
> 
> If a confirmation dialog is desired then I am sure we can figure out the
> right way to expose that, I just didn't want to do the work if it's not
> necessary :)

I think if we're putting this in about:debugging then we don't need confirmation, we should probably display a message saying that the add-on has been installed and include the add-on's name to confirm they got the right one.
Flags: needinfo?(dtownsend)
(In reply to Kris Maglione [:kmag] from comment #23)
> (In reply to Robert Helmer [:rhelmer] from comment #22)
> > I don't know the answers to these questions, but a few folks have mentioned
> > that it would be nice to move developer-focused features out of about:addons
> > to devtools (e.g. about:debugging).
> > 
> > I don't mind doing this in about:addons and the current patch I put up for
> > feedback WFM, but if we're just going to move it out of about:addons later
> > then it'd be less confusing for add-on developers to decide that now.
> 
> Yeah, that's fair. Either way, I'd rather be consistent, so if we don't add
> this to about:addons, it would be nice to remove the Debug buttons, and add
> something to point developers to about:debugging, as soon as possible.

OK after chatting w/ Dave and Andy and devtools folks as well, I've morphed this bug to be about removing the debug buttons from about:addons and linking to about:debugging.
(In reply to Dave Townsend [:mossop] from comment #24)
> (In reply to Robert Helmer [:rhelmer] from comment #14)
> > (In reply to Robert Helmer [:rhelmer] from comment #13)
> > > Comment on attachment 8695626 [details]
> > > MozReview Request: Bug 1209344 - about:addons UI for temporarily-installed
> > > add-ons f?mossop
> > > 
> > > Wondering what you think of this approach - the main thing that's confusing
> > > here is that our platform-specific nsIFilePicker implementations (e.g.
> > > https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsFilePicker.mm
> > > but same for GTK and Windows) do not allow for selecting both files and
> > > folders in the same file picker. MDN used to imply that they did, I just
> > > corrected
> > > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > > Interface/nsIFilePicker and asked for technical review, just in case I am
> > > missing something here.
> > > 
> > > Furthermore it's only possible to multi-select files, but not folders.
> > > 
> > > So - this patch has a separate menu item for installing from File or from
> > > Folder. Note that the code is largely identical, I can refactor that to be
> > > shared if you agree with this approach.
> > > 
> > > Also just a quick question on tests - the only test I can find for the
> > > current "Install Add-on from File..." is
> > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > > test/browser/browser_bug567127.js - should I make a new test, or should I
> > > rename that one and test this here? I expect there to be a lot of shared
> > > code between the two, not sure it's worth having a whole separate file, but
> > > I could rename it to something more generic without the bug # perhaps?
> > 
> > Oh I should also mention - there is no confirmation dialog currently, I am
> > not sure it's desired but if so then I noticed that the current "Install
> > Add-on from File..." code re-uses webInstaller.onWebInstallRequested() which
> > we'll need to adjust (it requires AddonManager Install objects, which we
> > don't have).
> > 
> > If a confirmation dialog is desired then I am sure we can figure out the
> > right way to expose that, I just didn't want to do the work if it's not
> > necessary :)
> 
> I think if we're putting this in about:debugging then we don't need
> confirmation, we should probably display a message saying that the add-on
> has been installed and include the add-on's name to confirm they got the
> right one.
Summary: Implement a UI for loading a temporary add-on from a local folder for testing → remove "debug" buttons on about:addons and refer to about:debugging
Attachment #8695626 - Flags: feedback?(dtownsend)
Attachment #8695626 - Attachment description: MozReview Request: Bug 1209344 - about:addons UI for temporarily-installed add-ons f?mossop → MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop
Attachment #8695626 - Flags: review?(dtownsend)
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27159/diff/1-2/
Bug 1209344 - link to about:debugging from about:addons r?mossop
Attachment #8698196 - Flags: review?(dtownsend)
Attachment #8695632 - Attachment is obsolete: true
Here's a screen shot of attachment 8698196 [details] - this adds a "Debug Add-ons" command in the "Gear" button pull-down menu in about:addons. The other patch removes "Debug" buttons from about:addons.

The "Debug Add-ons" command opens about:debugging#addons (or focuses the tab if already open).
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

https://reviewboard.mozilla.org/r/27159/#review25083
Attachment #8695626 - Flags: review?(dtownsend) → review+
Comment on attachment 8698196 [details]
MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop

https://reviewboard.mozilla.org/r/27851/#review25085

::: toolkit/mozapps/extensions/test/browser/browser_debug_addons.js:22
(Diff revision 1)
> +      });

Looks like you've indented slightly too much here
Attachment #8698196 - Flags: review?(dtownsend) → review+
Attachment #8695626 - Attachment description: MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop → MozReview Request: Bug 1209344 - remove debug button from about:addons r=mossop
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27159/diff/2-3/
Comment on attachment 8698196 [details]
MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27851/diff/1-2/
Attachment #8698196 - Attachment description: MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop → MozReview Request: Bug 1209344 - link to about:debugging from about:addons r=mossop
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

https://reviewboard.mozilla.org/r/27159/#review25101
Attachment #8695626 - Flags: review+
Comment on attachment 8698196 [details]
MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop

https://reviewboard.mozilla.org/r/27851/#review25105
Attachment #8698196 - Flags: review+
(In reply to Dave Townsend [:mossop] from comment #31)
> Comment on attachment 8698196 [details]
> MozReview Request: Bug 1209344 - link to about:debugging from about:addons
> r=mossop
> 
> https://reviewboard.mozilla.org/r/27851/#review25085
> 
> ::: toolkit/mozapps/extensions/test/browser/browser_debug_addons.js:22
> (Diff revision 1)
> > +      });
> 
> Looks like you've indented slightly too much here

Can't wait until we enable the eslint style rules :)
Keywords: checkin-needed
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6216881&repo=fx-team
Flags: needinfo?(rhelmer)
(In reply to Carsten Book [:Tomcat] from comment #39)
> sorry backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=6216881&repo=fx-team

Oops I should've caught that in the try run - thanks!
Flags: needinfo?(rhelmer)
Attachment #8695626 - Attachment description: MozReview Request: Bug 1209344 - remove debug button from about:addons r=mossop → MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27159/diff/3-4/
Comment on attachment 8698196 [details]
MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27851/diff/2-3/
Attachment #8698196 - Attachment description: MozReview Request: Bug 1209344 - link to about:debugging from about:addons r=mossop → MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop
(In reply to Robert Helmer [:rhelmer] from comment #42)
> (In reply to Carsten Book [:Tomcat] from comment #39)
> > sorry backed out for test failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=6216881&repo=fx-team
> 
> Oops I should've caught that in the try run - thanks!

The reason this happened is that I made a mistake in the "try:" syntax and it didn't run the tests that I intended it to.

The failing test now WFM locally (I had removed a bit of code that about:debugging still depends on from XPIProviders.jsm), also pushing to tryserver w/ the correct syntax now to make sure there isn't anything else.
Comment on attachment 8698196 [details]
MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27851/diff/3-4/
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ac14b1af757&selectedJob=15100725

I don't *think* any of the failures here are related to this patch, so this is ready for review.
Flags: needinfo?(dtownsend)
https://reviewboard.mozilla.org/r/27157/#review26539

::: toolkit/mozapps/extensions/test/browser/browser-common.ini
(Diff revision 5)
> -[browser_debug_button.js]

Please remove the test file too.
Flags: needinfo?(dtownsend)
Comment on attachment 8695626 [details]
MozReview Request: Bug 1209344 - remove debug button from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27159/diff/4-5/
Comment on attachment 8698196 [details]
MozReview Request: Bug 1209344 - link to about:debugging from about:addons r?mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27851/diff/4-5/
Setting checkin-needed since I can't use autoland at the moment (bug 1237380)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/330e05d5ee79
https://hg.mozilla.org/mozilla-central/rev/9cb535d7ea7e
https://hg.mozilla.org/mozilla-central/rev/1982e6826fb8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1237624
This issue is verified as fixed on Firefox 51.0a1 (2016-09-14) on Windows 7 64-bit.

The “Debug” buttons are now displayed only in about:debugging and the "Debug Add-ons" command from the “Gear” button pull-down menu in about:addons opens about:debugging#addons (or focuses the tab if already open).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.