1371842 - Update Debugger frontend (6/9/2017)

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Jason Laster [:jlast]]

Attachment: 6-9.patch

Comment 2

[Jason Laster [:jlast]]

Attachment: 6-9-1.patch

Comment 3

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=407e8dda4a3836c421a618b5a6ce21ca4023a5aa

Comment 4

[Jason Laster [:jlast]]

By the way, this patch has a change to attach-thread, shared/client/main to enable the faster stepping.

Comment 5

[Julian Descottes [:jdescottes]]

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."

Comment 6

[Jason Laster [:jlast]]

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

[Jason Laster [:jlast]]

Attachment: 6-9-2.patch

Comment 8

[Julian Descottes [:jdescottes]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7583224b1a42
Update Debugger frontend (6/9/2017). r=jdescottes

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7583224b1a42