If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Don't say "This page has no sources" when the debugger is loading.

VERIFIED FIXED in Firefox 30

Status

()

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

People

(Reporter: harth, Assigned: Hubert B Manilla)

Tracking

unspecified
Firefox 30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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.
Dupe of bug 883249?
(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

4 years ago
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?
(Assignee)

Comment 5

4 years ago
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
(Assignee)

Comment 7

4 years ago
Created attachment 8350797 [details] [diff] [review]
927673wip.patch

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
(Assignee)

Comment 11

4 years ago
Created attachment 8362154 [details] [diff] [review]
Fix for indicating when sources are loading
Attachment #8350797 - Attachment is obsolete: true
Attachment #8362154 - Flags: review?(nfitzgerald)
(Assignee)

Comment 12

4 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

4 years ago
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.
(Assignee)

Comment 15

4 years ago
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)
(Assignee)

Comment 17

4 years ago
Created attachment 8364213 [details] [diff] [review]
Fix for indicating when sources are loading
Attachment #8362154 - Attachment is obsolete: true
Attachment #8364213 - Flags: review?(nfitzgerald)
(Assignee)

Comment 18

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=38c168c1e23e
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

4 years ago
Created attachment 8364736 [details] [diff] [review]
Fix for indicating when sources are loading

https://tbpl.mozilla.org/?tree=Try&rev=2106da0e4d60
Attachment #8364213 - Attachment is obsolete: true
Attachment #8364736 - Flags: checkin?
(Assignee)

Updated

4 years ago
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!
(Assignee)

Comment 23

4 years ago
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.
(Assignee)

Comment 25

4 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.
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.
(Assignee)

Comment 28

4 years ago
Okie dokie
(Assignee)

Comment 29

4 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
(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

4 years ago
Created attachment 8371133 [details] [diff] [review]
Fix for indicating when sources are loading

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+
(Assignee)

Comment 33

4 years ago
Created attachment 8371193 [details] [diff] [review]
Fix for indicating when sources are loading
Attachment #8371133 - Attachment is obsolete: true
Attachment #8371193 - Flags: review+
Attachment #8371193 - Flags: checkin?
(Assignee)

Comment 34

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=1858ed6d6503
Attachment #8371193 - Flags: checkin?
Keywords: checkin-needed
(Assignee)

Comment 35

4 years ago
Thanks nick!
https://hg.mozilla.org/integration/fx-team/rev/2540b8b7563e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Duplicate of this bug: 883249
https://hg.mozilla.org/mozilla-central/rev/2540b8b7563e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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

Updated

4 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.