Closed
Bug 962070
Opened 10 years ago
Closed 10 years ago
Tooltips for sources contain the group URL instead of the file URL
Categories
(DevTools :: Debugger, defect)
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)
2.44 KB,
patch
|
vporof
:
feedback+
|
Details | Diff | Splinter Review |
3.60 KB,
patch
|
vporof
:
feedback+
|
Details | Diff | Splinter Review |
3.71 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•10 years ago
|
||
Hi :) I am completely new at this and eager to contribute. Could you please guide me through this bug?
Comment 3•10 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.
Comment 4•10 years ago
|
||
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•10 years ago
|
||
thanks victor. will get on it right away.
Flags: needinfo?(singh.kushagra93)
Assignee | ||
Comment 6•10 years ago
|
||
thanks victor. will get on it right away.
Comment 7•10 years ago
|
||
Great! Thank you!
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8369477 -
Flags: review?(vporof)
Flags: needinfo?(vporof)
Comment 9•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(vporof)
Assignee | ||
Comment 10•10 years ago
|
||
oh sorry.. i forgot to save the file that's why it didnt get included in the patch.
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8369490 -
Flags: review?(vporof)
Comment 12•10 years ago
|
||
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•10 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?
Updated•10 years ago
|
Flags: needinfo?(vporof)
Comment 14•10 years ago
|
||
(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)
Comment 15•10 years ago
|
||
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•10 years ago
|
||
Attachment #8369788 -
Flags: review?(vporof)
Assignee | ||
Comment 17•10 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)
Comment 18•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=22b24e0a73f0
Flags: needinfo?(vporof)
Comment 19•10 years ago
|
||
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•10 years ago
|
||
Amazing!! thanks victor for your help and patience :)
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a20dde0cabcf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Reporter | ||
Comment 22•10 years ago
|
||
This needs to be uplifted.
status-firefox27:
--- → unaffected
status-firefox28:
--- → unaffected
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•