Last Comment Bug 620997 - Open "Links" URLs in browser
: Open "Links" URLs in browser
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Page Info (show other bugs)
: Trunk
: Other Other
: -- normal (vote)
: ---
Assigned To: Edmund Wong (:ewong)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-22 10:31 PST by Karsten Düsterloh
Modified: 2012-05-25 07:48 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Open 'Links' URLs in browser. (5.84 KB, patch)
2012-05-05 06:51 PDT, Edmund Wong (:ewong)
neil: review-
Details | Diff | Review
Open 'Links' URLs in browser. (v2) (7.00 KB, patch)
2012-05-08 08:01 PDT, Edmund Wong (:ewong)
neil: review+
Details | Diff | Review
Open 'Links' URLs in browser (v3) (6.90 KB, patch)
2012-05-11 20:44 PDT, Edmund Wong (:ewong)
neil: review-
Details | Diff | Review
Open 'Links' URLs in browser. (v4) (9.13 KB, patch)
2012-05-12 19:46 PDT, Edmund Wong (:ewong)
neil: review+
Details | Diff | Review
Open 'Links' URLs in browser (v5) (10.00 KB, patch)
2012-05-13 19:24 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Review
Open 'Links' URLs in browser (v5) (10.06 KB, patch)
2012-05-13 19:33 PDT, Edmund Wong (:ewong)
neil: review-
Details | Diff | Review
Open 'Links' URLs in browser (v6) (9.48 KB, patch)
2012-05-15 21:59 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Review
Open 'Links' URLs in browser (v6) (9.08 KB, patch)
2012-05-15 22:05 PDT, Edmund Wong (:ewong)
neil: review+
Details | Diff | Review
Open 'Links' URLs in browser. (v7) (9.05 KB, patch)
2012-05-20 03:45 PDT, Edmund Wong (:ewong)
ewong: review+
Details | Diff | Review

Description Karsten Düsterloh 2010-12-22 10:31:50 PST
Page Info's "Links" tabs shows all links of a page, but you can't anything with them except copying. 

The context menu should have the usual options to open in a new window/tab etc., and (double/middle)clicking should work as usual as well.
Comment 1 Edmund Wong (:ewong) 2012-05-05 06:51:57 PDT
Created attachment 621293 [details] [diff] [review]
Open 'Links' URLs in browser.
Comment 2 neil@parkwaycc.co.uk 2012-05-05 11:49:45 PDT
Comment on attachment 621293 [details] [diff] [review]
Open 'Links' URLs in browser.

>+function getList()
You should move this function earlier and make doCopy call it too.
I'd like to think there was a better name for this but I can't think of one.

