Closed Bug 962070 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

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

VERIFIED FIXED
Firefox 30
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 --- affected
firefox30 --- verified

People

(Reporter: past, Assigned: kushagra)

Details

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

Attachments

(3 files)

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]
Hi :) I am completely new at this and eager to contribute. Could you please guide me through this bug?
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)
thanks victor. will get on it right away.
Flags: needinfo?(singh.kushagra93)
thanks victor. will get on it right away.
Great! Thank you!
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)
oh sorry.. i forgot to save the file that's why it didnt get included in the patch.
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+
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.
Attachment #8369788 - Flags: review?(vporof)
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)
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+
Amazing!! thanks victor for your help and patience :)
https://hg.mozilla.org/mozilla-central/rev/a20dde0cabcf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.