Web Developer > Get More Tools menu item

VERIFIED FIXED in Firefox 6

Status

defect
VERIFIED FIXED
8 years ago
Last year

People

(Reporter: dangoor, Assigned: msucan)

Tracking

Trunk
Firefox 6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

In bug 653221, we added a Web Developer submenu to the tools menu (a Web Developer menu already exists on Windows/Linux off of the Firefox menu).

In email with Alexander Limi, I asked what he thought of a "Get More Tools" menu item that open a browser to:

https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/

This should be the last item on the menu.
changing assigning to msucan. This should be simple to do for anyone, but I know Rob has some big immediate term things on his plate.
Assignee: rcampbell → mihai.sucan
Good idea! A couple of thoughts: only one of those add-ons (the Mozilla provided Test Pilot Add On) is listed as compatible with Nightly 6.0a1. 

There are also a couple of add-ons I'd like to see added to that list: Patrick's FrameRate Reporter, the Memory Meter and probably others. Do we know who owns that collection?
presumably we'll want this in the Web Developer sub-menu within the App Menu as well?
OS: Mac OS X → All
Hardware: x86 → All
No idea who owns the collection. I'll ask around.

and yes, we should add this to the app menu as well.
Posted patch proposed patch (obsolete) — Splinter Review
Proposed patch.

Please note that I did not include a test because this is trivial functionality which is well exercised within Mozilla's codebase.

I have also not included an accesskey for the menuitem since the purpose of the command is not to be repeatedly called, by users. I think we should save up on the accesskeys usage ... on commands that devs actually use frequently.

Looking forward to your feedback, thanks!
Attachment #531883 - Flags: feedback?(rcampbell)
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Attachment #531883 - Flags: review?(sdwilsh)
Comment on attachment 531883 [details] [diff] [review]
proposed patch

r=sdwilsh
Attachment #531883 - Flags: review?(sdwilsh) → review+
Comment on attachment 531883 [details] [diff] [review]
proposed patch

http://hg.mozilla.org/mozilla-central/rev/aca490d48c95
Attachment #531883 - Attachment description: proposed patch → [checked-in] proposed patch
Attachment #531883 - Flags: feedback?(rcampbell)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Is it really nessesary to hardcode "en-US" in https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/?
You can use https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/ and Firefox will forward user to localized version of AMO according to browser language.
Comment on attachment 531883 [details] [diff] [review]
proposed patch

>--- a/browser/base/content/browser-menubar.inc
>+++ b/browser/base/content/browser-menubar.inc
>@@ -562,16 +562,19 @@
>                             key="key_viewSource"
>                             command="View:PageSource"/>
>                   <menuitem id="javascriptConsole"
>                             hidden="true"
>                             label="&errorConsoleCmd.label;"
>                             accesskey="&errorConsoleCmd.accesskey;"
>                             key="key_errorConsole"
>                             oncommand="toJavaScriptConsole();"/>
>+                  <menuitem id="getMoreDevtools"
>+                            label="&getMoreDevtoolsCmd.label;"
>+                            oncommand="Devtools.getMoreLink();"/>

You're missing an accesskey here.

