Closed Bug 469434 Opened 12 years ago Closed 9 years ago

links in "view source" should have "copy link location" in context menu

Categories

(Toolkit :: View Source, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: ori, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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.
This might relate to bug 304081
Blocks: FF2SM
Hardware: x86 → All
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.
Duplicate of this bug: 522321
Attached patch patch, part1 (obsolete) — Splinter Review
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.
Attachment #406604 - Attachment is patch: true
Attachment #406604 - Attachment mime type: application/octet-stream → text/plain
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.
(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 {
Yes, I copied it from another nsContextMenu.js. Indeed, there is a lot of stuff that could probably be made simpler in there, still.
(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.
Duplicate of this bug: 560789
Duplicate of this bug: 561612
mark bug 645780 as duplicate?
So any news about this? The patch was released two years ago, and still no implementation.
Duplicate of this bug: 645780
Depends on: 680651
Blocks: 680651
No longer depends on: 680651
Duplicate of this bug: 687799
Duplicate of this bug: 694711
Assignee: nobody → geoff
Attached patch new patch (obsolete) — Splinter Review
Attachment #406604 - Attachment is obsolete: true
Attachment #406605 - Attachment is obsolete: true
Attachment #572775 - Flags: review?(bugs)
Comment on attachment 572775 [details] [diff] [review]
new patch

Some toolkit peer should review this. dao or gavin perhaps
Attachment #572775 - Flags: review?(bugs) → review?(dao)
Attached patch patch, v2 (obsolete) — Splinter Review
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
Attachment #572775 - Attachment is obsolete: true
Attachment #573155 - Flags: review?(dao)
Attachment #572775 - Flags: review?(dao)
Depends on: 702448
Attached patch patch, v3 (obsolete) — Splinter Review
This is the same patch as the previous one, updated to go with the new view source tests.
Attachment #573155 - Attachment is obsolete: true
Attachment #578727 - Flags: review?(dao)
Attachment #573155 - Flags: review?(dao)
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
Attachment #578727 - Flags: review?(dao) → review-
Attached patch patch v4 (obsolete) — Splinter Review
I ran into bug 669845 while testing the partial source window, so I've disabled that test on debug.
Attachment #578727 - Attachment is obsolete: true
Attachment #582949 - Flags: review?(dao)
Dão, can I get an ETA on this review? Thanks.
Depends on: 728655
Attached patch patch v5Splinter Review
I've rewritten the tests for partial view source using the functions created in bug 728655.
Attachment #582949 - Attachment is obsolete: true
Attachment #598636 - Flags: review?(dao)
Attachment #582949 - Flags: review?(dao)
Comment on attachment 598636 [details] [diff] [review]
patch v5

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

      if (partialTestRunning) {
        finish();
        return;
      }
Attachment #598636 - Flags: review?(dao) → review+
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
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
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.
Target Milestone: mozilla13 → ---
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.
Took out the offending waitForFocus call, checked on Try server, and relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c99485521cd
https://hg.mozilla.org/mozilla-central/rev/5c99485521cd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
No longer blocks: FF2SM
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.)
You need to log in before you can comment on or make changes to this bug.