Closed Bug 1324700 Opened 7 years ago Closed 6 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)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: hiro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(13 files)

Bug 1324700 - Servo_ParseProperty() takes nsCSSPropertyID instead of nsACString.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Call PreTraverseSync() before calling ResolveStyleLazily() in ResolveTransientStyle().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Add ResolveServoTransientStyle to get servo's computed values instead of nsStyleContext.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Add a function which is equivalent to CreateDeclaration() for servo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Add a function that checks PropertyDeclarationBlock has a CSSWideKeyword for a given property.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Add an FFI which returns computed values for a given declaration block with/without parent_style.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Add a function which is equivalent to GetFontStyleContext() for servo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Resolve font property for servo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Add a function which is equivalent to ResolveStyleForFilter for servo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Resolve filter property for servo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Drop warnings for stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Update assertion counts which had been caused by font handling in canvas element.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1324700 - Update reftest expectations.
59 bytes, 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
This is fixed in the latest mozilla-central -> stylo merge.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
This didn't get fixed after all (see bug 1334938); sorry for the noise.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1330589
Priority: -- → P3
Blocks: 1351978
From Hiro's analysis in bug 1351978, this is likely causing a lot of tests to fail. Bumping the priority.
Priority: P3 → P1
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)
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&regexp=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)
(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
Ok - Hiro, can you take this one?
Assignee: nobody → hikezoe
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4249547e0a83ea654c9f879a2ee81c172eba7740
Just fixed font style handling not filter style handling yet.
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+
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+
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+
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+
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+
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+
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+
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+
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+
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+
Comment on attachment 8857804 [details]
Bug 1324700 - Drop warnings for stylo.

https://reviewboard.mozilla.org/r/129802/#review132436
Attachment #8857804 - Flags: review?(cam) → 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+
Comment on attachment 8857806 [details]
Bug 1324700 - Update reftest expectations.

https://reviewboard.mozilla.org/r/129806/#review132440

Great!
Attachment #8857806 - Flags: review?(cam) → review+
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
Depends on: 1356919
Blocks: 1324701
You need to log in before you can comment on or make changes to this bug.