Closed Bug 1328319 Opened 6 years ago Closed 6 years ago

stylo: Implement @counter-style

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: bholley, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 5 obsolete files)

Bug 1328319 part 1 - Move nsCSSCounterStyleRule into a separate header file.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328319 part 2 - Add basic integration of @counter-style.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328319 part 3 - Add support for auto value in nsCSSValue sugar.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1328319 part 4 - Add binding for setting pair value of nsCSSValue.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1328319 part 5 - Add bindings for list and pair list value of nsCSSValue.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1328319 part 6 - Make it possible to delay resolving counter style.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328319 part 7 - Enable querying counter-style rule on Servo backend.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328319 part 8 - Make stylo use counter-style for list-style-type and counter functions.
59 bytes, text/x-review-board-request
heycam
: review+
SimonSapin
: review+
Details
Bug 1328319 part 10 - Update test expectations.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328319 part 9 - Don't clean up retired counter styles for stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
So far (and going forward after bz's work in bug 1298588), we've been hacking around the @counter-style machinery in Gecko. Eventually, I think we're going to want to implement support for this in the Servo code, and then leverage that from Gecko.

There are only a few tests in the tree that use @counter-style, and no other engine implements it. So this isn't urgent, and we should save it until everything else is more or less working.
Note that many of widely-supported builtin counter styles are currently implemented as @counter-style rule in counterstyles.css, so it may have larger impact than the at-rule itself.
Blocks: stylo-syntax
Not a super-important feature, but sufficiently complex to add uncertainty. P2.
Priority: -- → P2
Assigning myself since I implemented counter-style rule in Gecko :)

We would need this to support various builtin list-style-types.
Assignee: nobody → xidorn+moz
In case that’s useful, servo/components/layout/generated_content.rs has algorithms to turn an integer into a string representation roughly based on the CSS Counter Styles spec.
I think we just need to implement the parser and then hand the rule data to Gecko. We can reuse the string generation algorithm in C++.

Although I haven't looked into the code in Servo in details, I believe the algorithm for handling @counter-style is much more complicated than what has been implemented in Servo, because basically you don't need that complexity if you don't support @counter-style...
Blocks: 1352291
I think we can implement it in exactly the same way as @font-face that we have Servo parse the rule and use that data to initialize Gecko's corresponding rule object.
Given the number of failing tests in bug 1352291, I'm bumping the priority here.
Priority: P2 → P1
Depends on: 1354970
Blocks: 1356087
Depends on: 1362302
Blocks: 1363590
It doesn't fix that many tests... Oh well...
Blocks: 1363596
Blocks: stylo-cssom, 1357724
Comment on attachment 8866215 [details]
Bug 1328319 part 1 - Move nsCSSCounterStyleRule into a separate header file.

https://reviewboard.mozilla.org/r/137800/#review140966
Attachment #8866215 - Flags: review?(cam) → review+
Comment on attachment 8866216 [details]
Bug 1328319 part 2 - Add basic integration of @counter-style.

https://reviewboard.mozilla.org/r/137802/#review140970

