Closed Bug 1401573 Opened 2 years ago Closed 2 years ago

Fix PoE, Link, and linux issues

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox57 fixed, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(2 files, 3 obsolete files)

Attached patch fix-link.patch (obsolete) — Splinter Review
Attachment #8910407 - Flags: review?(nchevobbe)
Attached patch fix-expr.patch (obsolete) — Splinter Review
Attachment #8910441 - Flags: review?(nchevobbe)
Comment on attachment 8910407 [details] [diff] [review]
fix-link.patch

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

Looks fine except for the tab. But I don't think it is harmful, and will probably be replaced with the next bundle

::: devtools/client/debugger/new/debugger.js
@@ +32151,5 @@
>      if (!connection) {
>        return;
>      }
>  
> +	var services = options.services;

there is a TAB here
Attachment #8910407 - Flags: review?(nchevobbe) → review+
Comment on attachment 8910441 [details] [diff] [review]
fix-expr.patch

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

Looks good to me (except the TAB, but harmless and will be fixed in the next bundle)

::: devtools/client/debugger/new/debugger.js
@@ +19308,5 @@
>        console.warn("Expressions should not be empty");
>        return;
>      }
>  
> +	const input = wrapExpression(expression.input);

TAB
Attachment #8910441 - Flags: review?(nchevobbe) → review+
devtools/client/debugger/new/test/mochitest/browser_dbg-expressions.js is failing quite often. Let's try to fix this
It seems to be fixable by replacing "undefined" by "ReferenceError […]" in http://searchfox.org/mozilla-central/source/devtools/client/debugger/new/test/mochitest/browser_dbg-expressions.js#64.
And it looks better to me, if you evaluate something that throw, you do want to have the error.
Attached patch poe.patch (obsolete) — Splinter Review
TRY is fine with this patch.
Jason, what do you think of this
Attachment #8910441 - Attachment is obsolete: true
Flags: needinfo?(jlaster)
That looks good for now. I think (unavailable) has its merits too in many cases because watch expressions are global, yet often designed for a single function. Either way, we should land this patch as is for 57.
Flags: needinfo?(jlaster)
r+ since it's a squashed version of 2 already reviewed patches
Attachment #8910407 - Attachment is obsolete: true
Attachment #8910691 - Attachment is obsolete: true
Attachment #8910709 - Flags: review+
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a001ffb41787
Fix PoE, Link, and linux issue. r=nchevobbe
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a001ffb41787
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Attached patch 9-26-patch.patchSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: New debugger UI
[User impact if declined]: we introduce a couple regressions
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? No, the surface area is small and easy to test 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No, 
[Why is the change risky/not risky?]: They are edge cases, which are hard to hit.
[String changes made/needed]:
Attachment #8912308 - Flags: review?(nchevobbe)
Attachment #8912308 - Flags: approval-mozilla-beta?
I will approve this patch but next time I won't, please fill one uplift request per problem.
Here, this will make everyone's life harder if we have to debug regressions...
Summary: Fix PoE, Link, and linux issue → Fix PoE, Link, and linux issues
Comment on attachment 8912308 [details] [diff] [review]
9-26-patch.patch

Should be in 57b4 (gtb tomorrow Thursday)
Attachment #8912308 - Flags: review?(nchevobbe)
Attachment #8912308 - Flags: approval-mozilla-beta?
Attachment #8912308 - Flags: approval-mozilla-beta+
(In reply to Jason Laster [:jlast] from comment #16)
> [Is this code covered by automated tests?]: No
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? No, the surface area is small and easy to test 

Setting qe-verify- based on Jason Laster's assessment on manual testing needs.
Flags: qe-verify-
Duplicate of this bug: 1388096
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.