Update Debugger frontend (6/9/2017)

RESOLVED FIXED in Firefox 55



Developer Tools: Debugger
5 months ago
3 months ago


(Reporter: jlast, Assigned: jlast)


Firefox 55

Firefox Tracking Flags

(firefox55 fixed)



(1 attachment, 2 obsolete attachments)

Comment hidden (empty)

Comment 1

5 months ago
Created attachment 8876311 [details] [diff] [review]
Attachment #8876311 - Flags: review?(jdescottes)

Comment 2

5 months ago
Created attachment 8876314 [details] [diff] [review]
Attachment #8876311 - Attachment is obsolete: true
Attachment #8876311 - Flags: review?(jdescottes)
Attachment #8876314 - Flags: review?(jdescottes)

Comment 3

5 months ago

Comment 4

5 months ago
By the way, this patch has a change to attach-thread, shared/client/main to enable the faster stepping.
Comment on attachment 8876314 [details] [diff] [review]

Review of attachment 8876314 [details] [diff] [review]:

I'm not sure what's the best solution for this kind of patches. 
On one hand I'd like to have only files coming from GitHub in the changeset (and move the changes from attach-thread & main.js to another one).
On the other hand if it gets backed out, it's easier to deal with a single changeset.

Same functional regression with the search:
- start searching with CMD+F
- type something
- press ESCAPE
- press CMD+F
=> Search input has been cleared. 
With the current version of the new debugger the search input still contains the previous value.

Mentioning it here since I'm not sure if it's intended.

I'll let you set this one as checkin-needed. 
We're reaching the end of the code freeze, I prefer to let sheriffs decide how much they want to merge.

::: devtools/client/framework/attach-thread.js
@@ +46,5 @@
>    // client-side, so the server should not do it. It also does not support
>    // blackboxing yet.
>    let useSourceMaps = false;
>    let autoBlackBox = false;
> +  let ignoreFrameEnvironment = false;

nits: ignoreFrameEnvironment = newDebuggerEnabled.
The comment above looks a bit outdated too : "It also does not support blackboxing yet."
Attachment #8876314 - Flags: review?(jdescottes) → review+


5 months ago
Keywords: checkin-needed

Comment 6

5 months ago
thanks julian.

Hopefully this gets merged because the new UI is not on by default, so the risk is low. But if it doesn't that's okay too.

I think you're right that the previous version cleared the search results, but it's not clear what the right behavior should be. Both chrome devtools and atom keeps the cmd+f space in this scenario. I'm sure others clear it.

Good point on the comment. Updated.

Comment 7

5 months ago
Created attachment 8876360 [details] [diff] [review]
Attachment #8876314 - Attachment is obsolete: true
Attachment #8876360 - Flags: review+
Comment on attachment 8876360 [details] [diff] [review]

Review of attachment 8876360 [details] [diff] [review]:

One tiny nit: sha1/tag is missing from the commit message.
Assignee: nobody → jlaster

Comment 9

4 months ago
Pushed by ryanvm@gmail.com:
Update Debugger frontend (6/9/2017). r=jdescottes
Keywords: checkin-needed
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1383148
You need to log in before you can comment on or make changes to this bug.