Tooltips for sources contain the group URL instead of the file URL

VERIFIED FIXED in Firefox 30

Status

()

Firefox
Developer Tools: Debugger
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: past, Assigned: kushagra)

Tracking

Trunk
Firefox 30
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27 unaffected, firefox28 unaffected, firefox29 affected, firefox30 verified)

Details

(Whiteboard: [good first bug][lang=js][mentor=vporof])

Attachments

(3 attachments)

STR:
1) Open http://htmlpad.org/debugger
2) Hover over the file names in the source list

It might be a regression from bug 951633, but I don't know for sure.
This is easy to fix!
Whiteboard: [good first bug][lang=js][mentor=vporof]
(Assignee)

Comment 2

4 years ago
Hi :) I am completely new at this and eager to contribute. Could you please guide me through this bug?

Comment 3

4 years ago
Hi Victor porof, I have gone through the devetools-Debugger- Sources tab. When hover it doesn't shows file url. So i am interested in bug can you please assign it to me. I will work on this Bug.
Hey guys! I didn't see your messages in the bug. Since Kushagra asked first, I'm going to assign this to him. In the future, please use the "Need more information from..." to make sure I don't miss these bugs :)

What you'll need to do first is remove the tooltip text automatically set on groups and sources here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/SideMenuWidget.jsm#426

Secondly, set the correct tooltip in here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#127

let unicodeUrl = NetworkHelper.convertToUnicode(unescape(url));
contents.setAttribute("tooltiptext", unicodeUrl);

Third thing is to add some assertions in a test, to make sure we don't regress this in the future. This looks like a good place: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_scripts-switching-01.js

In testSourcesDisplay add the following assertions:

is(gSources.items[0].target.getAttribute("tooltiptext"), "...",
  "The correct tooltip text is displayed for the first source.");
is(gSources.items[1].target.getAttribute("tooltiptext"), "...",
  "The correct tooltip text is displayed for the second source.");

That's it.
Assignee: nobody → singh.kushagra93
Status: NEW → ASSIGNED
Flags: needinfo?(singh.kushagra93)
(Assignee)

Comment 5

4 years ago
thanks victor. will get on it right away.
Flags: needinfo?(singh.kushagra93)
(Assignee)

Comment 6

4 years ago
thanks victor. will get on it right away.
Great! Thank you!
(Assignee)

Comment 8

4 years ago
Created attachment 8369477 [details] [diff] [review]
bug962070_debuggerfileURLfix.diff
Attachment #8369477 - Flags: review?(vporof)
Flags: needinfo?(vporof)
Comment on attachment 8369477 [details] [diff] [review]
bug962070_debuggerfileURLfix.diff

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

Looks good! Please add a test as described in comment 4.
Attachment #8369477 - Flags: review?(vporof) → feedback+
Flags: needinfo?(vporof)
(Assignee)

Comment 10

4 years ago
oh sorry.. i forgot to save the file that's why it didnt get included in the patch.
(Assignee)

Comment 11

4 years ago
Created attachment 8369490 [details] [diff] [review]
bug962070_debuggerfileURLfix.diff
Attachment #8369490 - Flags: review?(vporof)
Comment on attachment 8369490 [details] [diff] [review]
bug962070_debuggerfileURLfix.diff

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

No worries. See below.

::: browser/devtools/debugger/test/browser_dbg_scripts-switching-01.js
@@ +46,5 @@
>      "Found the expected number of sources.");
>  
> +  is(gSources.items[0].target.getAttribute("tooltiptext"), "...",
> +    "The correct tooltip text is displayed for the first source.");
> +  is(gSources.items[1].target.getAttribute("tooltiptext"), "...",

Well, this will obviously fail. Did you try running this test?
Add the expected values here please.
Attachment #8369490 - Flags: review?(vporof) → feedback+
(Assignee)

Comment 13

4 years ago
i tried many things and finally this fixed it:-

  is(gSources.items[0].target.getAttribute("tooltiptext"),"",
    "The correct tooltip text is displayed for the first source.");
  is(gSources.items[1].target.getAttribute("tooltiptext"),"",
    "The correct tooltip text is displayed for the second source.");

but another test has been failing since the beginning(even after removing the new tests):-
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_scripts-switching-01.js | The first source is displayed. - Got 117, expected 118

what should i do about it?
Flags: needinfo?(vporof)
(In reply to Kushagra Singh [:kushagra] from comment #13)
> i tried many things and finally this fixed it:-
> 
>   is(gSources.items[0].target.getAttribute("tooltiptext"),"",
>     "The correct tooltip text is displayed for the first source.");
>   is(gSources.items[1].target.getAttribute("tooltiptext"),"",
>     "The correct tooltip text is displayed for the second source.");
> 

That doesn't look correct, it implies that the tooltips is empty! That's the opposite of what we want :) I'll take a closer look soon.
Flags: needinfo?(vporof)
Ok, those two lines in the test should be

  is(gSources.items[0].target.querySelector(".dbg-source-item").getAttribute("tooltiptext"),
    EXAMPLE_URL + "code_script-switching-01.js",
    "The correct tooltip text is displayed for the first source.");
  is(gSources.items[1].target.querySelector(".dbg-source-item").getAttribute("tooltiptext"),
    EXAMPLE_URL + "code_script-switching-02.js",
    "The correct tooltip text is displayed for the second source.");

All other tests work for me locally. Please make the above change and ask me again for review. I'll push to try and land everything if it's green.
(Assignee)

Comment 16

4 years ago
Created attachment 8369788 [details] [diff] [review]
bug962070_debuggerfileURLfix.diff
Attachment #8369788 - Flags: review?(vporof)
(Assignee)

Comment 17

4 years ago
i am still getting that failed test. i think it might be a problem with my local machine. i am running this test:

./mach mochitest-browser browser/devtools/debugger/test/browser_dbg_scripts-switching-01.js
Flags: needinfo?(vporof)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=22b24e0a73f0
Flags: needinfo?(vporof)
Comment on attachment 8369788 [details] [diff] [review]
bug962070_debuggerfileURLfix.diff

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

Try looks good. I'll land this.
Attachment #8369788 - Flags: review?(vporof) → review+
(Assignee)

Comment 20

4 years ago
Amazing!! thanks victor for your help and patience :)
https://hg.mozilla.org/mozilla-central/rev/a20dde0cabcf
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(Reporter)

Comment 22

4 years ago
This needs to be uplifted.
status-firefox27: --- → unaffected
status-firefox28: --- → unaffected
status-firefox29: --- → affected
status-firefox30: --- → fixed

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified disabled

Updated

4 years ago
status-firefox30: verified disabled → verified
You need to log in before you can comment on or make changes to this bug.