655776 - Web Developer > Get More Tools menu item

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Kevin Dangoor]

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]

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)]

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)]

presumably we'll want this in the Web Developer sub-menu within the App Menu as well?

Comment 4

[Kevin Dangoor]

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]]

Attachment: 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]

Comment on attachment 531883 [details] [diff] [review]
proposed patch

r=sdwilsh

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 531883 [details] [diff] [review]
proposed patch

http://hg.mozilla.org/mozilla-central/rev/aca490d48c95

Comment 8

[Alexander L. Slovesnik]

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]]

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]

(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]]

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]]

(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]]

(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]]

(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]]

> > 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)]

Backed out in http://hg.mozilla.org/mozilla-central/rev/a6aa4dc6ce83

Comment 17

[Mihai Sucan [:msucan]]

Attachment: 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]]

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]]

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]]

Attachment: 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]]

How about the separator?

Comment 22

[Dão Gottwald [:dao]]

Also, I tend to think there should be no ellipsis in the label.

Comment 23

[Mihai Sucan [:msucan]]

(In reply to comment #21)
> How about the separator?

I forgot it. :)

Comment 24

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Added menu separator and removed the ellipsis from the label.

Comment 25

[Dão Gottwald [: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.

Comment 26

[Dave Camp (:dcamp)]

This also broke the test at browser/base/content/test/browser_webdev_menu.js

Comment 27

[Dão Gottwald [:dao]]

Not a productive test. We can probably safely get rid of this.

Comment 28

[Nochum Sossonko [:Natch]]

(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]

(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]

Comment on attachment 534555 [details] [diff] [review]
patch update 3

Works for me.

Comment 31

[Dão Gottwald [:dao]]

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]]

http://hg.mozilla.org/mozilla-central/rev/90465908ac89

Comment 33

[Mihai Sucan [:msucan]]

Thank you guys!

Comment 34

[AndreiD[QA]]

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.