::: servo/ports/geckolib/glue.rs:837
(Diff revision 1)
>      getter: Servo_CssRules_GetSupportsRuleAt,
>      debug: Servo_SupportsRule_Debug,
>      to_css: Servo_SupportsRule_GetCssText,
>  }
>  
> +macro_rules! impl_getter_for_embeded_rule {

embedded
Attachment #8866216 - Flags: review?(cam) → review+
Comment on attachment 8866224 [details]
Bug 1328319 part 6 - Make it possible to delay resolving counter style.

https://reviewboard.mozilla.org/r/137820/#review140972

::: layout/style/nsStyleStruct.h:1518
(Diff revision 1)
> +  // mCounterStyle is the actual field for computed list-style-type.
> +  // mListStyleType is only used when we are off the main thread, so we
> +  // cannot safely construct CounterStyle object. FinishStyle() will
> +  // use it to setup mCounterStyle and then clear it. At any time, only
> +  // one of the following two fields should be non-null.
> +  nsCOMPtr<nsIAtom> mListStyleType;

I'm not certain whether it matters that we don't check mListStyleType from nsStyleList::CalcDifference.  I think we may as well compare it in there.
Attachment #8866224 - Flags: review?(cam) → review+
Comment on attachment 8866225 [details]
Bug 1328319 part 7 - Enable querying counter-style rule on Servo backend.

https://reviewboard.mozilla.org/r/137822/#review140974
Attachment #8866225 - Flags: review?(cam) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> Although their refcnt is threadsafe, their destroy functions are not,
> and thus the assertions there. It is not completely clear to me whether
> that's going to work. If not, we have two choices:
> 1. Stop using presshell arena for those objects, which is basically a
>    backout of bug 1077718. This may regress security since we use kind
>    of manual lifetime management for those objects, and the objects have
>    vtable, which means if we make any mistake on lifetime somewhere, we
>    may be in trouble (e.g. bug 1075336 or bug 1077687).
> 2. Collect all objects which are removed from the CounterStyleManager
>    cache table into a clear list, and add another cleanup phase after
>    we rebuild all the style structs. With that, we should be able to
>    guarantee all objects are eventually released in the main thread.

I think we do need to handle the case of a CounterStyle object being destroyed during traversal.

In bug 1356103 I added a mechanism to queue up tasks, from C++, to do once traversal is finished:

http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/layout/style/ServoStyleSet.h#318

Currently this is only used while the ServoFontMetrics lock is locked, so that would mean we'd either (a) need some other lock here so we can safely append our PostTraversalTask, or (b) we could maintain a queue of PostTraversalTasks per thread, like Servo does with its SequentialTasks.

If we don't expect to be destroying these objects frequently, then maybe it's OK just to lock.  WDYT?
Flags: needinfo?(xidorn+moz)
I'm going to make those objects not refcounted in bug 1363699.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8866216 [details]
Bug 1328319 part 2 - Add basic integration of @counter-style.

https://reviewboard.mozilla.org/r/137802/#review141226

::: servo/ports/geckolib/glue.rs:854
(Diff revision 1)
> -    }
> +            }
> -}
> +        }
> +    }
> +}
> +
> +impl_getter_for_embeded_rule!(Servo_CssRules_GetFontFaceRuleAt:

speling
Comment on attachment 8866217 [details]
Bug 1328319 part 3 - Add support for auto value in nsCSSValue sugar.

https://reviewboard.mozilla.org/r/137804/#review141236

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:105
(Diff revision 1)
>              x => panic!("The unit should not be {:?}", x),
>          }
>      }
>  
> +    fn set_valueless_unit(&mut self, unit: nsCSSUnit) {
> +        debug_assert_eq!(self.mUnit, nsCSSUnit::eCSSUnit_Null);

I think all the other `set_` methods call destructors if not null. But I guess this is okay since all the nscssvalues we manipulate are null anyway. Might be better to document this or make the func name indicate it.
Attachment #8866217 - Flags: review?(manishearth) → review+
Comment on attachment 8866218 [details]
Bug 1328319 part 4 - Add binding for setting pair value of nsCSSValue.

https://reviewboard.mozilla.org/r/137808/#review141242
Attachment #8866218 - Flags: review?(manishearth) → review+
Comment on attachment 8866220 [details]
Bug 1328319 part 5 - Add bindings for list and pair list value of nsCSSValue.

https://reviewboard.mozilla.org/r/137812/#review141252

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:246
(Diff revision 1)
> +    pub fn set_pair_list<I>(&mut self, mut values: I)
> +    where I: ExactSizeIterator<Item=(nsCSSValue, nsCSSValue)> {
> +        debug_assert!(values.len() > 0, "Empty list is not supported");
> +        unsafe { bindings::Gecko_CSSValue_SetPairList(self, values.len() as u32); }
> +        let mut item_ptr = &mut unsafe {
> +            self.mValue.mPairList.as_ref() // &*nsCSSValuePairList_heap

debug assert that the type is still pairlist?
Comment on attachment 8866220 [details]
Bug 1328319 part 5 - Add bindings for list and pair list value of nsCSSValue.

https://reviewboard.mozilla.org/r/137812/#review141276
Attachment #8866220 - Flags: review?(manishearth) → review+
Comment on attachment 8866223 [details]
Relax the requirement of Atom::with.

https://reviewboard.mozilla.org/r/137818/#review141278

fnonce is stricter than fnmut, so this is ok
Attachment #8866223 - Flags: review?(manishearth) → review+
Comment on attachment 8866219 [details]
Small changes to AdditiveTuple struct.

https://reviewboard.mozilla.org/r/137810/#review141684
Attachment #8866219 - Flags: review?(simon.sapin) → review+
Comment on attachment 8866221 [details]
Change ToNsCssValue to take the ownership of value.

https://reviewboard.mozilla.org/r/137814/#review141686
Attachment #8866221 - Flags: review?(simon.sapin) → review+
Comment on attachment 8866222 [details]
Set atom identifier for counter style names in @counter-style rule.

https://reviewboard.mozilla.org/r/137816/#review141688
Attachment #8866222 - Flags: review?(simon.sapin) → review+
Comment on attachment 8866227 [details]
Bug 1328319 part 8 - Make stylo use counter-style for list-style-type and counter functions.

https://reviewboard.mozilla.org/r/137826/#review141698

::: servo/components/style/properties/longhand/list.mako.rs:70
(Diff revision 1)
> +                use gecko_bindings::structs;
> +                SpecifiedValue(if value == structs::NS_STYLE_LIST_STYLE_NONE {
> +                    CounterStyleOrNone::None_
> +                } else {
> +                    <%
> +                        values = """disc circle square decimal lower-roman

Why does this list not include other styles pre-defined in the spec, or other constants in layout/style/nsStyleConsts.h? Please add a code comment to explain what should be inculuded or not, and why.
Comment on attachment 8866227 [details]
Bug 1328319 part 8 - Make stylo use counter-style for list-style-type and counter functions.

https://reviewboard.mozilla.org/r/137826/#review141698

> Why does this list not include other styles pre-defined in the spec, or other constants in layout/style/nsStyleConsts.h? Please add a code comment to explain what should be inculuded or not, and why.

These are the styles that `type` attribute of `ol`, `ul`, and `li` can use, via mapped attribute mechanism. I'll add a comment about this.
Attachment #8866226 - Flags: review?(cam)
Comment on attachment 8866227 [details]
Bug 1328319 part 8 - Make stylo use counter-style for list-style-type and counter functions.

https://reviewboard.mozilla.org/r/137826/#review141924

r=me with the __atomic_store_ thing removed and the other comments addressed.

::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js:32
(Diff revision 1)
>          /memchr/,
>          "strlen",
>          // Assume that atomic accesses are threadsafe.
>          /^__atomic_fetch_/,
>          /^__atomic_load_/,
> +        /^__atomic_store_/,

Why do we need this?  I don't know if we should just blanket whitelist all atomic writes.

::: layout/style/ServoBindings.cpp:1125
(Diff revision 1)
>  {
>    aDst->mImageOrientation = aSrc->mImageOrientation;
>  }
>  
>  void
> -Gecko_SetListStyleType(nsStyleList* style_struct, uint32_t type)
> +Gecko_SetListStyleType(nsStyleList* style_struct, nsIAtom* name)

Can you adjust this to use Gecko-style argument naming while you're here?  (And below in Gecko_CopyListStyleTypeFrom.)

::: servo/components/style/properties/longhand/counters.mako.rs:44
(Diff revision 1)
> +            % if product == "servo":
> -            /// `counter(name, style)`.
> +                /// `counter(name, style)`.
> -            Counter(String, list_style_type::computed_value::T),
> +                Counter(String, list_style_type::computed_value::T),
> -            /// `counters(name, separator, style)`.
> +                /// `counters(name, separator, style)`.
> -            Counters(String, String, list_style_type::computed_value::T),
> +                Counters(String, String, list_style_type::computed_value::T),
> +            % else:
> +                /// `counter(name, style)`.
> +                Counter(String, CounterStyleOrNone),
> +                /// `counters(name, separator, style)`.
> +                Counters(String, String, CounterStyleOrNone),
> +            % endif

Would be better with less duplication, like:

  % counter_style_type = "list_style_type::computed_value::T" if product == "servo" else "CounterStyleOrNone"
  /// ...
  Counter(String, ${counter_style_type}),
  /// ...
  Counters(String, String, ${counter_style_type}),

::: servo/components/style/properties/longhand/counters.mako.rs:153
(Diff revision 1)
>      }
>  
> +    #[cfg(feature = "servo")]
> +    fn parse_counter_style(context: &ParserContext, input: &mut Parser) -> list_style_type::computed_value::T {
> +        input.try(|input| {
> +            try!(input.expect_comma());

May as well make this:

  input.expect_comma()?;

to be consistent with the other function.

::: servo/components/style/properties/shorthand/list.mako.rs:44
(Diff revision 1)
>                      any = true;
>                      continue
>                  }
>              }
> +
> +            if list_style_type.is_none() {

Do we do this last because we don't want it to eat up <custom-ident>s when they could match e.g. position keywords?  If so, please add a comment here saying that we this needs to be last.

::: servo/components/style/values/generics/mod.rs:80
(Diff revision 1)
> +
> +/// https://drafts.csswg.org/css-counter-styles/#typedef-counter-style
> +///
> +/// The spec doesn't include 'none' as part of syntax of <counter-style>
> +/// but wherever <counter-style> is used, 'none' is a valid value as
> +/// well, so putting it as part of CounterStyle makes code simpler.

Did you rename the type from CounterStyle to CounterStyleOrNone after writing this comment?
Attachment #8866227 - Flags: review?(cam) → review+
Comment on attachment 8866228 [details]
Bug 1328319 part 10 - Update test expectations.

https://reviewboard.mozilla.org/r/137828/#review141926
Attachment #8866228 - Flags: review?(cam) → review+
Comment on attachment 8866227 [details]
Bug 1328319 part 8 - Make stylo use counter-style for list-style-type and counter functions.

https://reviewboard.mozilla.org/r/137826/#review141924

> Why do we need this?  I don't know if we should just blanket whitelist all atomic writes.

I think this is because of the refcount things added in previous commit... which is probably no longer relevant.

> Did you rename the type from CounterStyle to CounterStyleOrNone after writing this comment?

Exactly :)
Blocks: 1363665
Attachment #8866226 - Attachment is obsolete: true
Simon, the patch has been updated to include a comment about the list. Could you review the patch again now?
Flags: needinfo?(simon.sapin)
Comment on attachment 8866227 [details]
Bug 1328319 part 8 - Make stylo use counter-style for list-style-type and counter functions.

https://reviewboard.mozilla.org/r/137826/#review142412
Attachment #8866227 - Flags: review?(simon.sapin) → review+
Looks good, thanks.
Flags: needinfo?(simon.sapin)
Depends on: 1364824
Depends on: 1360805
Depends on: 1364871
Comment on attachment 8867666 [details]
Bug 1328319 part 9 - Don't clean up retired counter styles for stylo.

https://reviewboard.mozilla.org/r/139250/#review142524
Attachment #8867666 - Flags: review?(cam) → review+
Servo side PR: servo/servo#16888
Attachment #8866219 - Attachment is obsolete: true
Attachment #8866221 - Attachment is obsolete: true
Attachment #8866222 - Attachment is obsolete: true
Attachment #8866223 - Attachment is obsolete: true
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/372c322963ad
part 1 - Move nsCSSCounterStyleRule into a separate header file. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9f336d8bfaa4
part 2 - Add basic integration of @counter-style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3ce2bdd8e485
part 3 - Add support for auto value in nsCSSValue sugar. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/e1804542ca96
part 4 - Add binding for setting pair value of nsCSSValue. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/04865ebdafcc
part 5 - Add bindings for list and pair list value of nsCSSValue. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/3425106bcdf0
part 6 - Make it possible to delay resolving counter style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/26e81401d8cd
part 7 - Enable querying counter-style rule on Servo backend. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d314bdf838bc
part 8 - Make stylo use counter-style for list-style-type and counter functions. r=heycam,SimonSapin
https://hg.mozilla.org/integration/autoland/rev/d62b198312ec
part 9 - Don't clean up retired counter styles for stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9fa32a431bc8
part 10 - Update test expectations. r=heycam
You need to log in before you can comment on or make changes to this bug.