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)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: harth, Assigned: b4bomsy)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
(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.
can i work on this?
(In reply to Hubert B Manilla from comment #3)
> can i work on this?

Are you still working on bug 913665?
Yes..just finishing up the tests. Will push the patch in the nextb day or so...
(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
Attached patch 927673wip.patch (obsolete) — Splinter Review
Just want to make sure i'm on the right track.
Attachment #8350797 - Flags: feedback?(vporof)
Attachment #8350797 - Flags: feedback?(nfitzgerald)
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 on attachment 8350797 [details] [diff] [review]
927673wip.patch

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

What Nick said.
Attachment #8350797 - Flags: feedback?(vporof) → feedback+
setting status to assigned.
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #8350797 - Attachment is obsolete: true
Attachment #8362154 - Flags: review?(nfitzgerald)
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)
Attachment #8362154 - Flags: feedback?(vporof)
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+
(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.
Ok , Thanks victor.
Comment on attachment 8362154 [details] [diff] [review]
Fix for indicating when sources are loading

Victor's got it :)
Attachment #8362154 - Flags: feedback?(nfitzgerald)
Attachment #8362154 - Attachment is obsolete: true
Attachment #8364213 - Flags: review?(nfitzgerald)
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+
Attachment #8364736 - Flags: checkin? → checkin?(nfitzgerald)
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)
Sorry for not catching that earlier!
no probs. test coming up...
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.
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.
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.
Ok, let's just not do anything about that. Nick does have a point about avoiding too much housekeeping.
Okie dokie
> 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
(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!
https://tbpl.mozilla.org/?tree=Try&rev=1858ed6d6503
Attachment #8364736 - Attachment is obsolete: true
Attachment #8371133 - Flags: review?(nfitzgerald)
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+
Attachment #8371133 - Attachment is obsolete: true
Attachment #8371193 - Flags: review+
Attachment #8371193 - Flags: checkin?
Thanks nick!
https://hg.mozilla.org/integration/fx-team/rev/2540b8b7563e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
(see also bug 966894) For consistency with existing en-US strings, you should use … (single Unicode character), instead of 3 dots.
Depends on: 969804
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: