Web Developer > Get More Tools menu item

VERIFIED FIXED in Firefox 6

Status

()

Firefox
Developer Tools
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Kevin Dangoor, Assigned: msucan)

Tracking

Trunk
Firefox 6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Comment 4

6 years ago
No idea who owns the collection. I'll ask around.

and yes, we should add this to the app menu as well.
(Assignee)

Comment 5

6 years ago
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!
Attachment #531883 - Flags: feedback?(rcampbell)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Reporter)

Updated

6 years ago
Attachment #531883 - Flags: review?(sdwilsh)
Comment on attachment 531883 [details] [diff] [review]
proposed patch

r=sdwilsh
Attachment #531883 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 7

6 years ago
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)
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
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-
(Assignee)

Comment 12

6 years ago
(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?
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
(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.

Comment 16

6 years ago
Backed out in http://hg.mozilla.org/mozilla-central/rev/a6aa4dc6ce83
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

6 years ago
Attachment #531883 - Attachment description: [checked-in] proposed patch → proposed patch
(Assignee)

Comment 17

6 years ago
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!
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.
(Assignee)

Comment 20

6 years ago
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!
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.
(Assignee)

Comment 23

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

I forgot it. :)
(Assignee)

Comment 24

6 years ago
Created attachment 534555 [details] [diff] [review]
patch update 3

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.

Comment 26

6 years ago
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 33

6 years ago
Thank you guys!

Comment 34

6 years ago
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
You need to log in before you can comment on or make changes to this bug.