Closed
Bug 1324700
Opened 8 years ago
Closed 8 years ago
stylo: several tests non-fatally assert with "stylo: cannot resolve style for canvas from a ServoStyleSet yet"
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: heycam, Assigned: hiro)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(13 files)
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
Bug 1324700 - Call PreTraverseSync() before calling ResolveStyleLazily() in ResolveTransientStyle().
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
###!!! ASSERTION: stylo: cannot resolve style for canvas from a ServoStyleSet yet: 'Error', file /home/worker/workspace/build/src/dom/canvas/CanvasRenderingContext2D.cpp, line 2697
dom/canvas/crashtests/0px-size-font-667225.html
dom/canvas/crashtests/0px-size-font-shadow.html
dom/canvas/crashtests/1099143-1.html
dom/canvas/crashtests/1190705.html
dom/canvas/crashtests/1223740-1.html
dom/canvas/crashtests/1225381-1.html
dom/canvas/crashtests/1229932-1.html
dom/canvas/crashtests/1283113-1.html
dom/canvas/crashtests/1284356-1.html
dom/canvas/crashtests/1287652-1.html
dom/canvas/crashtests/1288872-1.html
dom/canvas/crashtests/745699-1.html
dom/canvas/crashtests/780392-1.html
gfx/tests/crashtests/390476.html
gfx/tests/crashtests/766452-1.html
gfx/tests/crashtests/950000.html
Reporter | ||
Comment 1•8 years ago
|
||
This is fixed in the latest mozilla-central -> stylo merge.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 2•8 years ago
|
||
This didn't get fixed after all (see bug 1334938); sorry for the noise.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•8 years ago
|
Priority: -- → P3
Comment 3•8 years ago
|
||
From Hiro's analysis in bug 1351978, this is likely causing a lot of tests to fail. Bumping the priority.
Priority: P3 → P1
Comment 4•8 years ago
|
||
Xidorn, what's your opinion on the best way to handle the canvas stuff in [1]. Should we do this over CSSOM, or should we just hard-code this stuff into Servo?
[1] http://searchfox.org/mozilla-central/rev/fd99701cafa324669d950ced902d08a7450cd48b/dom/canvas/CanvasRenderingContext2D.cpp#2667
Flags: needinfo?(xidorn+moz)
Comment 5•8 years ago
|
||
nsIStyleRule isn't actually part of CSSOM. It is a common interface for nsStyleSet to get declarations and settings. You can see the subclasses of this interface [1].
It seems there are two things that canvas wants style system to do:
1. parse a font shorthand, then compute to computed values and fill in an nsStyleFont for filling its internal state [2], and
2. parse a filter value with the given font information (for <length> values I suppose) and output an nsStyleFilter [3]
I think rather than hacking around the interface here, it is probably easier to just provide some FFI functions from Servo side to fulfull the requirements above.
[1] http://searchfox.org/mozilla-central/search?q=public+nsIStyleRule&case=false®exp=false&path=
[2] http://searchfox.org/mozilla-central/rev/990055a4902952e038cc350c9ffe1ac426d1c943/dom/canvas/CanvasRenderingContext2D.cpp#3647-3689
[3] http://searchfox.org/mozilla-central/rev/990055a4902952e038cc350c9ffe1ac426d1c943/dom/canvas/CanvasRenderingContext2D.cpp#2835-2851
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)
> nsIStyleRule isn't actually part of CSSOM. It is a common interface for
> nsStyleSet to get declarations and settings. You can see the subclasses of
> this interface [1].
>
> It seems there are two things that canvas wants style system to do:
> 1. parse a font shorthand, then compute to computed values and fill in an
> nsStyleFont for filling its internal state [2], and
> 2. parse a filter value with the given font information (for <length> values
> I suppose) and output an nsStyleFilter [3]
>
> I think rather than hacking around the interface here, it is probably easier
> to just provide some FFI functions from Servo side to fulfull the
> requirements above.
It sounds like somewhat similar to what we did for script animations.
For parsing a property, we can use Servo_ParseProperty[1].
For computing values with the parsed property, we can write a function similar to cascade_with_rules[2].
For getting nsStyleFont, we can use Servo_GetStyleFont.
[1] https://hg.mozilla.org/mozilla-central/file/35c7be9c2db2/servo/ports/geckolib/glue.rs#l905
[2] https://hg.mozilla.org/mozilla-central/file/35c7be9c2db2/servo/components/style/matching.rs#l443
[3] https://hg.mozilla.org/mozilla-central/file/35c7be9c2db2/servo/components/style/gecko_bindings/bindings.rs#l1912
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4249547e0a83ea654c9f879a2ee81c172eba7740
Just fixed font style handling not filter style handling yet.
Assignee | ||
Comment 9•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8857794 [details]
Bug 1324700 - Servo_ParseProperty() takes nsCSSPropertyID instead of nsACString.
https://reviewboard.mozilla.org/r/129782/#review132406
Attachment #8857794 -
Flags: review?(cam) → review+
Reporter | ||
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8857795 [details]
Bug 1324700 - Call PreTraverseSync() before calling ResolveStyleLazily() in ResolveTransientStyle().
https://reviewboard.mozilla.org/r/129784/#review132408
Attachment #8857795 -
Flags: review?(cam) → review+
Reporter | ||
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8857796 [details]
Bug 1324700 - Add ResolveServoTransientStyle to get servo's computed values instead of nsStyleContext.
https://reviewboard.mozilla.org/r/129786/#review132410
::: layout/style/ServoStyleSet.h:164
(Diff revision 1)
> dom::Element* aPseudoElement);
>
> // Resolves style for a (possibly-pseudo) Element without assuming that the
> // style has been resolved, and without worrying about setting the style
> // context up to live in the style context tree (a null parent is used).
> + // |aPeusoTag| and |aPseudoType| must match.
aPseudoTag
Attachment #8857796 -
Flags: review?(cam) → review+
Reporter | ||
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8857797 [details]
Bug 1324700 - Add a function which is equivalent to CreateDeclaration() for servo.
https://reviewboard.mozilla.org/r/129788/#review132412
::: dom/canvas/CanvasRenderingContext2D.cpp:2772
(Diff revision 1)
> + new URLExtraData(aDocument->GetDocumentURI(),
> + aDocument->GetDocBaseURI(),
Are these two URIs around the wrong way?
Attachment #8857797 -
Flags: review?(cam) → review+
Reporter | ||
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8857798 [details]
Bug 1324700 - Add a function that checks PropertyDeclarationBlock has a CSSWideKeyword for a given property.
https://reviewboard.mozilla.org/r/129790/#review132422
::: servo/components/style/properties/declaration_block.rs:367
(Diff revision 1)
> + self.declarations.iter().find(|&&(ref decl, _)|
> + decl.id().is_or_is_longhand_of(property) &&
> + decl.get_css_wide_keyword().is_some()
> + ).is_some()
Instead of find, you could use any:
self.declarations.iter().any(|&&(ref decl, _)|
decl.id().is_or_is_longhand_of(property) &&
decl.get_css_wide_keyword().is_some()
)
Attachment #8857798 -
Flags: review?(cam) → review+
Reporter | ||
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8857799 [details]
Bug 1324700 - Add an FFI which returns computed values for a given declaration block with/without parent_style.
https://reviewboard.mozilla.org/r/129792/#review132424
Attachment #8857799 -
Flags: review?(cam) → review+
Reporter | ||
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8857800 [details]
Bug 1324700 - Add a function which is equivalent to GetFontStyleContext() for servo.
https://reviewboard.mozilla.org/r/129794/#review132428
Attachment #8857800 -
Flags: review?(cam) → review+
Reporter | ||
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8857801 [details]
Bug 1324700 - Resolve font property for servo.
https://reviewboard.mozilla.org/r/129796/#review132430
Attachment #8857801 -
Flags: review?(cam) → review+
Reporter | ||
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8857802 [details]
Bug 1324700 - Add a function which is equivalent to ResolveStyleForFilter for servo.
https://reviewboard.mozilla.org/r/129798/#review132432
Attachment #8857802 -
Flags: review?(cam) → review+
Reporter | ||
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8857803 [details]
Bug 1324700 - Resolve filter property for servo.
https://reviewboard.mozilla.org/r/129800/#review132434
Attachment #8857803 -
Flags: review?(cam) → review+
Reporter | ||
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8857804 [details]
Bug 1324700 - Drop warnings for stylo.
https://reviewboard.mozilla.org/r/129802/#review132436
Attachment #8857804 -
Flags: review?(cam) → review+
Reporter | ||
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8857805 [details]
Bug 1324700 - Update assertion counts which had been caused by font handling in canvas element.
https://reviewboard.mozilla.org/r/129804/#review132438
Attachment #8857805 -
Flags: review?(cam) → review+
Reporter | ||
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8857806 [details]
Bug 1324700 - Update reftest expectations.
https://reviewboard.mozilla.org/r/129806/#review132440
Great!
Attachment #8857806 -
Flags: review?(cam) → review+
Assignee | ||
Comment 36•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 50•8 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c98b26bb1063
Servo_ParseProperty() takes nsCSSPropertyID instead of nsACString. r=heycam
https://hg.mozilla.org/integration/autoland/rev/73c495cad7d6
Call PreTraverseSync() before calling ResolveStyleLazily() in ResolveTransientStyle(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/b116710343a7
Add ResolveServoTransientStyle to get servo's computed values instead of nsStyleContext. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b38bae9c02fe
Add a function which is equivalent to CreateDeclaration() for servo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3e19c81c80fc
Add a function that checks PropertyDeclarationBlock has a CSSWideKeyword for a given property. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3d2bbd149e04
Add an FFI which returns computed values for a given declaration block with/without parent_style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1876d89c8f37
Add a function which is equivalent to GetFontStyleContext() for servo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e959737822e5
Resolve font property for servo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8dafc925b034
Add a function which is equivalent to ResolveStyleForFilter for servo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6b13b4abe4a9
Resolve filter property for servo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/eb6ce1493b85
Drop warnings for stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0ba623e978ef
Update assertion counts which had been caused by font handling in canvas element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ed88aa504601
Update reftest expectations. r=heycam
Comment 51•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c98b26bb1063
https://hg.mozilla.org/mozilla-central/rev/73c495cad7d6
https://hg.mozilla.org/mozilla-central/rev/b116710343a7
https://hg.mozilla.org/mozilla-central/rev/b38bae9c02fe
https://hg.mozilla.org/mozilla-central/rev/3e19c81c80fc
https://hg.mozilla.org/mozilla-central/rev/3d2bbd149e04
https://hg.mozilla.org/mozilla-central/rev/1876d89c8f37
https://hg.mozilla.org/mozilla-central/rev/e959737822e5
https://hg.mozilla.org/mozilla-central/rev/8dafc925b034
https://hg.mozilla.org/mozilla-central/rev/6b13b4abe4a9
https://hg.mozilla.org/mozilla-central/rev/eb6ce1493b85
https://hg.mozilla.org/mozilla-central/rev/0ba623e978ef
https://hg.mozilla.org/mozilla-central/rev/ed88aa504601
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•