This should probably have gotten ui-review, too.
(In reply to comment #8)
> https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/ and
> Firefox will forward user to localized version of AMO according to browser
> language.
I didn't know about that!  We should totally do that; can you file a follow-up bug for that please?
Comment on attachment 531883 [details] [diff] [review]
proposed patch

>+var Devtools = {
>+  getMoreLink: function Devtools_getMoreLink() {
>+    switchToTabHavingURI("https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/", true);
>+  },

Oh, and this really shouldn't be using switchToTabHavingURI. Please back out :(
Attachment #531883 - Flags: review-
(In reply to comment #11)
> Comment on attachment 531883 [details] [diff] [review] [review]
> [checked-in] proposed patch
> 
> >+var Devtools = {
> >+  getMoreLink: function Devtools_getMoreLink() {
> >+    switchToTabHavingURI("https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/", true);
> >+  },
> 
> Oh, and this really shouldn't be using switchToTabHavingURI. Please back out
> :(

What should it use?
(In reply to comment #9)
> Comment on attachment 531883 [details] [diff] [review] [review]
> [checked-in] proposed patch
> 
> >--- a/browser/base/content/browser-menubar.inc
> >+++ b/browser/base/content/browser-menubar.inc
> >@@ -562,16 +562,19 @@
> >                             key="key_viewSource"
> >                             command="View:PageSource"/>
> >                   <menuitem id="javascriptConsole"
> >                             hidden="true"
> >                             label="&errorConsoleCmd.label;"
> >                             accesskey="&errorConsoleCmd.accesskey;"
> >                             key="key_errorConsole"
> >                             oncommand="toJavaScriptConsole();"/>
> >+                  <menuitem id="getMoreDevtools"
> >+                            label="&getMoreDevtoolsCmd.label;"
> >+                            oncommand="Devtools.getMoreLink();"/>
> 
> You're missing an accesskey here.

I did not add an accesskey on purpose, see comment 5.
(In reply to comment #8)
> Is it really nessesary to hardcode "en-US" in
> https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/?
> You can use
> https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/ and
> Firefox will forward user to localized version of AMO according to browser
> language.

I can make a quick follow-up patch for this. I didn't know that I can remove the en-US hardcoding. Thanks!
> > Oh, and this really shouldn't be using switchToTabHavingURI. Please back out
> > :(
> 
> What should it use?

openUILinkIn

> I did not add an accesskey on purpose, see comment 5.

Comment 5 seems to be confusing command and access keys. Every item in the menu bar is supposed to have an access key, modulo some exceptions like the bookmarks and history menus.

> > Is it really nessesary to hardcode "en-US" in
> > https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/?
> > You can use
> > https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/ and
> > Firefox will forward user to localized version of AMO according to browser
> > language.
> 
> I can make a quick follow-up patch for this. I didn't know that I can remove
> the en-US hardcoding. Thanks!

No, please back out and attach a fresh patch.
Backed out in http://hg.mozilla.org/mozilla-central/rev/a6aa4dc6ce83
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #531883 - Attachment description: [checked-in] proposed patch → proposed patch
Posted patch updated patch (obsolete) — Splinter Review
Updated the patch as requested by Dao.

Please review the patch, as we would like it to land in time for Firefox 6. Thank you!
Attachment #531883 - Attachment is obsolete: true
Attachment #534545 - Flags: ui-review?(limi)
Attachment #534545 - Flags: review?(sdwilsh)
Attachment #534545 - Flags: review?(dao)
Comment on attachment 534545 [details] [diff] [review]
updated patch

I think there should be a separator before the menu item, in both instances, since it's different from the other items.

>+                  <menuitem id="getMoreDevtools"
>+                            label="&getMoreDevtoolsCmd.label;"
>+                            accesskey="&getMoreDevtoolsCmd.accesskey;"
>+                            oncommand="Devtools.getMoreLink();"/>

>+var Devtools = {
>+  getMoreLink: function Devtools_getMoreLink() {
>+    openUILinkIn("https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/", "tab");
>+  },
>+};

The Devtools object seems slightly lost here. Can you just define something like DEVTOOLS_GET_ADDONS_URI and do oncommand="openUILinkIn(DEVTOOLS_GET_ADDONS_URI, 'tab');" directly?
Or even just oncommand="openUILinkIn('https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/', 'tab');". I'm not sure if we should care about the URL being hardcoded in two places.
Posted patch patch update 2 (obsolete) — Splinter Review
Another patch update - changed as requested by Dao.

For Limi a screenshot:

http://img.i7m.de/show/plozo.png

Not asking Shawn for review again, actually, since he gave the patch r+ already. ;)

Thanks!
Attachment #534545 - Attachment is obsolete: true
Attachment #534545 - Flags: ui-review?(limi)
Attachment #534545 - Flags: review?(sdwilsh)
Attachment #534545 - Flags: review?(dao)
Attachment #534552 - Flags: ui-review?(limi)
Attachment #534552 - Flags: review?(dao)
How about the separator?
Also, I tend to think there should be no ellipsis in the label.
(In reply to comment #21)
> How about the separator?

I forgot it. :)
Added menu separator and removed the ellipsis from the label.
Attachment #534552 - Attachment is obsolete: true
Attachment #534552 - Flags: ui-review?(limi)
Attachment #534552 - Flags: review?(dao)
Attachment #534555 - Flags: ui-review?(limi)
Attachment #534555 - Flags: review?(dao)
Comment on attachment 534555 [details] [diff] [review]
patch update 3

>--- a/browser/base/content/browser-appmenu.inc
>+++ b/browser/base/content/browser-appmenu.inc
>@@ -197,16 +197,20 @@
>                     command="View:PageSource"
>                     key="key_viewSource"/>
>           <menuitem id="appmenu_errorConsole"
>                     hidden="true"
>                     label="&errorConsoleCmd.label;"
>                     key="key_errorConsole"
>                     oncommand="toJavaScriptConsole();"/>
>           <menuseparator/>
>+          <menuitem id="appmenu_getMoreDevtools"
>+                    label="&getMoreDevtoolsCmd.label;"
>+                    oncommand="openUILinkIn('https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/', 'tab');"/>
>+          <menuseparator/>

The separator before your new item needs an id, so that extensions can add items before it. Something like appmenu_devToolsEndSeparator. (I suck at making up names. Please always try to find something better than what I suggest.)

>--- a/browser/base/content/browser-menubar.inc
>+++ b/browser/base/content/browser-menubar.inc
>@@ -562,16 +562,21 @@
>                             key="key_viewSource"
>                             command="View:PageSource"/>
>                   <menuitem id="javascriptConsole"
>                             hidden="true"
>                             label="&errorConsoleCmd.label;"
>                             accesskey="&errorConsoleCmd.accesskey;"
>                             key="key_errorConsole"
>                             oncommand="toJavaScriptConsole();"/>
>+                  <menuseparator/>
>+                  <menuitem id="getMoreDevtools"
>+                            label="&getMoreDevtoolsCmd.label;"
>+                            accesskey="&getMoreDevtoolsCmd.accesskey;"
>+                            oncommand="openUILinkIn('https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/', 'tab');"/>

Same here.
This also broke the test at browser/base/content/test/browser_webdev_menu.js
Not a productive test. We can probably safely get rid of this.
(In reply to comment #27)
> Not a productive test. We can probably safely get rid of this.

This might be testing the default hidden menuitems (unless there's another test for that). That might be productive somewhat...
(In reply to comment #27)
> Not a productive test. We can probably safely get rid of this.
Yeah, that test would be more useful if it checked that it did the expected action when it was clicked/selected, but as it stands it is not useful.
Comment on attachment 534555 [details] [diff] [review]
patch update 3

Works for me.
Attachment #534555 - Flags: ui-review?(limi) → ui-review+
Comment on attachment 534555 [details] [diff] [review]
patch update 3

I'll land this with separator ids added and browser_webdev_menu.js removed, when the tree allows it.

I'm still not sure about the ellipsis. Ideally someone who doesn't just have a gut feeling but knows the rules would chime in here...
Attachment #534555 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/90465908ac89
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Thank you guys!
Verified fixed on:
Windows 7:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Window XP:
Mozilla/5.0 (Windows NT 5.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Mac OS 10.6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 
Linux i686:
Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

*Note: Under Tools->Web Developer there is a "Get more tools" option that opens the page https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/ in a new tab. Marking this bug as Verified.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.