Last Comment Bug 655776 - Web Developer > Get More Tools menu item
: Web Developer > Get More Tools menu item
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 11:31 PDT by Kevin Dangoor
Modified: 2011-05-26 05:03 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (4.36 KB, patch)
2011-05-12 03:34 PDT, Mihai Sucan [:msucan]
sdwilsh: review+
dao+bmo: review-
Details | Diff | Splinter Review
updated patch (4.47 KB, patch)
2011-05-23 13:32 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
patch update 2 (3.57 KB, patch)
2011-05-23 13:49 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
patch update 3 (3.63 KB, patch)
2011-05-23 14:00 PDT, Mihai Sucan [:msucan]
dao+bmo: review+
limi: ui‑review+
Details | Diff | Splinter Review

Description Kevin Dangoor 2011-05-09 11:31:04 PDT
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.
Comment 1 Kevin Dangoor 2011-05-09 11:31:56 PDT
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.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-05-09 11:55:28 PDT
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?
Comment 3 Rob Campbell [:rc] (:robcee) 2011-05-09 11:56:04 PDT
presumably we'll want this in the Web Developer sub-menu within the App Menu as well?
Comment 4 Kevin Dangoor 2011-05-10 12:50:12 PDT
No idea who owns the collection. I'll ask around.

and yes, we should add this to the app menu as well.
Comment 5 Mihai Sucan [:msucan] 2011-05-12 03:34:44 PDT
Created attachment 531883 [details] [diff] [review]
proposed patch

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!
Comment 6 Shawn Wilsher :sdwilsh 2011-05-23 09:52:27 PDT
Comment on attachment 531883 [details] [diff] [review]
proposed patch

r=sdwilsh
Comment 7 Mihai Sucan [:msucan] 2011-05-23 11:52:30 PDT
Comment on attachment 531883 [details] [diff] [review]
proposed patch

http://hg.mozilla.org/mozilla-central/rev/aca490d48c95
Comment 8 Alexander L. Slovesnik 2011-05-23 12:52:27 PDT
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 9 Dão Gottwald [:dao] 2011-05-23 12:56:45 PDT
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.
Comment 10 Shawn Wilsher :sdwilsh 2011-05-23 12:56:50 PDT
(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 11 Dão Gottwald [:dao] 2011-05-23 12:59:33 PDT
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 :(
Comment 12 Mihai Sucan [:msucan] 2011-05-23 13:05:48 PDT
(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?
Comment 13 Mihai Sucan [:msucan] 2011-05-23 13:06:53 PDT
(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.
Comment 14 Mihai Sucan [:msucan] 2011-05-23 13:08:02 PDT
(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!
Comment 15 Dão Gottwald [:dao] 2011-05-23 13:10:00 PDT
> > 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.
Comment 16 Dave Camp (:dcamp) 2011-05-23 13:16:25 PDT
Backed out in http://hg.mozilla.org/mozilla-central/rev/a6aa4dc6ce83
Comment 17 Mihai Sucan [:msucan] 2011-05-23 13:32:07 PDT
Created attachment 534545 [details] [diff] [review]
updated patch

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!
Comment 18 Dão Gottwald [:dao] 2011-05-23 13:37:11 PDT
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?
Comment 19 Dão Gottwald [:dao] 2011-05-23 13:39:10 PDT
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.
Comment 20 Mihai Sucan [:msucan] 2011-05-23 13:49:18 PDT
Created attachment 534552 [details] [diff] [review]
patch update 2

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!
Comment 21 Dão Gottwald [:dao] 2011-05-23 13:53:51 PDT
How about the separator?
Comment 22 Dão Gottwald [:dao] 2011-05-23 13:56:17 PDT
Also, I tend to think there should be no ellipsis in the label.
Comment 23 Mihai Sucan [:msucan] 2011-05-23 13:59:36 PDT
(In reply to comment #21)
> How about the separator?

I forgot it. :)
Comment 24 Mihai Sucan [:msucan] 2011-05-23 14:00:51 PDT
Created attachment 534555 [details] [diff] [review]
patch update 3

Added menu separator and removed the ellipsis from the label.
Comment 25 Dão Gottwald [:dao] 2011-05-23 14:07:32 PDT
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.
Comment 26 Dave Camp (:dcamp) 2011-05-23 14:14:51 PDT
This also broke the test at browser/base/content/test/browser_webdev_menu.js
Comment 27 Dão Gottwald [:dao] 2011-05-23 14:20:03 PDT
Not a productive test. We can probably safely get rid of this.
Comment 28 Nochum Sossonko [:Natch] 2011-05-23 14:22:07 PDT
(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...
Comment 29 Shawn Wilsher :sdwilsh 2011-05-23 14:22:47 PDT
(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 30 Alex Limi (:limi) — Firefox UX Team 2011-05-23 17:54:33 PDT
Comment on attachment 534555 [details] [diff] [review]
patch update 3

Works for me.
Comment 31 Dão Gottwald [:dao] 2011-05-23 22:54:35 PDT
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...
Comment 32 Dão Gottwald [:dao] 2011-05-24 03:07:52 PDT
http://hg.mozilla.org/mozilla-central/rev/90465908ac89
Comment 33 Mihai Sucan [:msucan] 2011-05-24 03:25:58 PDT
Thank you guys!
Comment 34 AndreiD[QA] 2011-05-26 05:03:09 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.