Closed Bug 1371842 Opened 3 years ago Closed 3 years ago

Update Debugger frontend (6/9/2017)

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch 6-9.patch (obsolete) — Splinter Review
Attachment #8876311 - Flags: review?(jdescottes)
Attached patch 6-9-1.patch (obsolete) — Splinter Review
Attachment #8876311 - Attachment is obsolete: true
Attachment #8876311 - Flags: review?(jdescottes)
Attachment #8876314 - Flags: review?(jdescottes)
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+
Keywords: checkin-needed
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.
Attached patch 6-9-2.patchSplinter Review
Attachment #8876314 - Attachment is obsolete: true
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
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.