Update Debugger Frontend v96

RESOLVED FIXED in Firefox 65

Status

enhancement
P3
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: davidwalsh, Assigned: davidwalsh)

Tracking

(Blocks 1 bug)

unspecified
Firefox 65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Updated

7 months ago
Blocks: 1502256
Comment on attachment 9020254 [details] [diff] [review]
1502224-01.patch

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]
1502224-02.patch

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

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

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

Do we no longer need the DebuggerModules instruction? Should we delete https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/node-templates.mozbuild ? 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 https://hg.mozilla.org/mozilla-central/rev/0f30ba8cfeec.

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

Comment 6

7 months ago
Attachment #9020410 - Attachment is obsolete: true
(Assignee)

Comment 8

7 months ago
Posted patch 1502224-04.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59f88434009d15d88731f658a1f0b177dbf2d008
Attachment #9020258 - Attachment is obsolete: true
Attachment #9020408 - Attachment is obsolete: true
Attachment #9020408 - Flags: review?(jdescottes)
Attachment #9020434 - Flags: review?(jdescottes)
(Assignee)

Updated

7 months ago
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
(Assignee)

Comment 10

7 months ago
- 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]
1502224-04.patch

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

Comment 12

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79f7abd11cd713cb7af1ace331d59b79e426f468

Skipped querystring test, will debug separately.
Attachment #9020434 - Attachment is obsolete: true
Attachment #9020824 - Flags: review?(jdescottes)
(Assignee)

Updated

7 months ago
Attachment #9020824 - Flags: review?(jdescottes) → review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 14

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc381d0178d2
Status: NEW → RESOLVED
Last Resolved: 7 months 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.