Closed
Bug 927673
Opened 11 years ago
Closed 10 years ago
Don't say "This page has no sources" when the debugger is loading.
Categories
(DevTools :: Debugger, defect, P3)
DevTools
Debugger
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 30
People
(Reporter: harth, Assigned: b4bomsy)
References
Details
Attachments
(1 file, 5 obsolete files)
6.43 KB,
patch
|
b4bomsy
:
review+
|
Details | Diff | Splinter Review |
When loading the browser debugger in particular the debugger's left pane shows the message "This page has no sources". After a few seconds I think it's true and right as I'm about to close the debugger, a huge list of scripts appears. The text of this message is really misleading. The left pane should indicate that it's being loaded, and not that it couldn't find any sources.
Comment 1•11 years ago
|
||
Dupe of bug 883249?
Comment 2•11 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #1) > Dupe of bug 883249? I think we should fix both things. Maybe show a "Waiting for sources..." for a couple of seconds, then switch to "This page has no sources" if nothing is retrieved after the getSources() call. This should be ok for the browser debugger as well.
Assignee | ||
Comment 3•11 years ago
|
||
can i work on this?
Comment 4•11 years ago
|
||
(In reply to Hubert B Manilla from comment #3) > can i work on this? Are you still working on bug 913665?
Assignee | ||
Comment 5•11 years ago
|
||
Yes..just finishing up the tests. Will push the patch in the nextb day or so...
Comment 6•11 years ago
|
||
(In reply to Hubert B Manilla from comment #5) > Yes..just finishing up the tests. Will push the patch in the nextb day or > so... Awesome :)
Assignee: nobody → b4bomsy
Assignee | ||
Comment 7•10 years ago
|
||
Just want to make sure i'm on the right track.
Attachment #8350797 -
Flags: feedback?(vporof)
Attachment #8350797 -
Flags: feedback?(nfitzgerald)
Comment 8•10 years ago
|
||
Comment on attachment 8350797 [details] [diff] [review] 927673wip.patch Review of attachment 8350797 [details] [diff] [review]: ----------------------------------------------------------------- Pretty much on the right track, just see the comment below. ::: browser/devtools/debugger/debugger-controller.js @@ +1185,5 @@ > dumpn(msg); > return; > } > > + DebuggerView.Sources.emptyText = L10N.getStr("loadingSourcesText"); You'd want to do this when we first call |getSources|, not in the callback for handling the sources. Otherwise, it won't show at all because this is all happening on the same tick of the event loop.
Attachment #8350797 -
Flags: feedback?(nfitzgerald)
Comment 9•10 years ago
|
||
Comment on attachment 8350797 [details] [diff] [review] 927673wip.patch Review of attachment 8350797 [details] [diff] [review]: ----------------------------------------------------------------- What Nick said.
Attachment #8350797 -
Flags: feedback?(vporof) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8350797 -
Attachment is obsolete: true
Attachment #8362154 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8362154 [details] [diff] [review] Fix for indicating when sources are loading The patch works ok. Just a quick one about setting this.Sources.emptyText in the debugger-view.js. Is this an ok place to set it? i did this because "Waiting for Sources..." needs to show immediately the DebuggerView.TabNavigated is called rather than when the getSources() is called quite later (The period inbetween is visibly obvious).
Attachment #8362154 -
Flags: review?(nfitzgerald) → feedback?(nfitzgerald)
Assignee | ||
Updated•10 years ago
|
Attachment #8362154 -
Flags: feedback?(vporof)
Comment 13•10 years ago
|
||
Comment on attachment 8362154 [details] [diff] [review] Fix for indicating when sources are loading Review of attachment 8362154 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-view.js @@ +630,5 @@ > this.editor.setMode(Editor.modes.text); > this.editor.setText(""); > this.editor.clearHistory(); > this._editorSource = {}; > + this.Sources.emptyText = L10N.getStr("loadingSourcesText"); You should move this to SourceScripts.handleTabNavigation, so it's closer to _onSourcesAdded.
Attachment #8362154 -
Flags: feedback?(vporof) → feedback+
Comment 14•10 years ago
|
||
(In reply to Hubert B Manilla from comment #12) > Comment on attachment 8362154 [details] [diff] [review] > Fix for indicating when sources are loading > > The patch works ok. Just a quick one about setting this.Sources.emptyText in > the debugger-view.js. Is this an ok place to set it? i did this because > "Waiting for Sources..." needs to show immediately the > DebuggerView.TabNavigated is called rather than when the getSources() is > called quite later (The period inbetween is visibly obvious). You're right, I didn't read this before, sorry. Disregard comment 13. Yes, DebuggerView.handleTabNavigation is a better place to put it, to make sure the change happens immediately when the target starts navigating. However, move it outside the conditional, it has no reason to be in there.
Assignee | ||
Comment 15•10 years ago
|
||
Ok , Thanks victor.
Comment 16•10 years ago
|
||
Comment on attachment 8362154 [details] [diff] [review] Fix for indicating when sources are loading Victor's got it :)
Attachment #8362154 -
Flags: feedback?(nfitzgerald)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8362154 -
Attachment is obsolete: true
Attachment #8364213 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 18•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=38c168c1e23e
Comment 19•10 years ago
|
||
Comment on attachment 8364213 [details] [diff] [review] Fix for indicating when sources are loading Review of attachment 8364213 [details] [diff] [review]: ----------------------------------------------------------------- Try has been having load issues and recently was reset, so try pushes from before the reset have been funky. Can you do a new one? Once you have a good try push and nothing is breaking, and addressed the comment below, go ahead and add the "checkin-needed" keyword. Thanks Hubert! ::: browser/devtools/debugger/debugger-controller.js @@ +1181,5 @@ > } > > + if(aResponse.sources.length === 0){ > + DebuggerView.Sources.emptyText = L10N.getStr("noSourcesText"); > + } Can you emit the |EVENTS.SOURCES_ADDED| event and return early here? There is no point in doing any of this other stuff, like calculating the best source to select, when we don't have sources.
Attachment #8364213 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 20•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2106da0e4d60
Attachment #8364213 -
Attachment is obsolete: true
Attachment #8364736 -
Flags: checkin?
Assignee | ||
Updated•10 years ago
|
Attachment #8364736 -
Flags: checkin? → checkin?(nfitzgerald)
Comment 21•10 years ago
|
||
Comment on attachment 8364736 [details] [diff] [review] Fix for indicating when sources are loading Review of attachment 8364736 [details] [diff] [review]: ----------------------------------------------------------------- Aaaaaaaand I just realized that we should have a small test for this. Can you write a quick mochitest that verifies we get the empty test on a page with no sources? Thanks!
Attachment #8364736 -
Flags: checkin?(nfitzgerald)
Comment 22•10 years ago
|
||
Sorry for not catching that earlier!
Assignee | ||
Comment 23•10 years ago
|
||
no probs. test coming up...
Comment 24•10 years ago
|
||
Comment on attachment 8364736 [details] [diff] [review] Fix for indicating when sources are loading Review of attachment 8364736 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-controller.js @@ +1179,5 @@ > dumpn(msg); > return; > } > > + if(aResponse.sources.length === 0){ I have a drive-by nit: Please add a space after "if". @@ +1181,5 @@ > } > > + if(aResponse.sources.length === 0){ > + DebuggerView.Sources.emptyText = L10N.getStr("noSourcesText"); > + window.emit(EVENTS.SOURCES_ADDED); Would it make more sense to emit a slightly different event here (as long as it won't break tests)? SOURCES_ADDED implies to the untrained eye that *some* sources were actually added. We should have a different event for this imo, but I defer to Nick's opinion.
Assignee | ||
Comment 25•10 years ago
|
||
A different event would be nice, since we now know when sources have not been added, The event could be EVENTS.NO_SOURCES_ADDED. I could add it and run the tests, just to see if it breaks, depending on what Nick thinks.
Comment 26•10 years ago
|
||
I think a second event is unnecessary and complicates the logic of doing something after we are done loading whatever sources the server may have. You would have to register two event listeners and remove the other one depending on which fired first. Or if we had a SOURCES_ADDED and DONE_ADDING_SOURCES, the former firing when we actually add sources and the latter always firing after all the server's sources have been loaded, it is still too much to keep track of! Ugh! If we want to rename the event to say FINISHED_LOADING_SOURCES or something that makes it more clear that there aren't necessarily sources added to the view, I'd be fine with that. In any case, the existing event isn't that confusing to me: we added all sources we received, which happened to be zero. But I digress... Change the name of the existing event if you want; let's avoid adding new events.
Comment 27•10 years ago
|
||
Ok, let's just not do anything about that. Nick does have a point about avoiding too much housekeeping.
Assignee | ||
Comment 28•10 years ago
|
||
Okie dokie
Assignee | ||
Comment 29•10 years ago
|
||
> Aaaaaaaand I just realized that we should have a small test for this. Can > you write a quick mochitest that verifies we get the empty test on a page > with no sources? > > Thanks! i've written the test, but just saw this test that already exists and was wondering if it doesn't already meet our test requirement or am i misunderstanding it? http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_location-changes-02-blank.js
Comment 30•10 years ago
|
||
(In reply to Hubert B Manilla from comment #29) > > Aaaaaaaand I just realized that we should have a small test for this. Can > > you write a quick mochitest that verifies we get the empty test on a page > > with no sources? > > > > Thanks! > > i've written the test, but just saw this test that already exists and was > wondering if it doesn't already meet our test requirement or am i > misunderstanding it? > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/ > browser_dbg_location-changes-02-blank.js I think it would be fine to either add the check that the noSourcesText is shown in that test or your new test. Sorry for not catching that existing one earlier!
Assignee | ||
Comment 31•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1858ed6d6503
Attachment #8364736 -
Attachment is obsolete: true
Attachment #8371133 -
Flags: review?(nfitzgerald)
Comment 32•10 years ago
|
||
Comment on attachment 8371133 [details] [diff] [review] Fix for indicating when sources are loading Review of attachment 8371133 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the last final nit. Assuming the try push is green (ignoring known intermittent failures, if you're not sure just ask), upload and the updated patch, carry over the r+ (meaning don't flag me for review again, just mark it as r+ yourself) and add the "checkin-needed" keyword so that a sheriff will check it in for you. Thanks and good work! ::: browser/devtools/debugger/test/browser_dbg_no-page-sources.js @@ +34,5 @@ > + > + is(gEditor.getText().length, 0, > + "The source editor should not have any text displayed."); > + > + is(gDebugger.document.querySelectorAll("#sources .side-menu-widget-empty-text")[0].getAttribute("value"), Nit: `.querySelectorAll(...)[0]` could be simply `.querySelector(...)`
Attachment #8371133 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8371133 -
Attachment is obsolete: true
Attachment #8371193 -
Flags: review+
Attachment #8371193 -
Flags: checkin?
Assignee | ||
Comment 34•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1858ed6d6503
Updated•10 years ago
|
Attachment #8371193 -
Flags: checkin?
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 35•10 years ago
|
||
Thanks nick!
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2540b8b7563e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Comment 39•10 years ago
|
||
(see also bug 966894) For consistency with existing en-US strings, you should use … (single Unicode character), instead of 3 dots.
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•