Closed Bug 952806 Opened 10 years ago Closed 5 years ago

No way to copy the url of the script the error is in

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED DUPLICATE of bug 1457111

People

(Reporter: bzbarsky, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

STEPS TO REPRODUCE:

1)  Load a web page that includes a script throws an exception with the web
    console open.
2)  Try to copy the URL to that script

ACTUAL RESULTS: hovering over that link will show the url as a tooltip, but there is no way to copy it.  In particular, the context menu for that link is not an actual link context menu.

EXPECTED RESULTS: Have a way to copy the URL.
I confirm this problem on Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mentor: mihai.sucan
Priority: -- → P3
Hi, I would like to work on this bug, is that ok? I am new in contributing to open source so hope that everything will go well.
Hello Marek!

Thanks for your interest in fixing this bug. I'm happy to help you with contributing a patch.

Overview of the workflow:

1. Get the firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Please make sure you ask for feedback from me. Set the patch flag feedback? and write my nick, :msucan.

If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but sometimes better.

http://dxr.mozilla.org


Here's an overview of how you can fix this bug:

Load browser/devtools/webconsole/webconsole.js in your editor - this is the implementation of the web console UI. Search for ConsoleContextMenu and please check how it currently works. You will see that 'open link in new tab' and 'copy link' only shows for network requests. You need to change the code such that these two menu items are also displayed when the user right clicks the message source link that shows on the right side. Update the code such that the two menu items work as expected when they are used.

Search for WCF_createLocationNode to see how the location dom element is displayed on the right side of the webconsole output.



If you have any questions, please ask me here. Use the needinfo flag and write my nick, :msucan.

Thank you very much.
[Blocking Requested - why for this release]:
Flags: needinfo?(mihai.sucan)
Flags: needinfo?(mihai.sucan) → needinfo?
Flags: needinfo?
I have already fixed the problem but want to ask you if "Copy Link Location" and "Open URL in New Tab" menu items should be showed also for CSS and Security logs. And if so, should it appear only if there is a URL included? For instance, CSS reflows have no URL so "Open URL in New Tab" menu item does nothing. To hide it ,in this case, I would have to add a new classification or hide these two items namely in 'shouldHideMenuItem' function.
Flags: needinfo?(mihai.sucan)
(In reply to marek.strelec from comment #5)
> I have already fixed the problem but want to ask you if "Copy Link Location"
> and "Open URL in New Tab" menu items should be showed also for CSS and
> Security logs. And if so, should it appear only if there is a URL included?

Yes, that is correct. Show the menu items for every message that has a URL. Otherwise just hide the menu items.

Thanks!
Flags: needinfo?(mihai.sucan)
Attached patch 952806.patch (obsolete) — Splinter Review
Attachment #8468151 - Flags: review?(mihai.sucan)
Comment on attachment 8468151 [details] [diff] [review]
952806.patch

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

This is a good start. Thanks! General comments:

- the menu items do not show for security messages - they should.

- the menu items do not show for window.console API messages - they should. Try console.log('foo') from a page. To fix this also look into console-output.js.

- please remove any trailing whitespaces added by the patch.

::: browser/devtools/webconsole/webconsole.js
@@ +5271,5 @@
>      metadata.selectionType = selectedItems.length > 1 ? "multiple" : "single";
>  
>      let selection = metadata.selection;
>      for (let item of selectedItems) {
> +      metadata.url = !(typeof (item.url) === 'undefined' || item.url === null || item.url == '');

You can simply do metadata.url = !!item.url.

@@ +5313,5 @@
>        return false;
>      }
>  
> +    let command = aMenuItem.getAttribute("command");
> +    if((command == "consoleCmd_openURL" || command == "consoleCmd_copyURL") && !aMetadata.url)

nit: please follow the coding style of this file. Add space after |if| and use curly braces even if there's only one statement in the code block.

if (foo) {
  foobar();
}
Attachment #8468151 - Flags: review?(mihai.sucan)
Attached patch 952806-2.patch (obsolete) — Splinter Review
Thanks for your help! I had only problem with security errors. I don't know how to simulate this state and get security errors in console so I wasn't able to test it.
Attachment #8469833 - Flags: review?(mihai.sucan)
Comment on attachment 8469833 [details] [diff] [review]
952806-2.patch

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

Hey Marek.

Sorry for the delay here, but I'll be taking this review. Hope to get at it this weekend or Monday at earliest.

thanks.
Attachment #8469833 - Flags: review?(mihai.sucan) → review?(rcampbell)
Marking this bug as assigned.

While you're waiting, we're going to need to test this as well. Check out the devtools/browser/webconsole/test directory for examples. If you can find something related to url copying, feel free to modify the test file to check for these new conditions as well.
Assignee: nobody → marek.strelec
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8469833 [details] [diff] [review]
952806-2.patch

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

I think the original intent of this was to provide a Copy URL right-click context menu item on the error's location rather than relying on having to select it first.

Canceling review for now. Care to make another attempt?

::: browser/devtools/webconsole/console-output.js
@@ +493,5 @@
>      container.setAttribute("category", this._categoryNameCompat);
>      container.setAttribute("severity", this._severityNameCompat);
>      container.setAttribute("filter", this._filterKeyCompat);
>      container.clipboardText = this.textContent;
> +    container.url = this.location.url;

does this always exist?

::: browser/devtools/webconsole/webconsole.js
@@ +5287,5 @@
>            selection.add("webdev");
>            break;
> +        case CATEGORY_SECURITY:
> +          selection.add("security");
> +          break;

this is good. Probably overlooked when the Security category was added.

@@ +5326,5 @@
>        // check whether this menu item should show or not.
>        if (itemData.indexOf(type) !== -1) {
>          shouldHide = false;
>          break;
>        }

If we took your patch as-is, I don't think we'd need this selection logic anymore.

I think since we have all the categories now, this check will be pointless and shouldHide will always be false. The only real check we need is whether the selection metadata has a url or not. This block becomes | return false; |
Attachment #8469833 - Flags: review?(rcampbell)
Attachment #8468151 - Attachment is obsolete: true
whups. I tested this without applying your patch first. :)

