Last Comment Bug 469434 - links in "view source" should have "copy link location" in context menu
: links in "view source" should have "copy link location" in context menu
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: View Source (show other bugs)
: Trunk
: All All
: -- enhancement with 8 votes (vote)
: mozilla13
Assigned To: Geoff Lankow (:darktrojan)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
: 522321 560789 561612 645780 687799 694711 (view as bug list)
Depends on: 702448 728655
Blocks: 680651
  Show dependency treegraph
 
Reported: 2008-12-12 17:31 PST by Ori Avtalion (salty-horse)
Modified: 2012-03-12 04:16 PDT (History)
24 users (show)
geoff: in‑testsuite+
geoff: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, part1 (10.65 KB, patch)
2009-10-15 18:07 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
patch, part2 nsContextMenu.js file (8.67 KB, patch)
2009-10-15 18:08 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
new patch (7.76 KB, patch)
2011-11-08 03:39 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch, v2 (8.02 KB, patch)
2011-11-09 04:00 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch, v3 (7.37 KB, patch)
2011-12-02 14:26 PST, Geoff Lankow (:darktrojan)
dao+bmo: review-
Details | Diff | Splinter Review
patch v4 (15.28 KB, patch)
2011-12-19 14:06 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v5 (10.45 KB, patch)
2012-02-19 01:48 PST, Geoff Lankow (:darktrojan)
dao+bmo: review+
Details | Diff | Splinter Review

Description Ori Avtalion (salty-horse) 2008-12-12 17:31:22 PST
Right clicking on links should have a "Copy link location" item in the
context menu, just like the "regular" browser window.

Since this feature is missing, copying links is now very hard. I have to be careful to click and drag from one edge of the link to the other, while being careful not to trigger the link by pressing it directly.
Comment 1 Ori Avtalion (salty-horse) 2008-12-12 17:46:23 PST
This might relate to bug 304081
Comment 2 Robert Accettura [:raccettura] 2009-06-27 15:41:37 PDT
I've been realizing the same thing.  I've been right clicking on URL's and expecting to see the copy link location functionality.

cc'ing cbartley, who tackled bug 17612.  Doesn't seem like many others have given view-source much love in the past several years.
Comment 3 Ria Klaassen (not reading all bugmail) 2009-10-14 14:33:13 PDT
*** Bug 522321 has been marked as a duplicate of this bug. ***
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-10-15 18:07:28 PDT
Created attachment 406604 [details] [diff] [review]
patch, part1