>+function onOpenInTabClick()
>+{
>+  var browser = document.getElementById("content");
Unnecessary.

>+  var linkList = getList();
>+
>+  if (linkList)
This is always true, because getList() returns an array object. Maybe you meant to check the length?

>+    while (tmp = linkList.pop())
>+      openNewTabWith(tmp, gDocument);
This probably opens the tabs in the wrong order. Also it will fail as soon as you get to an insecure link. Calling openUILinkArrayIn would be better except that it does no security checks at all. (This could be fixed by adding some extra security checks somewhere. You can choose what you want to do if only some links fail the security check.)

>+    while (tmp = linkList.pop())
>+      openNewWindowWith(tmp, this.ownerDocument);
Opening a separate window for each link doesn't seem to make sense.

>   <menupopup id="picontext">
>+  	<menuitem id="menu_openInTab"
>+  		        label="&openInTab.label;"
>+  		        command="cmd_openInTab"
>+  		        accesskey="&openInTab.accesskey;"/>
Nit: wrong indentation.
Comment 3 Edmund Wong (:ewong) 2012-05-08 08:01:56 PDT
Created attachment 621979 [details] [diff] [review]
Open 'Links' URLs in browser. (v2)
Comment 4 neil@parkwaycc.co.uk 2012-05-08 08:46:15 PDT
Comment on attachment 621979 [details] [diff] [review]
Open 'Links' URLs in browser. (v2)

Looks good from a quick skim, haven't tried it out yet. I see you added the security check when getting the list of links. This means that if the selected link is insecure, then nothing might happen. I guess we should follow up to disable the menuitems in this case. (I did wonder about combining the link opening functions into a single function and moving the security check there, but even then you run into the problem of having an insecure link selected.)

>+function getList()
Nit: How about calling this getSelectedLinks?

>+  if (linkList.length > 0)
Nit: don't need to compare > 0, if (linkList.length) should suffice. (x2)

>+  {
>+    openUILinkArrayIn(linkList, 'tab');
>+  }
Nit: file style is not to use {}s for a single line. (x2)

>+    openUILinkArrayIn(linkList, 'window');
[If the functions hadn't been so small, I would have suggested combining them into one function with a parameter which we forward to openUILinkArrayIn.]
Comment 5 Edmund Wong (:ewong) 2012-05-10 21:41:34 PDT
(In reply to neil@parkwaycc.co.uk from comment #4)
> 
> >+    openUILinkArrayIn(linkList, 'window');
> [If the functions hadn't been so small, I would have suggested combining
> them into one function with a parameter which we forward to
> openUILinkArrayIn.]

Both onOpenInWindowClick() and onOpenInTabClick() have pretty much the
same content, just a different parameter for openUILinkArrayIn().  Would
it be actually better to just do a function like:

  function onOpenIn(mode)
  {
    var linkList = getSelectedLinks();

    if (linkList.length)
      openUILinkArrayIn(linkList, mode);
  }

then just call the function in the xul file and use parameters like you mentioned?
Comment 6 neil@parkwaycc.co.uk 2012-05-11 14:28:15 PDT
(In reply to Edmund Wong from comment #5)
> Both onOpenInWindowClick() and onOpenInTabClick() have pretty much the
> same content
Given that they have three lines of code each, then you're calling two identical lines "pretty much the same" ;-)
Comment 7 neil@parkwaycc.co.uk 2012-05-11 14:32:44 PDT
Comment on attachment 621979 [details] [diff] [review]
Open 'Links' URLs in browser. (v2)

>+<!ENTITY  openInTab.label        "Open in Tab">
>+<!ENTITY  openInTab.accesskey    "T">
>+<!ENTITY  openInWindow.label     "Open in Window">
>+<!ENTITY  openInWindow.accesskey "W">
These should say New Tab/Window. r=me with this and my other nits fixed.
Comment 8 Edmund Wong (:ewong) 2012-05-11 20:44:20 PDT
Created attachment 623385 [details] [diff] [review]
Open 'Links' URLs in browser (v3)

requesting review on the js code.
Comment 9 neil@parkwaycc.co.uk 2012-05-12 08:19:04 PDT
Comment on attachment 623385 [details] [diff] [review]
Open 'Links' URLs in browser (v3)

I've just discovered a major problem with these patches...

All the trees currently use the same context menu, so you see the extra menuitems on e.g. the Meta and Fields trees where they make no sense.

Worse, it breaks the "copy" command on those trees, because you added a security check, but we're not copying links...
Comment 10 Edmund Wong (:ewong) 2012-05-12 19:46:56 PDT
Created attachment 623471 [details] [diff] [review]
Open 'Links' URLs in browser. (v4)
Comment 11 neil@parkwaycc.co.uk 2012-05-13 02:36:41 PDT
Comment on attachment 623471 [details] [diff] [review]
Open 'Links' URLs in browser. (v4)

>   <menupopup id="picontext">
>-    <menuitem id="menu_selectall" label="&selectall.label;" command="cmd_selectall" accesskey="&selectall.accesskey;"/>
>-    <menuitem id="menu_copy"      label="&copy.label;"      command="cmd_copy"      accesskey="&copy.accesskey;"/>
>+    <menuitem id="menu_selectall"
>+              label="&selectall.label;"
>+              command="cmd_selectall"
>+              accesskey="&selectall.accesskey;"/>
>+    <menuitem id="menu_copy"
>+              label="&copy.label;"
>+              command="cmd_copy"
>+              accesskey="&copy.accesskey;"/>
>+  </menupopup>
You don't need this change any more.

>+    <menuitem id="menu_selectall"
...
>+    <menuitem id="menu_copy"
Duplicate ids. r=me with that fixed.

>+              label="&copy.label;"
>+              command="cmd_copyLinks"
Hmm, I wonder whether we should give this menuitem a different label...
Comment 12 Edmund Wong (:ewong) 2012-05-13 19:23:27 PDT
(In reply to neil@parkwaycc.co.uk from comment #11)
> Comment on attachment 623471 [details] [diff] [review]
> Open 'Links' URLs in browser. (v4)
> 
> >   <menupopup id="picontext">
> >-    <menuitem id="menu_selectall" label="&selectall.label;" command="cmd_selectall" accesskey="&selectall.accesskey;"/>
> >-    <menuitem id="menu_copy"      label="&copy.label;"      command="cmd_copy"      accesskey="&copy.accesskey;"/>
> >+    <menuitem id="menu_selectall"
> >+              label="&selectall.label;"
> >+              command="cmd_selectall"
> >+              accesskey="&selectall.accesskey;"/>
> >+    <menuitem id="menu_copy"
> >+              label="&copy.label;"
> >+              command="cmd_copy"
> >+              accesskey="&copy.accesskey;"/>
> >+  </menupopup>
> You don't need this change any more.

Reverted.

> 
> >+    <menuitem id="menu_selectall"
> ...
> >+    <menuitem id="menu_copy"
> Duplicate ids. r=me with that fixed.
> 
> >+              label="&copy.label;"
> >+              command="cmd_copyLinks"
> Hmm, I wonder whether we should give this menuitem a different label...

Fixed with some additional code to determine whether the user selected
one link or more than one link and would change the popup menu item
to use the plural version.   So re-requesting review on this change.
Comment 13 Edmund Wong (:ewong) 2012-05-13 19:24:35 PDT
Created attachment 623564 [details] [diff] [review]
Open 'Links' URLs in browser (v5)
Comment 14 Edmund Wong (:ewong) 2012-05-13 19:33:31 PDT
Created attachment 623567 [details] [diff] [review]
Open 'Links' URLs in browser (v5)
Comment 15 neil@parkwaycc.co.uk 2012-05-14 01:29:41 PDT
Comment on attachment 623567 [details] [diff] [review]
Open 'Links' URLs in browser (v5)

>+function changeLinksLabel()
>+{
>+	var linkLabel = document.getElementById("menu_copy_links");
>+	var itemList = getSelectedItems(false);
>+
>+  if (itemList.length > 1)
>+    linkLabel.label = gBundle.getString("linkCopyLinks");
This isn't the correct way to do plurals (you should use PluralForm.jsm) but it just goes to show that my comment isn't as throwaway as I'd hoped. Let's just forget that I ever mentioned it, and go back to the original label.

>+    <menuitem id="menu_selectall"
Still duplicates an existing id.

And of course there's the tabs ;-)
Comment 16 Edmund Wong (:ewong) 2012-05-15 21:59:19 PDT
Created attachment 624297 [details] [diff] [review]
Open 'Links' URLs in browser (v6)
Comment 17 Edmund Wong (:ewong) 2012-05-15 22:05:33 PDT
Created attachment 624298 [details] [diff] [review]
Open 'Links' URLs in browser (v6)
Comment 18 neil@parkwaycc.co.uk 2012-05-16 05:20:37 PDT
Comment on attachment 624298 [details] [diff] [review]
Open 'Links' URLs in browser (v6)

>+    <menuitem id="menu_copy_links"
>+              label="&copyLinks.label;"
>+              command="cmd_copyLinks"
Nit: could use id=menu_copyLinks to match cmd_copyLinks perhaps?

>+<!ENTITY  copyLinks.key             "o">
Nit: unused. r=me with this removed.

>+<!ENTITY  copyLinks.label           "Copy Link">
Maybe we should use "Copy Link(s)" here. (I've done it before, and as yet, none of the localisers have screamed at me...)

>+<!ENTITY  copyLinks.accesskey       "o">
Nit: Wouldn't "L" or maybe "C" be better?
Comment 19 Edmund Wong (:ewong) 2012-05-20 03:45:53 PDT
Created attachment 625483 [details] [diff] [review]
Open 'Links' URLs in browser. (v7)
Comment 20 Edmund Wong (:ewong) 2012-05-25 07:48:39 PDT
Pushed to comm-central:

http://hg.mozilla.org/comm-central/rev/26efbc349918

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