Last Comment Bug 866507 - Style Inspector: open URLs in new tab
: Style Inspector: open URLs in new tab
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 23
Assigned To: briangrinstead
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-28 07:09 PDT by briangrinstead
Modified: 2013-05-11 10:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial patch that opens external URLs in new tab instead of new window (1.73 KB, patch)
2013-04-29 15:17 PDT, briangrinstead
mratcliffe: review+
Details | Diff | Review

Description briangrinstead 2013-04-28 07:09:16 PDT
Recently added the ability to open an external URL in the Style Inspector in a new window: https://bugzilla.mozilla.org/show_bug.cgi?id=677930.

We should use openUILinkIn to allow us to open multiple links in multiple tabs, as opposed to opening the resource in a new window.
Comment 1 briangrinstead 2013-04-29 15:17:04 PDT
Created attachment 743307 [details] [diff] [review]
Initial patch that opens external URLs in new tab instead of new window
Comment 2 briangrinstead 2013-04-29 15:24:11 PDT
Michael,
I have submitted a patch for review.  A couple of notes:
1) I used the fat arrow function to bind `this` on the event handler.  I've seen that in use elsewhere in the devtools code, so figured this would be OK.
2) It may not be needed to actually use an <a> tag now that we are opening in a new tab, but I left it in since it would likely be just setting an attribute on a span rather than href on the a tag.
3) Regarding testing, I am currently just checking the `href` attribute to make sure it is correct - do you think it would be good to somehow also check to make sure a tab opens?

Thanks, and let me know if there is anything I can do to improve the patch.
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2013-04-30 06:29:40 PDT
Comment on attachment 743307 [details] [diff] [review]
Initial patch that opens external URLs in new tab instead of new window

Review of attachment 743307 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Grinstead from comment #2)
> Michael,
> I have submitted a patch for review.  A couple of notes:
> 1) I used the fat arrow function to bind `this` on the event handler.  I've
> seen that in use elsewhere in the devtools code, so figured this would be OK.

Yes, we like fat arrow functions ;o)

> 2) It may not be needed to actually use an <a> tag now that we are opening
> in a new tab, but I left it in since it would likely be just setting an
> attribute on a span rather than href on the a tag.

In this case an a tag makes sense.

> 3) Regarding testing, I am currently just checking the `href` attribute to
> make sure it is correct - do you think it would be good to somehow also
> check to make sure a tab opens?
> 

You could do that but programatically clicking things can cause problems due by focus issues. I think the current test is fine.


> Thanks, and let me know if there is anything I can do to improve the patch.

Nope, it is great as it is.
Comment 4 Rob Campbell [:rc] (:robcee) 2013-05-06 06:04:47 PDT
has this run through try or through a local test run? If either answer is true, this can land?
Comment 5 briangrinstead 2013-05-06 18:36:07 PDT
I don't have access to the try server, but I have run the local tests with mach mochitest-browser and everything is green.  I'm sure Michael could give a more definitive answer.
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2013-05-07 03:58:13 PDT
I will run it through try.
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2013-05-07 04:01:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1333a39b38d3
Comment 8 Rob Campbell [:rc] (:robcee) 2013-05-07 05:43:52 PDT
got some broken builds on Win7 and Fedora.

Looking at the logs they look like infrastructure blow-outs. Retriggered the builds to see what happens.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2013-05-08 03:24:03 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c76ccea39da1
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2013-05-10 07:11:08 PDT
https://hg.mozilla.org/integration/fx-team/rev/117a4b00981a
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-05-11 10:19:28 PDT
https://hg.mozilla.org/mozilla-central/rev/117a4b00981a

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