This is what I have, it seems to work well. Perhaps there should also be a "Save Link As.."?
I tried so hard to get one patch, but for some reason, that is impossible with my tree, so that's why I'm posting it as two parts.
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-10-15 18:08:08 PDT
Created attachment 406605 [details] [diff] [review]
patch, part2 nsContextMenu.js file
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-10-16 01:25:21 PDT
We probably don't want to have the view-source: protocol added, so the goDoCommand('cmd_copyLink'); thing might have to be changed in a copyLink function that removes the view-source: protocol or something.
Comment 7 Curtis Bartley [:cbartley] 2009-10-16 10:30:30 PDT
(In reply to comment #5)
> Created an attachment (id=406605) [details]
> patch, part2 nsContextMenu.js file

Martijn: Did you write the code in nsContextMenu.prototype.setTarget, or is that just copied from elsewhere?  Either way, that function would really benefit from being factored out into multiple functions.  It's not that the function is all that long, it just feels like the the nesting is excessively deep:

  while {
    if {
      if {
        while {
          try {
            if {
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-10-17 08:26:43 PDT
Yes, I copied it from another nsContextMenu.js. Indeed, there is a lot of stuff that could probably be made simpler in there, still.
Comment 9 Curtis Bartley [:cbartley] 2009-10-17 11:16:20 PDT
(In reply to comment #8)
> Yes, I copied it from another nsContextMenu.js. Indeed, there is a lot of stuff
> that could probably be made simpler in there, still.

It kind of had that look :-).

I think it's probably reasonable to dispense with the separate nsContextMenu.js file and just incorporate that code directly into viewSource.js.  If you want to keep the separate file, you should probably rename it to viewSourceContextMenu.js.

I'll try this patch out later today, and get back to you with some more substantive feedback.
Comment 10 Ria Klaassen (not reading all bugmail) 2010-04-21 12:02:26 PDT
*** Bug 560789 has been marked as a duplicate of this bug. ***
Comment 11 Jo Hermans 2010-04-25 02:37:47 PDT
*** Bug 561612 has been marked as a duplicate of this bug. ***
Comment 12 Christian Ascheberg 2011-04-27 15:41:10 PDT
mark bug 645780 as duplicate?
Comment 13 Vitaliy 2011-04-27 20:13:48 PDT
So any news about this? The patch was released two years ago, and still no implementation.
Comment 14 Jo Hermans 2011-08-20 04:38:25 PDT
*** Bug 645780 has been marked as a duplicate of this bug. ***
Comment 15 Jo Hermans 2011-09-20 03:53:00 PDT
*** Bug 687799 has been marked as a duplicate of this bug. ***
Comment 16 Matthias Versen [:Matti] 2011-10-14 19:54:59 PDT
*** Bug 694711 has been marked as a duplicate of this bug. ***
Comment 17 Geoff Lankow (:darktrojan) 2011-11-08 03:39:42 PST
Created attachment 572775 [details] [diff] [review]
new patch
Comment 18 Olli Pettay [:smaug] 2011-11-08 03:43:47 PST
Comment on attachment 572775 [details] [diff] [review]
new patch

Some toolkit peer should review this. dao or gavin perhaps
Comment 19 Geoff Lankow (:darktrojan) 2011-11-09 04:00:56 PST
Created attachment 573155 [details] [diff] [review]
patch, v2

My original test fails most of the time on try. This new version seems to pass on all the platforms (some haven't finished yet): https://tbpl.mozilla.org/?tree=Try&rev=69a908e2cd7a
Comment 20 Geoff Lankow (:darktrojan) 2011-12-02 14:26:52 PST
Created attachment 578727 [details] [diff] [review]
patch, v3

This is the same patch as the previous one, updated to go with the new view source tests.
Comment 21 Dão Gottwald [:dao] 2011-12-15 11:22:33 PST
Comment on attachment 578727 [details] [diff] [review]
patch, v3

You need to update viewPartialSource.xul as well.

Also replace the deprecated document.popupNode with popup.triggerNode.

nit: let those function names start with a small letter
Comment 22 Geoff Lankow (:darktrojan) 2011-12-19 14:06:36 PST
Created attachment 582949 [details] [diff] [review]
patch v4

I ran into bug 669845 while testing the partial source window, so I've disabled that test on debug.
Comment 23 Geoff Lankow (:darktrojan) 2012-02-09 04:04:35 PST
Dão, can I get an ETA on this review? Thanks.
Comment 24 Geoff Lankow (:darktrojan) 2012-02-19 01:48:49 PST
Created attachment 598636 [details] [diff] [review]
patch v5

I've rewritten the tests for partial view source using the functions created in bug 728655.
Comment 25 Dão Gottwald [:dao] 2012-02-22 08:39:20 PST
Comment on attachment 598636 [details] [diff] [review]
patch v5

>+      if (partialTestRunning)
>+        return finish();

      if (partialTestRunning) {
        finish();
        return;
      }
Comment 26 Geoff Lankow (:darktrojan) 2012-02-22 20:46:14 PST
On inbound with that nit fixed and also bug 728655 comment 20, which slipped through the cracks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8fc342348b
Comment 27 Phil Ringnalda (:philor) 2012-02-22 22:14:19 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/467940965809 - so far, OS X 10.5 and 10.6 debug have timed out, "browser_contextmenu.js | application timed out after 330 seconds with no output," which might wind up meaning it's broken on OS X, or very well might mean it's broken on the slow things, and will eventually time out on WinXP too.
Comment 28 Phil Ringnalda (:philor) 2012-02-22 22:31:02 PST
Win7 was a bit more interesting in https://tbpl.mozilla.org/php/getParsedLog.php?id=9552718&tree=Mozilla-Inbound&full=1 detecting a deadlock in nsWindowMediator.mListLock and taking down the browser over it.
Comment 29 Geoff Lankow (:darktrojan) 2012-02-23 04:59:34 PST
Took out the offending waitForFocus call, checked on Try server, and relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c99485521cd
Comment 30 Richard Newman [:rnewman] 2012-02-23 18:53:11 PST
https://hg.mozilla.org/mozilla-central/rev/5c99485521cd
Comment 31 neil@parkwaycc.co.uk 2012-03-12 04:16:37 PDT
Comment on attachment 598636 [details] [diff] [review]
patch v5

>+    if (gContextMenu.triggerNode.href.indexOf('view-source:') == 0)
For a known string, /^view-source:/.test is probably our best version of startsWith. (For an unknown string, .lastIndexOf(needle, 0) == 0 works best.)

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