Update Debugger frontend (6/9/2017)

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: jlast, Assigned: jlast)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=407e8dda4a3836c421a618b5a6ce21ca4023a5aa
(Assignee)

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]
6-9-1.patch

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

Updated

5 months ago
Keywords: checkin-needed
(Assignee)

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

Comment 7

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

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:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7583224b1a42
Update Debugger frontend (6/9/2017). r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7583224b1a42
Status: NEW → RESOLVED
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.