1269933 - Handle list-style-type in stylo and work around the CounterStyleManager

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I talked with Xidorn. Supporting @counter-style would be a lot of work, and Firefox is the only UA to implement it right now so it's a pretty low priority for us at the moment.

The machinery initially seems a bit hard to scoot out of the way, but we figured out a strategy. Patches to follow.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Pass the builtin counter manager corresponding to the appropriate default when initializing style structs for servo. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Teach CounterStyles their name and remove the string member from the style structs. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add hooks for Servo to manipulate list-style-type. v1

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7224c4ec0c

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8748887 [details] [diff] [review]
Part 2 - Teach CounterStyles their name and remove the string member from the style structs. v1

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

::: layout/style/CounterStyleManager.h
@@ +50,5 @@
>    // Custom styles are certainly dependent. In addition, some builtin
>    // styles are dependent for fallback.
>    bool IsDependentStyle() const;
>  
> +  virtual void GetStyleName(nsSubstring& aResult) = 0;

I think GetName should be enough here, but I can live with GetStyleName as well.

::: layout/style/nsStyleStruct.cpp
@@ -737,5 @@
>    if (EqualImages(mListStyleImage, aOther.mListStyleImage) &&
>        mCounterStyle == aOther.mCounterStyle) {
>      if (mImageRegion.IsEqualInterior(aOther.mImageRegion)) {
> -      if (mListStyleType != aOther.mListStyleType)
> -        return nsChangeHint_NeutralChange;

This change actually indicates this patch does change the behavior in some cases.

Specifically, this changes the following two cases:
* if list-style-type is a non-existing counter style, the computed value becomes "decimal" rather than the specified value;
* if list-style-type refers to a builtin style, the computed value is always lowercased rather than keeps the original case.

I guess these changes are fine, probably even somehow perferable. I'll raise this with CSSWG.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8748888 [details] [diff] [review]
Part 3 - Add hooks for Servo to manipulate list-style-type. v1

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

::: layout/style/CounterStyleManager.cpp
@@ +1001,1 @@
>    NS_DECL_OWNINGTHREAD

If we are going to refcount the pointers off-main-thread, is there still any owning thread here? (Actually I don't see why we need this decl here. It doesn't seem to be used anywhere. I think I just copied it from some other place without understanding the reason.)

In addition, this object is allocated from the arena, would there be any issue in terms of thread-safety for that?

It seems you are not going to support custom counter style (and probably also complex CJK counter styles) at this point, so this change is actually not needed at present. I guess you can probably just assert somewhere that this would never be created within Servo's style.

@@ +1191,5 @@
>    // not 'extends'.
>    CounterStyle* mExtendsRoot;
>  
> +  // Parallel restyle may refcount these pointers off-main-thread.
> +  ThreadSafeAutoRefCnt mRefCnt;

Same here.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> Comment on attachment 8748888 [details] [diff] [review]
> Part 3 - Add hooks for Servo to manipulate list-style-type. v1
> 
> Review of attachment 8748888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/CounterStyleManager.cpp
> @@ +1001,1 @@
> >    NS_DECL_OWNINGTHREAD
> 
> If we are going to refcount the pointers off-main-thread, is there still any
> owning thread here? (Actually I don't see why we need this decl here. It
> doesn't seem to be used anywhere. I think I just copied it from some other
> place without understanding the reason.)

There isn't an owning thread, but the the addref/release macros still won't compile without it: http://searchfox.org/mozilla-central/rev/64a1e03ff39d72e0f9c138af0af4fda99606c54d/xpcom/glue/nsISupportsImpl.h#571


> In addition, this object is allocated from the arena, would there be any
> issue in terms of thread-safety for that?

I guess we'd need to proxy-release on the main thread in the destroy method - that's a good point.

> 
> It seems you are not going to support custom counter style (and probably
> also complex CJK counter styles) at this point, so this change is actually
> not needed at present. I guess you can probably just assert somewhere that
> this would never be created within Servo's style.

Fair enough.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add hooks for Servo to manipulate list-style-type. v2

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8749404 [details] [diff] [review]
Part 3 - Add hooks for Servo to manipulate list-style-type. v2

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

OK, thanks.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Although I r+ed these patches, but it seems neither you nor I am peer of style system, so I'm not completely sure whether I'm qualified to do this. Probably need heycam to double check.

Comment 11

[Cameron McCormack (:heycam)]

Patches look OK to me.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

ni? myself for emailing CSSWG mentioned in comment 5.

Comment 13

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ec3f7e368f

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/382ae5a47588
https://hg.mozilla.org/integration/mozilla-inbound/rev/922ba5fbebbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fe66b7e3c2

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/382ae5a47588
https://hg.mozilla.org/mozilla-central/rev/922ba5fbebbe
https://hg.mozilla.org/mozilla-central/rev/94fe66b7e3c2

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

https://lists.w3.org/Archives/Public/www-style/2016May/0076.html