Looks like the copy link location menu item does show up without a selection, so that's good.

It doesn't appear on links in Security categories. One example I found was navigating to a non-https page with a password field on it. E.g., http://reddit.com without being logged in.

"Password fields present on an insecure (http://) page. This is a security risk that allows user login credentials to be stolen.[Learn More]"

the "Learn More" link isn't copyable until you select it.

Still, this patch is working better than I initially thought. I'd still like to see if we can get rid of that loop in shouldHideMenuItem().
Hi, sorry for the late reply (just back from vacation). I've already fixed the problem with security errors - thanks for the link you gave me. I'm still having few questions here though:

Can you please specify the removing of the selection logic?
- do you mean that shouldHideMenuItem will only check whether the selection metadata has a url or not 
and everything else that includes 'selection' and 'selectionType' will be removed? (e.g. in webconsole.xul and in getSelectionMetadata method)

also, is there any interface for tests in devtools/browser/webconsole/test directory? If I make a change in a test file, should it be submitted as a separated patch file or included in the final patch?

Thanks
Panos, maybe you can advise here since Rob and Mihai aren't available?
Flags: needinfo?(past)
> ::: browser/devtools/webconsole/console-output.js
> @@ +493,5 @@
> >      container.setAttribute("category", this._categoryNameCompat);
> >      container.setAttribute("severity", this._severityNameCompat);
> >      container.setAttribute("filter", this._filterKeyCompat);
> >      container.clipboardText = this.textContent;
> > +    container.url = this.location.url;
> 
> does this always exist?

Nope. Loading the reddit.com page displays an error in the terminal:

System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js:513 - TypeError: this.location is undefined

Just use:

container.url = this.location && this.location.url;

(In reply to marek.strelec from comment #15)
> Can you please specify the removing of the selection logic?
> - do you mean that shouldHideMenuItem will only check whether the selection
> metadata has a url or not 
> and everything else that includes 'selection' and 'selectionType' will be
> removed? (e.g. in webconsole.xul and in getSelectionMetadata method)

I think Rob just wanted the replacement of all the code in shouldHideMenuItem below the "let shouldHide = true;" line with a single "return false;". You cannot remove the entire selection/selectionType machinery, because we still need to hide menu items that only work when a single message is selected (which depends on the selectionType attribute).

You *could* remove the selection attribute from the markup (since we now want this menu item to appear in every category), but you would have to change the order of checks in shouldHideMenuItem, otherwise messages without URLs wouldn't hide these menu items.

However, what I think is a more future-proof solution than either of the above, is to leave the loop block in there, but use selection="any" in webconsole.xul and add an explicit test for that value before the loop block. That would allow the console to get category-specific menu items in the future if necessary, but wouldn't tax the current code with a useless loop. That's what the places code does already:

http://dxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#518

> also, is there any interface for tests in devtools/browser/webconsole/test
> directory? If I make a change in a test file, should it be submitted as a
> separated patch file or included in the final patch?

Use a single patch for both code and tests. To run the webconsole frontend tests (the only ones of interest here) use;

./mach mochitest-devtools browser/devtools/webconsole

I'm sure you will get at least one failure with this patch in browser_bug_638949_copy_link_location.js and maybe others. You have to fix those failures and in the case of this specific test ensure that every message has these menu items present.

Let me know if you need more help.
Flags: needinfo?(past)
Thanks a lot, this should help. I will post a patch asap.
Attached patch 952806-4.patchSplinter Review
Attachment #8469833 - Attachment is obsolete: true
Attachment #8494266 - Flags: review?(past)
Comment on attachment 8494266 [details] [diff] [review]
952806-4.patch

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

I'm still getting test failures, did you try running the webconsole tests locally as I suggested?

::: browser/devtools/webconsole/test/test-console.html
@@ +27,4 @@
>      <h1 id="header">Heads Up Display Demo</h1>
>      <button onclick="test();">Log stuff about Dolske</button>
> +    <div id="myDiv">
> +    

Nit: trailing whitespace.

::: browser/devtools/webconsole/webconsole.js
@@ +5321,3 @@
>        return false;
>      }
> +  

Nit: trailing whitespace.
Attachment #8494266 - Flags: review?(past) → feedback+
I was testing browser_bug_638949_copy_link_location.js and didn't get any errors after my patch.

What test failures are you getting?
Flags: needinfo?(past)
There is a large number of webconsole tests that you need to check. Use the following command:

./mach mochitest-devtools browser/devtools/webconsole

There were 2 tests that were failing, which I can't remember now.
Flags: needinfo?(past)
Is bug 848502 a dupe of this or vice versa?

I look forward to this feature, hope it also handles URLs to CSS files and such.
(In reply to Hallvord R. M. Steen [:hallvors] from comment #24)
> Is bug 848502 a dupe of this or vice versa?

The web console doesn't use the SideMenuWidget, so the bugs are independent.

Marek, are you still working on this, or can someone else look into it?
Flags: needinfo?(marek.strelec)
Assignee: marek.strelec → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(marek.strelec)
Mentor: mihai.sucan → past
Mentor: past
Product: Firefox → DevTools
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: