Closed Bug 1420026 Opened 8 years ago Closed 8 years ago

replace one more use of nsCSSParser::ParseColor with ServoCSSParser::ComputeColor

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files, 5 obsolete files)

Bug 1420026 - Part 2: Replace one more use of nsCSSParser::ParseColor with ServoCSSParser::ComputeColor.
59 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1420026 - Part 1: Add aWasCurrentColor outparam to ServoCSSParser::ComputeColor.
59 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1420026 - Part 3: Report canvas color parsing errors to the console.
59 bytes, text/x-review-board-request
TYLin
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review
followup: Enable Servo CSS color parsing in canvas.
2.39 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
I missed one in bug 1408312.
Comment on attachment 8931205 [details] geckolib: Return from Servo_ComputeColor whether the value was currentcolor. https://reviewboard.mozilla.org/r/202302/#review207662 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/canvas/CanvasRenderingContext2D.cpp:1198 (Diff revision 1) > - Unused << nsRuleNode::ComputeColor( > + Unused << nsRuleNode::ComputeColor( > - value, presShell ? presShell->GetPresContext() : nullptr, parentContext, > + value, presShell ? presShell->GetPresContext() : nullptr, parentContext, > - *aColor); > + *aColor); > - } > + } > - return true; > + return true; > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8931205 [details] geckolib: Return from Servo_ComputeColor whether the value was currentcolor. https://reviewboard.mozilla.org/r/202302/#review207664 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/canvas/CanvasRenderingContext2D.cpp:1183 (Diff revision 2) > + currentColor = canvasFrame->StyleColor()->mColor; > + } > + } > + > + return ServoCSSParser::ComputeColor(set, currentColor, aString, aColor); > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Attachment #8931205 - Flags: review?(tlin)
Some test failures, cancelling review for now.
Attachment #8931205 - Flags: review?(tlin)
Comment on attachment 8931205 [details] geckolib: Return from Servo_ComputeColor whether the value was currentcolor. https://reviewboard.mozilla.org/r/202302/#review208078 ::: servo/ports/geckolib/glue.rs:4599 (Diff revision 3) > Some(computed_color) => { > let rgba = computed_color.to_rgba(current_color); > *result_color = gecko::values::convert_rgba_to_nscolor(&rgba); > + if !was_current_color.is_null() { > + unsafe { > + *was_current_color = !computed_color.is_numeric(); Should this be computed_color.is_currentcolor()?
Attachment #8931205 - Flags: review?(tlin) → review+
Comment on attachment 8931582 [details] geckolib: Return from Servo_ComputeColor whether the value was currentcolor. https://reviewboard.mozilla.org/r/202712/#review208080
Attachment #8931582 - Flags: review?(tlin) → review+
Comment on attachment 8931583 [details] Bug 1420026 - Part 2: Replace one more use of nsCSSParser::ParseColor with ServoCSSParser::ComputeColor. https://reviewboard.mozilla.org/r/202714/#review208082
Attachment #8931583 - Flags: review?(tlin) → review+
Comment on attachment 8931205 [details] geckolib: Return from Servo_ComputeColor whether the value was currentcolor. https://reviewboard.mozilla.org/r/202302/#review208078 > Should this be computed_color.is_currentcolor()? Yeah, I should probably change it to is_currentcolor(). Both will work, since here we never have a complex color with a non-zero-or-one foreground ratio (which should only ever result from animations, and can't be specified in the color string). But is_currentcolor() looks more sensible...
Attachment #8931205 - Attachment is obsolete: true
Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/75636e9e1d13 Part 1: Add aWasCurrentColor outparam to ServoCSSParser::ComputeColor. r=TYLin https://hg.mozilla.org/integration/autoland/rev/7ef37ebdf7b2 Part 2: Replace one more use of nsCSSParser::ParseColor with ServoCSSParser::ComputeColor. r=TYLin
Backout by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/47453904f1f8 Backed out 2 changesets for failing devtools webconsole/test/browser_webconsole_bug_595934_message_categories.js on Windows 7 debug without e10s. r=backout on a CLOSED TREE
Priority: -- → P3
Attachment #8934051 - Flags: review?(tlin)
Attachment #8934052 - Flags: review?(tlin)
Attachment #8934053 - Flags: review?(tlin)
Part 1 didn't change, though mozreview decided to ask you for review again...
Comment on attachment 8934051 [details] geckolib: Allow an ErrorReporter to be created with null URL data. https://reviewboard.mozilla.org/r/204990/#review210452
Attachment #8934051 - Flags: review?(tlin) → review+
Comment on attachment 8934052 [details] style: Add a new CSS error type for lone value parsing errors. https://reviewboard.mozilla.org/r/204992/#review210454
Attachment #8934052 - Flags: review?(tlin) → review+
Comment on attachment 8934053 [details] geckolib: Allow Servo_ComputeColor to report errors to the console. https://reviewboard.mozilla.org/r/204994/#review210456
Attachment #8934053 - Flags: review?(tlin) → review+
Comment on attachment 8934054 [details] Bug 1420026 - Part 1: Add aWasCurrentColor outparam to ServoCSSParser::ComputeColor. https://reviewboard.mozilla.org/r/204996/#review210460
Attachment #8934054 - Flags: review?(tlin) → review+
Comment on attachment 8934055 [details] Bug 1420026 - Part 3: Report canvas color parsing errors to the console. https://reviewboard.mozilla.org/r/204998/#review210466
Attachment #8934055 - Flags: review?(tlin) → review+
Attachment #8931582 - Attachment is obsolete: true
Attachment #8934051 - Attachment is obsolete: true
Attachment #8934052 - Attachment is obsolete: true
Attachment #8934053 - Attachment is obsolete: true
Attached file Servo PR —
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/14359477d5da Part 1: Add aWasCurrentColor outparam to ServoCSSParser::ComputeColor. r=TYLin https://hg.mozilla.org/integration/autoland/rev/c9c3ce217c3d Part 2: Replace one more use of nsCSSParser::ParseColor with ServoCSSParser::ComputeColor. r=TYLin https://hg.mozilla.org/integration/autoland/rev/e68d4aa659c6 Part 3: Report canvas color parsing errors to the console. r=TYLin
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/10d466f59108 Disable this code path for now for permafailure. r=me
This was permafailing a devtools test (devtools/client/webconsole/test/browser_webconsole_bug_595934_message_categories.js): https://treeherder.mozilla.org/logviewer.html#?job_id=149942062&repo=autoland&lineNumber=16203 So I landed the change above to avoid backing out.
Flags: needinfo?(cam)
Keywords: leave-open
Hmm, perhaps I messed something up when rebasing, since that's the exact test failure that I initially hit and fixed before relanding. Thanks for landing the patch to avoid this getting backed out.
Oh, this test is disabled on e10s (bug 1243979), which is why it didn't show up in my try run. :-/
For half a day, push from comment 39 caused this regression: == Change summary for alert #10882 (as of Tue, 05 Dec 2017 05:03:25 GMT) == Regressions: 6% tcanvasmark linux64 opt e10s 10,658.62 -> 9,977.88 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10882
:emilio's fix from comment 41 fixed it: == Change summary for alert #10887 (as of Tue, 05 Dec 2017 17:11:06 GMT) == Improvements: 8% tcanvasmark linux64 opt e10s 9,877.00 -> 10,625.46 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10887
Looks like we need to finish up this before landing bug 1430014.
The talos regression is also concerning... Sounds like Servo's color parsing is slower?
It looks like tcanvasmark has been removed in bug 1426682, so maybe we don't need to care about it... Although I'm a bit concerned about regressing performance there...
Weird that I cannot reproduce the failure on browser_webconsole_message_categories.js locally like what I saw in my try push for bug 1430014...
A try push with just enabling the new code path also shows the test failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbfafbaa9fc1e12841586d98c1e3b351406ffaa6 But I still have no luck to reproduce it locally :/
I couldn't repro either (didn't try too hard), but I suspect it times out because it's missing a couple null-checks (the original code for `document`, the code you write for `mCanvasElement` in the color case). Also there's no need to hold a strong reference to a shell given we don't use it after the only potential flush, I hope that helps with the perf regression.
Ok, so no such luck, but I managed to reproduce the error. Basically you need to load: devtools/client/webconsole/new-console-output/test/mochitest/test-message-categories-canvas-css.html Locally. In the patched build or with stylo disabled, you don't see the error message in the console (beware that CSS errors don't get reported by default, so you need to "filter output" to see them) I don't know if I'll have time to investigate this evening, but hope it helps.
Attachment #8947154 - Flags: review?(xidorn+moz) → review+
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/be78627933ff followup: Enable Servo CSS color parsing in canvas. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/be78627933ff
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: