Closed Bug 1502224 Opened 6 years ago Closed 6 years ago

Update Debugger Frontend v96


(DevTools :: Debugger, enhancement, P3)



(firefox65 fixed)

Firefox 65
Tracking Status
firefox65 --- fixed


(Reporter: davidwalsh, Assigned: davidwalsh)




(1 file, 6 obsolete files)

Blocks: 1502256
Comment on attachment 9020254 [details] [diff] [review]

I suppose the first patch is obsolete?
Attachment #9020254 - Attachment is obsolete: true
Attachment #9020254 - Flags: review?(jdescottes)
Comment on attachment 9020258 [details] [diff] [review]

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

Thanks! A few comments that we need to look at before landing.

::: devtools/client/debugger/new/src/
@@ +13,4 @@
>      'workers',
>  ]
> +DevToolsModules(

Do we no longer need the DebuggerModules instruction? Should we delete ? Haven't followed the topic closely lately.

::: devtools/client/debugger/new/test/mochitest/browser_dbg-scopes.js
@@ +8,4 @@
>  add_task(async function() {
>    const dbg = await initDebugger("doc-script-switching.html");
> +  const ready = Promise.all([

the patch doesn't apply because this change is already on central from

Needs to be rebased.

::: devtools/client/debugger/new/test/mochitest/browser_dbg-sources-querystring.js
@@ +15,5 @@
> +    selectors: { getSelectedSource },
> +    getState
> +  } = dbg;
> +
> +  await waitForSources(dbg, "simple1.js?x=1", "simple1.js?x=2");

The test often blocks here. It seems to be super frequent on OSX debug.
I can't reproduce the issue locally for now, even thought I am using debug builds on OSX with --verify.

Maybe skip on this platform if you can't figure out why this is failing? With a bugged (or GH issue) logged to investigate it.
Attachment #9020258 - Flags: review?(jdescottes)
Attachment #9020410 - Attachment is obsolete: true
Attached patch 1502224-04.patch (obsolete) — Splinter Review
Attachment #9020258 - Attachment is obsolete: true
Attachment #9020408 - Attachment is obsolete: true
Attachment #9020408 - Flags: review?(jdescottes)
Attachment #9020434 - Flags: review?(jdescottes)
Blocks: 1502494
Some questions about the last update:
- which patch should I review, the one on splinter or the one on phabricator? 
- looking at try the querystring test still seems very intermittent, not sure if anything changed about it?
- regarding my question about "DebuggerModules" vs "DevToolsModules", I see the change is gone now, but I would still like to know if we plan to remove it since it's only used in one spot?
Flags: needinfo?(dwalsh)
Attachment #9020430 - Attachment is obsolete: true
- They were both the same, but I closed the Phabricator revision to avoid confusion.
- Weird; we've had 0 problems with it on travis.  I'll look into it.
- I'll talk to Alex about "DebuggerModules" vs "DevToolsModules" -- I don't know what it does and didn't want to change it.
Flags: needinfo?(dwalsh)
Comment on attachment 9020434 [details] [diff] [review]

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

The patch is fine codewise, but given the intermittent failures on try this might be backed out.

As mentioned in my first review, can we:
- skip the test
- file a follow up bug or GH issue to reenable it
- skip-if=true should be followed by a comment linking to the bug/issue
Attachment #9020434 - Flags: review?(jdescottes) → review+
Attached patch 1502224-05.patchSplinter Review

Skipped querystring test, will debug separately.
Attachment #9020434 - Attachment is obsolete: true
Attachment #9020824 - Flags: review?(jdescottes)
Attachment #9020824 - Flags: review?(jdescottes) → review+
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Blocks: 1503278
No longer blocks: 1503278
You need to log in before you can comment on or make changes to this bug.