966166 - Implement @counter-style rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Implement the @counter-style rule described in CSS Counter Styles Level 3.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: WIP patch

This WIP patch enables the browser to parse @counter-style rules and operate the rules in DOM. Since it is the first time I implement such a huge feature, I hope someone could have a brief glance at this patch, and tell me if there is any crucial problem in it, so that I can solve them as soon as possible before I go further in a wrong way.

Comment 2

[Gordon P. Hemsley [:GPHemsley]]

Comment on attachment 8372109 [details] [diff] [review]
WIP patch

@Xidorn Quan: When setting a needinfo/feedback/review flag like this, always specify a specific individual, as it makes it more likely that you will get a response.

@David: I think you're an appropriate person to give feedback on this patch to determine if it's heading in the right direction.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #2)
> Comment on attachment 8372109 [details] [diff] [review]
> WIP patch
> 
> @Xidorn Quan: When setting a needinfo/feedback/review flag like this, always
> specify a specific individual, as it makes it more likely that you will get
> a response.

I got it, thanks.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8372109 [details] [diff] [review]
WIP patch

I think the patch mostly looks good architecturally.

However, I'm a little unsure about one architectural issue in the implementation inside of nsCSSCounterStyleRule.  It looks rather like you've put the bulk of the implementation in the public API, rather than implementing the public API on top of an efficient internal API.

This leads to inefficiency like the macro-generated code here:
>+NS_IMETHODIMP
>+nsCSSCounterStyleRule::GetCssText(nsAString& aCssText)
>+{
>+  aCssText.AssignLiteral("@counter-style ");
>+  aCssText.Append(mName);
>+  aCssText.AppendLiteral(" {\n");
>+  nsAutoString tmp;
>+#define CSS_COUNTER_DESC(name_, method_)                                \
>+  if (mValues[eCSSCounterDesc_##method_].GetUnit() != eCSSUnit_Null) {  \
>+    Get##method_(tmp);                                                  \
>+    aCssText.AppendLiteral(#name_ ": ");                                \
>+    aCssText.Append(tmp);                                               \
>+    aCssText.AppendLiteral(";\n");                                      \
>+  }
>+#include "nsCSSCounterDescList.h"
>+#undef CSS_COUNTER_DESC
>+  aCssText.AppendLiteral("}");
>+  return NS_OK;
>+}

The macro-generated code could be fixed directly, by using an array of pointer-to-member functions generated from the descriptor list header.

Likewise, the larger macro-generated getters and setters should probably share a helper function and have less inside the macro (for less generated code).


If you've implemented most of what needs to be implemented in that class, then it's probably fine as is, with fixes like what I describe.  But if you're going to add a bit more to the implementation, then it might be worth having a stronger internal API like some of the other CSS storage does.


I didn't review line-by-line, though, since it's not clear to me that you wanted that level of feedback yet.

Sorry for the delay getting back to you, and thanks for taking on this project.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #4)
> If you've implemented most of what needs to be implemented in that class,
> then it's probably fine as is, with fixes like what I describe.  But if
> you're going to add a bit more to the implementation, then it might be worth
> having a stronger internal API like some of the other CSS storage does.

I'll fix them.

There is one problem which puzzles me for a long time: I want to make all the getters simply depend on nsCSSValue::AppendToString which, however, requires the property be provided to stringify some values. Though, the pairs in at-rules are called descriptors, which I don't think should be added into the list of properties. Nonetheless, I still think that method should be used instead of writing stringify code manually for some getters, which seems to be redundant. But I have no idea how to solve this. Would you mind giving me some suggestions?

Thanks for your informative feedback.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #5)
> There is one problem which puzzles me for a long time: I want to make all
> the getters simply depend on nsCSSValue::AppendToString which, however,
> requires the property be provided to stringify some values. Though, the
> pairs in at-rules are called descriptors, which I don't think should be
> added into the list of properties. Nonetheless, I still think that method
> should be used instead of writing stringify code manually for some getters,
> which seems to be redundant. But I have no idea how to solve this. Would you
> mind giving me some suggestions?

That's one of the reasons it might be easier to just leave it as it is.

Maybe the best way would be to add another nsCSSValue::AppendToString overload, taking a counter-style descriptor?

Are you planing to add more to the counter style rule implementation, or is it mostly done as of this patch?  If it's mostly done, it might be better to leave it the way you have it now and just fix up the issues with overuse of macros.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #6)
> That's one of the reasons it might be easier to just leave it as it is.
> 
> Maybe the best way would be to add another nsCSSValue::AppendToString
> overload, taking a counter-style descriptor?
> 
> Are you planing to add more to the counter style rule implementation, or is
> it mostly done as of this patch?  If it's mostly done, it might be better to
> leave it the way you have it now and just fix up the issues with overuse of
> macros.

No, the parser and interface part has all been done in this patch, and no new attribute will be added. I have been planing to start implementing the rendering part.

As you said, I'll leave it as it is for now.

However, it might be necessary soon to consider a more elegant way, since an increasing number of CSS interfaces will use attributes like this, such as css3-fonts.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: WIP patch 1

This is the revised version of the patch I uploaded last time. Abuse of macro should have been solved in this version. I believe it has been very close to the final version of patch 1.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: WIP patch 2

This is the second patch for this bug. I just finished it, and since currently there is no code links to this part, I haven't tested whether they are correct. I want some feedback about whether it is in good structure, and hope to know is there any suggestion about my code.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

I have two questions about the nsCSSValue:

1. Why the parameter of string value constructor of nsCSSValue is "const nsString&", not "const nsAString&"?
2. Is there any helper function to build different type of nsCSSValue, like MakeStringCSSValue, MakeIntegerCSSValue, and so on. It is boring to write eCSSUnit_* over and over again. They make code long.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #10)
> I have two questions about the nsCSSValue:
> 
> 1. Why the parameter of string value constructor of nsCSSValue is "const
> nsString&", not "const nsAString&"?

nsAString is a deprecated name (an alias for nsSubstring).  The two primary string classes are nsSubstring and nsString.  nsCSSValue could perhaps be changed to take nsSubstring, although it is more efficient to pass an nsString since nsCSSValue requires a null-terminated buffer.

> 2. Is there any helper function to build different type of nsCSSValue, like
> MakeStringCSSValue, MakeIntegerCSSValue, and so on. It is boring to write
> eCSSUnit_* over and over again. They make code long.

No.  I wouldn't expect you to have to write it too many times, though.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #11)
> (In reply to Xidorn Quan from comment #10)
> > 1. Why the parameter of string value constructor of nsCSSValue is "const
> > nsString&", not "const nsAString&"?
> 
> nsAString is a deprecated name (an alias for nsSubstring).  The two primary
> string classes are nsSubstring and nsString.

So nsSubstring is a preferable name, is it right?

> > 2. Is there any helper function to build different type of nsCSSValue, like
> > MakeStringCSSValue, MakeIntegerCSSValue, and so on. It is boring to write
> > eCSSUnit_* over and over again. They make code long.
> 
> No.  I wouldn't expect you to have to write it too many times, though.

So you may provide some suggestion for my second patch, in which I use them many times. I just find it convenient to use such structure for storing various type of values.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #12)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #11)
> > (In reply to Xidorn Quan from comment #10)
> > > 1. Why the parameter of string value constructor of nsCSSValue is "const
> > > nsString&", not "const nsAString&"?
> > 
> > nsAString is a deprecated name (an alias for nsSubstring).  The two primary
> > string classes are nsSubstring and nsString.
> 
> So nsSubstring is a preferable name, is it right?

Over nsAString, yes.  But in this case I'd be inclined to leave it as nsString, although maybe I'll see a good reason to change.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8380326 [details] [diff] [review]
WIP patch 1

Seems like you addressed my previous comments well.

I'm not sure why you're waiting to ask for review on this one.  Since you didn't ask for review, I didn't do a line-by-line review yet, though.  (I'd rather not do that more than once.)

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #14)
> Comment on attachment 8380326 [details] [diff] [review]
> WIP patch 1
> 
> Seems like you addressed my previous comments well.
> 
> I'm not sure why you're waiting to ask for review on this one.  Since you
> didn't ask for review, I didn't do a line-by-line review yet, though.  (I'd
> rather not do that more than once.)

Because it is still changing. Once all the main parts has been done (which means @counter-style rule can stably work), I'll post them all together here for review. There will be some relatively trivial patches for cleanup and test after those patches, which I will request for review separately.

At the moment, I have finished the final part of the main ones, and started doing some primary tests. Hope I can submit them soon :)

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

I have a question about the style struct. We know that, currently nsStyleList::mListStyleType is an uint8_t, and I'll change it to nsString so that it could accept custom counter styles. My question is: may I change the type to CounterStyleData* directly, so that frames could use the struct directly instead of looking-up the style table separately?

I didn't find such usage in nsStyleStruct.h. Animationsa are stored only by name. Since they are not inheritable, there may not be a large effect on performance. But if each list item needs to do a lookup separately, it could be a problem.

If the nsPresContext which owns nsCounterStyleManager always overlives the style structures, I think it is not a bad idea to store the data pointer in such structure. What's your opinion?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8380327 [details] [diff] [review]
WIP patch 2

nsBulletFrame.cpp:

  I'd prefer not to use extern.  Can you use static class members
  instead?

nsStyleConsts.h:

  Put the -1 at the beginning so the NS_STYLE_LIST_STYLE_* constants are
  in numeric order, so that somebody doesn't try adding a new -1 at the
  start of the list.

moz.build:

  I don't think you need to put nsCounterStyleManager.h in the EXPORTS
  list; it doesn't seem like it will be used outside of layout.

nsCounterStyleManager.h:
nsCounterStyleManager.cpp:

  I guess it's not clear to me why you wrote the whole thing using
  nsCSSValue.  And, perhaps, at a higher level, it's not clear to me why
  you put all the computation code in a GetComputedDescValue method,
  which then has a switch over the possible descriptors (same in the
  methods it calls).  The only code shared across descriptors in all of
  that code seems to be the middle bit of GetComputedDescValue.

  Maybe there's a good reason for that approach that I'm not seeing,
  though.

  But what would be the problem with writing the code in separate
  methods (GetRange, GetPad, GetNegative) or merging it into
  GetFallback, GetPrefix, and GetSuffix, and then having a shared
  function for that little bit of shared code if needed?  The advantages
  of this approach would be returning the expected types rather than
  tagged unions with a tag you already know, and less having to go
  through the same switch statements multiple times.

  Also, in general, you should avoid having function return types be
  struct types like nsCSSValue that have expensive copy-constructors.
  Instead, you should take references as an "out-parameter" (generally
  the last parameter).

  I didn't look all that closely at the code yet, though.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #16)
> I have a question about the style struct. We know that, currently
> nsStyleList::mListStyleType is an uint8_t, and I'll change it to nsString so
> that it could accept custom counter styles. My question is: may I change the
> type to CounterStyleData* directly, so that frames could use the struct
> directly instead of looking-up the style table separately?
> 
> I didn't find such usage in nsStyleStruct.h. Animationsa are stored only by
> name. Since they are not inheritable, there may not be a large effect on
> performance. But if each list item needs to do a lookup separately, it could
> be a problem.

I think it's better to store something that's more useful to the code using it, so a CounterStyleData* would be more useful than just a name.  (You'll still need the name for computed style.)

> If the nsPresContext which owns nsCounterStyleManager always overlives the
> style structures, I think it is not a bad idea to store the data pointer in
> such structure. What's your opinion?

The nsPresContext will outlive the style structs, but that doesn't mean that the @counter-style rule will.  (How do you handle dynamic changes to @counter-style rules?)

So when would a CounterStyleData change, once you handle dynamic changes correctly?

And how cheap is it to either copy or reference-count them?  Given that this is an inherited property, copying from parent to child needs to be cheap, both in time and memory.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #18)
> (In reply to Xidorn Quan from comment #16)
> > I have a question about the style struct. We know that, currently
> > nsStyleList::mListStyleType is an uint8_t, and I'll change it to nsString so
> > that it could accept custom counter styles. My question is: may I change the
> > type to CounterStyleData* directly, so that frames could use the struct
> > directly instead of looking-up the style table separately?
> > 
> > I didn't find such usage in nsStyleStruct.h. Animationsa are stored only by
> > name. Since they are not inheritable, there may not be a large effect on
> > performance. But if each list item needs to do a lookup separately, it could
> > be a problem.
> 
> I think it's better to store something that's more useful to the code using
> it, so a CounterStyleData* would be more useful than just a name.  (You'll
> still need the name for computed style.)

There's no problem with name, since it could be generated from the style data (either from the name attribute of the rule or the keyword value).
 
> > If the nsPresContext which owns nsCounterStyleManager always overlives the
> > style structures, I think it is not a bad idea to store the data pointer in
> > such structure. What's your opinion?
> 
> The nsPresContext will outlive the style structs, but that doesn't mean that
> the @counter-style rule will.  (How do you handle dynamic changes to
> @counter-style rules?)
> 
> So when would a CounterStyleData change, once you handle dynamic changes
> correctly?

In my current code, CounterStyleData will never be changed, and even nsCounterStyleManager will never destroy any CounterStyleData before itself being released. In fact, I'm not sure what will happen if there is dynamic changes. I guess nsCSSStyleSheet::SetModifiedByChildRule called in nsCSSCounterStyleRule::SetDesc will rebuild all things when changes happened, is it correct?

> And how cheap is it to either copy or reference-count them?  Given that this
> is an inherited property, copying from parent to child needs to be cheap,
> both in time and memory.

I don't want the style sheets to own CounterStyleData. What I want is to put a pointer in it, and always let nsCounterStyleManager own the data. Copying a pointer should be very cheap.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #19)
> In my current code, CounterStyleData will never be changed, and even
> nsCounterStyleManager will never destroy any CounterStyleData before itself
> being released. In fact, I'm not sure what will happen if there is dynamic
> changes. I guess nsCSSStyleSheet::SetModifiedByChildRule called in
> nsCSSCounterStyleRule::SetDesc will rebuild all things when changes
> happened, is it correct?

No, it doesn't rebuild everything.  It just rebuilds the cascading we've done for the style sheet.  There are various codepaths that trigger the necessary handling of dynamic changes.  Changes to style rule, for example, cause us to rerun selector matching, which in turn trigger the necessary style change handling.  (The March 11, 2012 talk listed on http://dbaron.org/talks/ might be useful here.)

You need to figure out how dynamic updates ought to work for @counter-style rules.  It's not magic, and you do need to have a plan.  It's worth considering both efficiency and code-complexity, but efficiency matters less for things that will happen rarely (which dynamic changes to @counter-style rules probably will).

> > And how cheap is it to either copy or reference-count them?  Given that this
> > is an inherited property, copying from parent to child needs to be cheap,
> > both in time and memory.
> 
> I don't want the style sheets to own CounterStyleData. What I want is to put
> a pointer in it, and always let nsCounterStyleManager own the data. Copying
> a pointer should be very cheap.

When does nsCounterStyleManager know it needs to create and destroy the data?  And how many objects would be created?

(Also, I was talking about style structs owning the data, which are computed style, not style sheets, which are specified style.)

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - modify tests

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - rewrite some builtin counter styles

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

The patchset I just submitted has passed nearly all current tests, with some unrelated failures and one related failure in some platforms. https://tbpl.mozilla.org/?tree=Try&rev=adb1336b1ec3

The only related failure in it is the failure in Android 2.2 R2 and Android 4.0 R4. This problem occurs in all platform in some old version, and it should have been fixed. So I have no idea why it still failed in the two specific platforms. Other failures seems to be unrelated with my patchset.

I haven't write any specific test for the new function. However, as a bunch of previous builtin counter styles have been rewrited using the new syntax, and all related tests has been passed, I believe the current code has reached a usable state.

The most outstanding omission of this patchset is process of dynamic changes to counter style rules, which I will start working on soon.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8384119 [details] [diff] [review]
patch 2 - computation of counter style

I'd like to rewrite this part using inheritance instead of a bunch of checking.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8384122 [details] [diff] [review]
patch 5 - rewrite some builtin counter styles

The rewrite of patch 2 will influence this patch.

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8384120 [details] [diff] [review]
patch 3 - link to other parts

Rewrite of patch 2 may also affect this patch.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - rewrite some builtin counter styles

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - fix fallback problem introduced in patch 2

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

try server result of the latest patchset: https://tbpl.mozilla.org/?tree=Try&rev=a09beb698d5b

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Sorry for uploading a wrong patch.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To continue the discussion on dynamic changes that started in bug 978648, I believe we handle dynamic changes on @media and @font-face well.  See, for example, the BEGIN_MEDIA_CHANGE/END_MEDIA_CHANGE macros, or bug 457821 (and the current version of the code added there) for @font-face.  (They require different sorts of handling; I suspect what you want here is probably more like @font-face.)

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #36)
> To continue the discussion on dynamic changes that started in bug 978648, I
> believe we handle dynamic changes on @media and @font-face well.  See, for
> example, the BEGIN_MEDIA_CHANGE/END_MEDIA_CHANGE macros, or bug 457821 (and
> the current version of the code added there) for @font-face.  (They require
> different sorts of handling; I suspect what you want here is probably more
> like @font-face.)

Thanks very much for your information.

Given that I'll continue changing some of the patches to implement the dynamic change, you may review the modification to tests, which is patch 4, first. There is few chance for it to be influenced by other parts.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, the dynamic changes I'm most concerned about handling are the addition and removal (or enabling and disabling) of style sheets that contain @counter-style rules.

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - rewrite some builtin counter styles

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - move builtin styles code

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

The new patchset has implemented all types of dynamic changes of counter-style rule. Patch 4 & 6 are unchanged in this set.

try server result: https://tbpl.mozilla.org/?tree=Try&rev=d47f0611e447
The result has been explained in comment 26.

I'll start working on testcases then.

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8387425 [details] [diff] [review]
patch 2 - computation of counter style

Several problems have been found in this patch. The most important one is the performance issue of fallback and range.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8387417 [details] [diff] [review]
patch 1 - parse @counter-style rule

So based on
http://lists.w3.org/Archives/Public/www-style/2014Jan/0610.html
I think we're probably ok shipping this feature by default, although it
would be nice if the CR were actually published before we do so.

In order to do that, you should probably send an intent to ship email as
described in https://wiki.mozilla.org/WebAPI/ExposureGuidelines ,
though.

It definitely requires more tests than you currently have to land this.
You need tests that test the new features.  It would actually be helpful
to see the tests when reviewing the patch, since then I can look at the
tests to make sure you've tested things that I think need testing, as
I'm reviewing the patch.

In theory, our rapid release plan requires that new features be easy to
disable or back out; we normally accomplish that by having a pref to
disable them.  That said, I think it's probably ok not to have a pref
here; the disabling plan could be to back it out.

Is my understanding correct that you're implementing everything in the
css-counter-styles spec except for the symbols() function?

I think it would also be helpful to merge the changes in part 4 into the
patches that require the respective changes.  It's generally good if
patches in a series all compile and pass tests, although it's
occasionally acceptable to do otherwise.  (The piece that says it
requires review from a DOM peer actually requires review from a DOM
peer.)


I'm reviewing this relative to the css-counter-styles spec as of
https://dvcs.w3.org/hg/csswg/rev/6f59b9da1892 .


nsCSSParser:
 - it's not clear what the aURL parameter is to your new methods
   on the public nsCSSParser interface.  Other methods take separate
   aSheetURI and aBaseURI methods, although a few (which don't need a
   base URL) don't.

   However, ParseCounterDescriptor definitely needs a correct base URL,
   so it needs separate aSheetURI and aBaseURI parameters.  You should
   add a test for correct behavior here.

   I suspect ParseCounterStyleName is probably ok, but we should
   probably actually have code to guarantee that (i.e., code that would
   cause assertions in the cases where we think we don't need a base URL
   if we actually end up needing one).

  In ParseCounterStyleRule, this code:

  >+  if (!ExpectSymbol('}', true)) {
  >+    REPORT_UNEXPECTED_TOKEN(PECounterStyleBadBlockEnd);
  >+    return false;
  >+  }

  appears to be unreachable.  ExpectSymbol('}') will return true
  for both a } and an EOF.  So you should remove it, and the associated
  error message.

  And then you should add a second REPORT_UNEXPECTED_EOF(PECounterStyleEOF)
  in the case here where SkipDeclaration returns false:

  >+    if (!ParseCounterDescriptor(rule)) {
  >+      REPORT_UNEXPECTED(PEDeclSkipped);
  >+      OUTPUT_ERROR();
  >+      if (!SkipDeclaration(true)) {
  >+        break;
  >+      }
  >+    }

  >+      auto& symbols = rule->GetDesc(eCSSCounterDesc_Symbols);

  It seems unnecessary to use auto for nsCSSValue.

  >+  if (isCorrect) {
  >+    (*aAppendFunc)(rule, aData);
  >+  }

  Where does the spec say that an @counter-style rule is syntactically
  valid only when it meets the above criteria?  (I haven't reviewed those
  checks because I don't know what I should be reviewing them against.)


I'm up to that point in the patch; I'm inclined to wait for the tests before
reviewing further, though.

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #46)
>
> Is my understanding correct that you're implementing everything in the
> css-counter-styles spec except for the symbols() function?

No, I'm only implementing the @counter-style rule. Besides, most of the predefined counter styles are also included since their definition can be directly copied from the spec. The exceptions are disclosure-{open,closed} and ethiopic-numeric. For the former, I have open bug 982355 to track, and we have had a prefixed version for the latter, which should be checked and unprefixed. I think it should be in another new bug as well.

In addition, the current implementation also lacks image symbol support. I currently have no plan to add it as I need to do more investigation to known how to implement this feature.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, I guess I should have said "everything in the spec except for symbols() and for changing/reimplementing the existing counter styles in terms of @counter-style".

Presumably you're also implementing the changes to list-style-type, counter(), and counters() that add lookup for styles defined in @counter-style rules, right?

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #48)
> Sorry, I guess I should have said "everything in the spec except for
> symbols() and for changing/reimplementing the existing counter styles in
> terms of @counter-style".
> 
> Presumably you're also implementing the changes to list-style-type,
> counter(), and counters() that add lookup for styles defined in
> @counter-style rules, right?

That's right. If not, the whole thing is useless, isn't it :)

Changes to those parts are in patch 3.

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #46)
> Comment on attachment 8387417 [details] [diff] [review]
> patch 1 - parse @counter-style rule
> 
> So based on
> http://lists.w3.org/Archives/Public/www-style/2014Jan/0610.html
> I think we're probably ok shipping this feature by default, although it
> would be nice if the CR were actually published before we do so.
> 
> In order to do that, you should probably send an intent to ship email as
> described in https://wiki.mozilla.org/WebAPI/ExposureGuidelines ,
> though.

Need I also send an intent to implement according to the guidelines?

> I think it would also be helpful to merge the changes in part 4 into the
> patches that require the respective changes.  It's generally good if
> patches in a series all compile and pass tests, although it's
> occasionally acceptable to do otherwise.  (The piece that says it
> requires review from a DOM peer actually requires review from a DOM
> peer.)

OK.

> nsCSSParser:
>  - it's not clear what the aURL parameter is to your new methods
>    on the public nsCSSParser interface.  Other methods take separate
>    aSheetURI and aBaseURI methods, although a few (which don't need a
>    base URL) don't.
> 
>    However, ParseCounterDescriptor definitely needs a correct base URL,
>    so it needs separate aSheetURI and aBaseURI parameters.  You should
>    add a test for correct behavior here.

Yes, you are right. I didn't realize the meaning of such parameters, and just copied what other similar methods are defined. I realized that ParseCounterDescriptor requires a correct base URL to parse url(), however the current implementation does not support that kind of value. So I don't have an idea what kind of test should I add to check the behavior? Maybe we could add such test in the future when we support url()?

>    I suspect ParseCounterStyleName is probably ok, but we should
>    probably actually have code to guarantee that (i.e., code that would
>    cause assertions in the cases where we think we don't need a base URL
>    if we actually end up needing one).

If my understanding is correct, a base URL is only necessary for parsing url(), is it right? If so, it is obvious that we don't need the base URL. Besides, what kind of assertion do you think we should add to guarantee that? I didn't find any similar code.

> 
>   In ParseCounterStyleRule, this code:
> 
>   >+  if (!ExpectSymbol('}', true)) {
>   >+    REPORT_UNEXPECTED_TOKEN(PECounterStyleBadBlockEnd);
>   >+    return false;
>   >+  }
> 
>   appears to be unreachable.  ExpectSymbol('}') will return true
>   for both a } and an EOF.  So you should remove it, and the associated
>   error message.

Actually, it was reachable, as the '}' will be ungotten when detected in loop. As it seems to be unnecessary, I'll remove this block as well as the UngetToken before break.

>   And then you should add a second REPORT_UNEXPECTED_EOF(PECounterStyleEOF)
>   in the case here where SkipDeclaration returns false:
> 
>   >+    if (!ParseCounterDescriptor(rule)) {
>   >+      REPORT_UNEXPECTED(PEDeclSkipped);
>   >+      OUTPUT_ERROR();
>   >+      if (!SkipDeclaration(true)) {
>   >+        break;
>   >+      }
>   >+    }
> 
>   >+      auto& symbols = rule->GetDesc(eCSSCounterDesc_Symbols);
> 
>   It seems unnecessary to use auto for nsCSSValue.

I just want to make it shorter. Are you unhappy with it? I don't think it is a big problem as auto is allowed.

> 
>   >+  if (isCorrect) {
>   >+    (*aAppendFunc)(rule, aData);
>   >+  }
> 
>   Where does the spec say that an @counter-style rule is syntactically
>   valid only when it meets the above criteria?  (I haven't reviewed those
>   checks because I don't know what I should be reviewing them against.)

In each section 3.1.*. Each system is described separately. It says "the @counter-style rule is invalid" if such criteria are not met.

Comment 51

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule

Mostly same with the previous version with some modification according to comment 46.

Comment 52

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

Correct some bugs found by tests.

Comment 53

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Fix bug & integrate test modification

Comment 54

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - rewrite some builtin counter styles

This should be identical to the previous version except the part number.

Comment 55

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - fix fallback problem introduced in patch 2

Change part number.

Comment 56

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - move builtin styles code

Comment 57

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 (WIP) - tests

I have done the reftests. dbaron, you can continue the review with these tests.

Comment 58

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule

Sorry, I forgot to refresh the patch...

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #50)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #46)
> > In order to do that, you should probably send an intent to ship email as
> > described in https://wiki.mozilla.org/WebAPI/ExposureGuidelines ,
> > though.
> 
> Need I also send an intent to implement according to the guidelines?

One email is fine.

> have an idea what kind of test should I add to check the behavior? Maybe we
> could add such test in the future when we support url()?

Yes, that's fine.

> >    I suspect ParseCounterStyleName is probably ok, but we should
> >    probably actually have code to guarantee that (i.e., code that would
> >    cause assertions in the cases where we think we don't need a base URL
> >    if we actually end up needing one).
> 
> If my understanding is correct, a base URL is only necessary for parsing
> url(), is it right? If so, it is obvious that we don't need the base URL.
> Besides, what kind of assertion do you think we should add to guarantee
> that? I didn't find any similar code.

Code that tracks (#ifdef DEBUG) whether we initialized the scanner using a method that takes separate sheet and base URLs, and then asserts if we try to do something with the base URL object if we came through one of the codepaths that doesn't.

> Actually, it was reachable, as the '}' will be ungotten when detected in
> loop. As it seems to be unnecessary, I'll remove this block as well as the
> UngetToken before break.

Oops, sorry about that.  Your solution sounds great.

> >   >+      auto& symbols = rule->GetDesc(eCSSCounterDesc_Symbols);
> > 
> >   It seems unnecessary to use auto for nsCSSValue.
> 
> I just want to make it shorter. Are you unhappy with it? I don't think it is
> a big problem as auto is allowed.

I'd prefer not to use auto for such simple types.

Comment 60

[Xidorn Quan [:xidorn] UTC+10]

try server: https://tbpl.mozilla.org/?tree=Try&rev=887b04d9bdd1

Comment 61

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #59)
> (In reply to Xidorn Quan from comment #50)
> > (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #46)
> > > In order to do that, you should probably send an intent to ship email as
> > > described in https://wiki.mozilla.org/WebAPI/ExposureGuidelines ,
> > > though.
> > 
> > Need I also send an intent to implement according to the guidelines?
> 
> One email is fine.

Done.

> > If my understanding is correct, a base URL is only necessary for parsing
> > url(), is it right? If so, it is obvious that we don't need the base URL.
> > Besides, what kind of assertion do you think we should add to guarantee
> > that? I didn't find any similar code.
> 
> Code that tracks (#ifdef DEBUG) whether we initialized the scanner using a
> method that takes separate sheet and base URLs, and then asserts if we try
> to do something with the base URL object if we came through one of the
> codepaths that doesn't.

It seems to be a large project as there has been many methods initialize scanner without a valid base URL. I think it should be better to open another bug to do this for all such methods.

Comment 62

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8389791 [details] [diff] [review]
patch 1 - parse @counter-style rule

As it says review from a DOM peer is necessary, bz, can you review this patch?

Comment 63

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

Solve an accessibly-related bug found by test. Part of processing speak-as is changed.

Comment 64

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Comment 65

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - rewrite some builtin counter styles

Affected by change in patch 2.

Comment 66

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - move builtin styles code

Affected by changes in patch 2.

Comment 67

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style

Compared to the WIP version, this version adds a11y test and insertRule test. Do we need an a11y peer to review this patch in addition?

try server: https://tbpl.mozilla.org/?tree=Try&rev=a717b7d9590b

Comment 68

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8389791 [details] [diff] [review]
patch 1 - parse @counter-style rule

r=me on the DOM/classinfo bits

Comment 69

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Xidorn Quan from comment #54)
> Created attachment 8389784 [details] [diff] [review]
> patch 4 - rewrite some builtin counter styles
> 
> This should be identical to the previous version except the part number.

FYI, if the patch doesn't have any changes, you can simply rename it by going to Details > Edit Details.

Comment 70

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #69)
> (In reply to Xidorn Quan from comment #54)
> > Created attachment 8389784 [details] [diff] [review]
> > patch 4 - rewrite some builtin counter styles
> > 
> > This should be identical to the previous version except the part number.
> 
> FYI, if the patch doesn't have any changes, you can simply rename it by
> going to Details > Edit Details.

Thanks for your information. I just wanted to update the message in the patches as well, though it seems to be meaningless...

BTW, ping for review. There has been more than a week since my last update.

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, was in a work week all last week and busy pretty much the whole time; I'll get to it this week.  (I'll try for today, but not sure about that.)

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm most of the way through reviewing patch 1 at this point.

Comment 73

[Xidorn Quan [:xidorn] UTC+10]

One potential change to patch 1 and 2 could be the case-sensitivity problem I just posted on the www-style mailing list. The current implementation could break the back-compatibility as refering predefined styles with a name containing upper-case characters could have an inconsistent behavior.

Comment 74

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule, r=bz

This patch solves the case-sensitivity problem.

Comment 75

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

This version removes some useless conditions.

Comment 76

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Solve the case-sensitivity problem.

Comment 77

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - move builtin styles code

This version merges patch from bug 985186.
It also removes some literal constants, and modifies test accordingly.

Comment 78

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style (WIP)

Add one test for name case-sensitivity.

Comment 79

[Xidorn Quan [:xidorn] UTC+10]

try server for the new patchset: https://tbpl.mozilla.org/?tree=Try&rev=13e0f5ce514c

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #74)
> This patch solves the case-sensitivity problem.

It would be helpful for getting the reviews done if you did additional fixes in new patches rather than modifying the existing ones.  (Going forward; I think I'll deal with this one, though I haven't yet diff'd the patches.)

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8389791 [details] [diff] [review]
patch 1 - parse @counter-style rule

Here are my review comments that I had on this patch (which is no longer the latest).  I was up to nsCSSCounterStyleRule::SetName.

Please do NOT try to address the review comments until I'm done going through the patch, though; that will just make it harder to complete the review.  (I normally wouldn't post them until I'm done, but now that I'm dealing with reviewing part of one version of the patch and part of another version, I'm going to post them.)

I'm reviewing this relative to the css-counter-styles spec as of
https://dvcs.w3.org/hg/csswg/rev/342406a97c8a .


nsIDOMCSSCounterStyleRule.idl:

 - maybe list speakAs and fallback in the same order as the spec?


css.properties:

>+PECounterOverrideNotIdent=Expected identifier for override system.

this needs a "but found '%1$S'" somewhere

>+PECounterASWeight=Expected descending order weight.

I think this message could be clearer.


nsCSSParser.cpp:

I think ParseCounterDescriptor should probably take an aSheetPrincipal
argument like ParseProperty does, and should pass it to InitScanner.

In ParseCounterStyleRule, it seems like it would make more sense to
initialize this:
>+  bool isCorrect = true;
to false, and also to assert in the default: case of the switch.

ParseCounterStyleRule may need to be adjusted to deal with the response
to http://lists.w3.org/Archives/Public/www-style/2014Mar/0543.html

In ParseCounterDescriptor(nsCSSCounterStyleRule*):

>+  if (descID == eCSSCounterDesc_UNKNOWN) {
>+    if (NonMozillaVendorIdentifier(descName)) {
>+      SkipDeclaration(true);
>+      return true;
>+    } else {
>+      REPORT_UNEXPECTED_P(PEUnknownCounterDesc, descName);
>+      return false;
>+    }
>+  }

Whether it's another vendor's prefix should only affect whether we
report an error, not how we behave.  I realize the current code actually
handles this correctly, but it looks dangerous for future modifications.
Furthermore, I think the likelihood of vendor prefixes becoming common
in @counter-style rules is low, so I think you should just skip the
vendor-prefix check and just report the error for all unknown
descriptors.  So please delete the NonMozillaVendorIdentifier check,
and use only the else codepath instead.

Your current code implements what I propose for the spec in 
http://lists.w3.org/Archives/Public/www-style/2014Mar/0544.html
but if the spec ends up being different, you may need to change it.

In ParseCounterDescriptorValue:

In the parsing of eCSSCounterDesc_Range:

>+      if (CheckEndProperty()) {
>+        break;
>+      }
>+      if (!ExpectSymbol(',', true)) {
>+        return false;
>+      }

There's no need to use CheckEndProperty() here (which I prefer to
avoid as much as possible, since it's really incorrect to use it at all
from the perspective of how recursive descent parsing ought to work).

Instead, you can just write:
>      if (!ExpectSymbol(',', true)) {
>        return true;
>      }
and rely on the caller to fail if there's garbage afterwards (as we
already do for most of the other descriptors).

This also lets you drop the "return true" after the loop, since the
loop now never exits other than by returning.

For eCSSCounterDesc_Pad:

Your code here isn't sufficient; to handle an "&&" in the grammar,
you need to handle the values in either order.  Please fix this, e.g.,
by replacing:
>+    if (!ParseVariant(width, VARIANT_INTEGER, nullptr) ||
>+        !ParseVariant(symbol, VARIANT_COUNTER_SYMBOL, nullptr)) {
>+      return false;
>+    }
with:
>    bool haveInteger = ParseVariant(width, VARIANT_INTEGER, nullptr);
>    if (!ParseVariant(symbol, VARIANT_COUNTER_SYMBOL, nullptr) ||
>        (!haveInteger && !ParseVariant(width, VARIANT_INTEGER, nullptr))) {
>      return false;
>    }

Please add a test for this as well.

For eCSSCounterDesc_Fallback:

If the response to
http://lists.w3.org/Archives/Public/www-style/2014Mar/0554.html is not
what I expect, you might need to check that the name is not 'decimal' or
'none' before succeeding.  Probably the best way to do this would be by
calling ParseCounterStyleName (since that's the spec's logic).

Please add a test for this as well.

For eCSSCounterDesc_Symbols:

>+      if (!ParseVariant(item->mValue, VARIANT_COUNTER_SYMBOL, nullptr)) {
>+        return false;
>+      }
>+      if (CheckEndProperty()) {
>+        break;
>+      }

Instead of using CheckEndProperty, use a "bool first", i.e., replacing
the whole case with:

>    nsCSSValueList* item;
>    bool first = true;
>    for (;;) {
>      nsCSSValue itemValue;
>      if (!ParseVariant(itemValue, VARIANT_COUNTER_SYMBOL, nullptr)) {
>        return !first;
>      }
>      if (first) {
>        first = false;
>        item = aValue.SetListValue();
>      } else {
>        item->mNext = new nsCSSValueList;
>        item = item->mNext;
>      }
>      item->mValue = itemValue;
>    }

(Note again the removal of the final return.)

For eCSSCounterDesc_AdditiveSymbols:

All of the changes for eCSSCounterDesc_Range **and** those for
eCSSCounterDesc_Pad apply here as well.  (Note *not*
eCSSCounterDesc_Symbols.)

Again, please add tests for both sets of changes.

For eCSSCounterDesc_SpeakAs:

If the response to
http://lists.w3.org/Archives/Public/www-style/2014Mar/0554.html is not
what I expect, you might need to adjust this as well.

>+  default:
>+    return false;

This should also assert (MOZ_ASSERT(false, "unexpected descriptor"))
before returning false.

nsCSSRuleProcessor.cpp:

>+      for (nsTArray<nsCSSCounterStyleRule*>::size_type i = 0,
>+            iEnd = newCascade->mCounterStyleRules.Length(); i < iEnd; ++i) {

Please indent either 0 or 2 spaces from the (, not 1.

nsCSSRules.h:

I'm inclined to ask you to rename mRevision to mGeneration, just because
we tend to use the name "generation" more often for that sort of thing.
(Same for GetRevision(), etc.)

>+    NS_ABORT_IF_FALSE(aDescID > eCSSCounterDesc_UNKNOWN && 
>+                      aDescID < eCSSCounterDesc_COUNT,
>+                      "descriptor ID out of range");

Given that you're indexing into an array, I think it's better to
write >= 0 rather than > UNKNOWN, even though you know UNKNOWN is -1.

>+  nsresult GetAttribute(nsCSSCounterDesc aDescID, nsAString& aValue);
>+  nsresult SetAttribute(nsCSSCounterDesc aDescID, const nsAString& aValue);

Calling these GetAttribute and SetAttribute doesn't make sense.  You should
either:
 * call them GetDesc and SetDesc (overloads for that name), or
 * call them GetDescriptor and SetDescriptor

nsCSSRules.cpp:

In GetCssText:

>+  aCssText.Append(mName);

You need nsStyleUtil::AppendEscapedCSSIdent here.

>+  for (nsCSSCounterDesc id = nsCSSCounterDesc(eCSSCounterDesc_UNKNOWN + 1);

just start at 0.

>+      aCssText.AppendASCII(nsCSSProps::GetStringValue(id).get());

Make this:
  AppendASCIItoUTF16(nsCSSProps::GetStringValue(id), aCssText);
instead.

Comment 82

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #80)
> (In reply to Xidorn Quan from comment #74)
> > This patch solves the case-sensitivity problem.
> 
> It would be helpful for getting the reviews done if you did additional fixes
> in new patches rather than modifying the existing ones.  (Going forward; I
> think I'll deal with this one, though I haven't yet diff'd the patches.)

I'm sorry.

BTW, as you haven't started reviewing patch 3, I'll modify it according to http://lists.w3.org/Archives/Public/www-style/2014Mar/0584.html

Comment 83

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Explicitly reject list-style if more than one position keywords are presented.

Comment 84

[Xidorn Quan [:xidorn] UTC+10]

integer-range and ua-limits reftests keep failing in ARM platforms because of bug 989718.

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8398893 [details] [diff] [review]
patch 1 - parse @counter-style rule, r=bz

comments on the diffs between attachment 8389791 [details] [diff] [review] and attachment 8398893 [details] [diff] [review]:

I'm glad to see the ParseCounterStyleName boolean parameter; it will
help address my review comments above as well.

When calling ParseCounterStyleName at your three new callers (which
convert the result to an nsCSSValue), just use nsString rather than
nsAutoString.  The nsCSSValue requires a reference-counted string buffer
anyway, so you may as well get it created sooner by the nsString rather
than having to do an extra copy out of the nsAutoString's stack buffer.

I think kCSSRawPredefinedCounterStyles should also include "none"
and "cjk-ideographic".  (Does it make sense for it to also include
the -moz- prefixed list styles that we currently support?)

I'm not sure this really needs an nsStaticCaseInsensitiveNameTable,
but I suppose it's ok.

FOLLOWUP: As a followup bug, would you mind refactoring the table
initialization code in nsCSSProps::AddRefTable into a function so it's
not copy-pasted four times?

And could you make your nsCSSProps::EnabledState operator| cast to
a typedef of EnabledState_size_t that's defined right next to
EnabledState, and make the typedef int32_t rather than just int?

Comment 86

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #85)
> I think kCSSRawPredefinedCounterStyles should also include "none"
> and "cjk-ideographic".  (Does it make sense for it to also include
> the -moz- prefixed list styles that we currently support?)

Oops, I forgot that two since they were not listed in the table of content.

I think whether we should add the prefixed styles depends on our attitude to compatibility of those styles. If we don't need to keep it perfectly compatible, they should not be included in the list as it increases the burden when we decide to remove them. Otherwise, they should. Anyway, I don't think there's a large impact to exclude them.

> FOLLOWUP: As a followup bug, would you mind refactoring the table
> initialization code in nsCSSProps::AddRefTable into a function so it's
> not copy-pasted four times?

Should I do it directly in the patch 1 or have a separate patch for it?

> And could you make your nsCSSProps::EnabledState operator| cast to
> a typedef of EnabledState_size_t that's defined right next to
> EnabledState, and make the typedef int32_t rather than just int?

Well, it's not my modification, it is a change of context.

Comment 87

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #86)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #85)
> > FOLLOWUP: As a followup bug, would you mind refactoring the table
> > initialization code in nsCSSProps::AddRefTable into a function so it's
> > not copy-pasted four times?
> 
> Should I do it directly in the patch 1 or have a separate patch for it?

Not only a separate patch, but a separate bug.

> > And could you make your nsCSSProps::EnabledState operator| cast to
> > a typedef of EnabledState_size_t that's defined right next to
> > EnabledState, and make the typedef int32_t rather than just int?
> 
> Well, it's not my modification, it is a change of context.

Oh, sorry, never mind, then, I guess.

Comment 88

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, do you have tests for changes to @counter-style rules, similar to the tests for @keyframes rules added in https://hg.mozilla.org/mozilla-central/rev/1a34a6a07d71 ?  I didn't see them in a quick skim of patch 7, though I might be looking in the wrong place.

Comment 89

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #88)
> Also, do you have tests for changes to @counter-style rules, similar to the
> tests for @keyframes rules added in
> https://hg.mozilla.org/mozilla-central/rev/1a34a6a07d71 ?  I didn't see them
> in a quick skim of patch 7, though I might be looking in the wrong place.

I haven't done that... I'll work on it soon.

Comment 90

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8398893 [details] [diff] [review]
patch 1 - parse @counter-style rule, r=bz

I'm not confident about how well the dynamic change handling works; I'd
like to see more tests for that, and any bugs uncovered fixed in
additional patches (not modifications of this one!).

You should also have tests for your serialization code that test that
the results of serialization are valid inputs, and that they produce
the same result when reserialized.  And maybe some other things like
test_value_storage.html or test_descriptor_storage.html.  And you might
also want tests like test_descriptor_syntax_errors.html -- making it
worth having a counter_descriptor_database.js like descriptor_database.js.

Those should go in separate patches or be added to the tests patch if
I haven't gotten to it yet.


>+  auto& system = GetDesc(eCSSCounterDesc_System);

Please use "const nsCSSValue& system" (or value) throughout; don't use
auto for a "const nsCSSValue&".

>+nsCSSCounterStyleRule::SetDesc(nsCSSCounterDesc aDescID, const nsCSSValue& aValue)
>+{
>+  NS_ABORT_IF_FALSE(aDescID > eCSSCounterDesc_UNKNOWN && 
>+                    aDescID < eCSSCounterDesc_COUNT,
>+                    "descriptor ID out of range");

Test >= 0 rather than > UNKNOWN (given that you're indexing into an array).

In GetSymbols and GetAdditiveSymbols:

>+    for (auto item = value.GetListValue(); item; item = item->mNext) {

Please use "const nsCSSValueList* item" here.

Also, just Truncate() before the switch, and turn the switch into an if.

In GetRange:

Again, please don't use auto (I'd rather have the const-ness and
the type label).

Make lower and upper be const nsCSSValue& rather than just nsCSSValue;
there's no need to copy.

In GetSpeakAs and GetAttribute (or its new name) you can also
move the Truncate() call to the top.

nsCSSCounterStyleRule::GetAttribute (or its new name):

Please assert that you've been given one of the descriptors that's
expected here.  Or, alternatively, move the code from the hand-written
getter functions into GetAttribute/GetDescriptor and make it use a
switch over the descriptor, and make all the getters be stub functions.

Odds are I've missed a bunch of bugs in the serialization code in
the getters, but better tests should find them.

In nsCSSRules.h:

Change protected to private; the class is MOZ_FINAL.

r=dbaron with those comments and the comments in comment 81 and
comment 85/comment 87.

Comment 91

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #90)
> Comment on attachment 8398893 [details] [diff] [review]
> patch 1 - parse @counter-style rule, r=bz
> 
> I'm not confident about how well the dynamic change handling works; I'd
> like to see more tests for that, and any bugs uncovered fixed in
> additional patches (not modifications of this one!).

One more thing I'd like to change in this patch is using ParseCustomIdent in ParseCounterStyleName instead, since the spec has been changed, and the counter-style-name is custom-ident now. Is it okay to do it with other modifications you mentioned?

Comment 92

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That seems fine.  Presumably you'd pass nonempty aExcludedKeywords only for the name in a @counter-style rule, or something like that?

Comment 93

[Jonathan Kew [:jfkthame]]

Comment on attachment 8398894 [details] [diff] [review]
patch 2 - computation of counter style

Stealing r? from dbaron here, as he has lots in his queue already... I'll try to post a review within a couple of days here, and perhaps take more of the patches as well if time permits.

Comment 94

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule, r=bz

Most modifications are to follow comment 81, comment 85, comment 87, comment 90, and comment 91. Additionally, I added to CSSParserImpl a helper function ParseNonNegativeInteger, which is a simple wrapper of ParseNonNegativeVariant, to simplify code. I think it is fine but you may want to review it.

Comment 95

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style

Add tests for dynamic changes to @counter-style rule.

Note that, one bug in patch 1 is revealed by the new tests. The bug is that getter of name attribute does not return a properly escaped identifier.

In some sense, this bug is similiar to bug 993746. However, the css-counter-style spec says it "must return a DOMString object that contains the serialization of the <counter-style-name> defined for the associated rule", while css-animation does not clearly state the behavior.

The fix of this bug can be found in https://hg.mozilla.org/try/rev/02ab6ed19a2d . Since it is very simple, I plan to include it in patch 1, is it ok?

Comment 96

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style

Test for dynamic change is modified to avoid being affected by bug 994418.

Comment 97

[Jonathan Kew [:jfkthame]]

A general question, based on reading some of the code here: I notice a lot of variables are being declared as "auto". Is this purely to save a few keystrokes ("auto" is shorter than "nsString" or "additive_symbol", etc) or are there any other reasons to use this? I can see it could be convenient in some cases, especially with complex templates, STL iterators, etc., where the full types become extremely verbose. But in many cases, I feel it makes the code harder to follow than if the actual types were mentioned.

So I'd prefer to avoid so much use of "auto" in general. But perhaps that's just me; :dbaron, WDYT about this? Am I just too old-fashioned? :) If there's general agreement that using auto all over the place is a Good Thing, I'll just have to get used to it, I guess.

Comment 98

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #97)
> A general question, based on reading some of the code here: I notice a lot
> of variables are being declared as "auto". Is this purely to save a few
> keystrokes ("auto" is shorter than "nsString" or "additive_symbol", etc) or
> are there any other reasons to use this? I can see it could be convenient in
> some cases, especially with complex templates, STL iterators, etc., where
> the full types become extremely verbose. But in many cases, I feel it makes
> the code harder to follow than if the actual types were mentioned.
> 
> So I'd prefer to avoid so much use of "auto" in general. But perhaps that's
> just me; :dbaron, WDYT about this? Am I just too old-fashioned? :) If
> there's general agreement that using auto all over the place is a Good
> Thing, I'll just have to get used to it, I guess.

I guess dbaron will agree with your opinion, since he has required me to avoid using "auto" for simple types in the patch 1 (see comment 90). Patch 2 has not been modified since then.

If you all do not like this style, I'll change mine. I just feel it is convenient and makes the line shorter which reduces line breaks in statements.

Comment 99

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan from comment #98)
> 
> I guess dbaron will agree with your opinion, since he has required me to
> avoid using "auto" for simple types in the patch 1 (see comment 90). Patch 2
> has not been modified since then.
> 
> If you all do not like this style, I'll change mine. I just feel it is
> convenient and makes the line shorter which reduces line breaks in
> statements.

Yes, it's often shorter, but I feel this is outweighed by the fact that it makes it harder for the reader to know what is going on. Given dbaron's comment 90, I think we'd like you to make this change throughout the patches, please.

[clearing needinfo?dbaron, as the comment mentioned above seems clear enough]

Comment 100

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #99)
> (In reply to Xidorn Quan from comment #98)
> > 
> > I guess dbaron will agree with your opinion, since he has required me to
> > avoid using "auto" for simple types in the patch 1 (see comment 90). Patch 2
> > has not been modified since then.
> > 
> > If you all do not like this style, I'll change mine. I just feel it is
> > convenient and makes the line shorter which reduces line breaks in
> > statements.
> 
> Yes, it's often shorter, but I feel this is outweighed by the fact that it
> makes it harder for the reader to know what is going on. Given dbaron's
> comment 90, I think we'd like you to make this change throughout the
> patches, please.

OK, I'll change them with other modifications you may want me to do after you complete the review. BTW, I don't think you have a shorter review queue than dbaron as 6 of 11 patches in his queue are from this bug :)

Comment 101

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, I think auto makes sense for types that are particularly complex to write out, but not for simple types.  (I also dislike that it tends to discourage const-ness.)

Comment 102

[Jonathan Kew [:jfkthame]]

Comment on attachment 8398894 [details] [diff] [review]
patch 2 - computation of counter style

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

I think the new classes here should be put into mozilla::css rather than the global namespace. (We currently have a mixture of namespaced and ns-prefixed stuff, but AIUI the trend is towards using actual namespacing instead of ns...)

** Unless dbaron doesn't want us to go that way here; setting needinfo? for an opinion on that.

Overall, this looks good to me, aside from all the 'auto' variables! A few other small style issues, etc...

Marking this r- because it'll need updating and then I'll want to re-read it, but in principle I expect to r+ a new version of it.

::: layout/generic/nsBulletFrame.cpp
@@ +793,5 @@
>    char16_t unit10K[2];
>    uint8_t lang;
>    bool informal;
>  };
> +extern const char16_t gJapaneseNegative[] = {

No "extern" on these definitions, please; they're actually defined here, not elsewhere. Just removing the "static" should be enough to make them visible; "extern" is only needed on the declarations in nsCounterStyleManager.cpp.

::: layout/style/nsCounterStyleManager.cpp
@@ +17,5 @@
> +
> +using namespace mozilla;
> +
> +typedef std::pair<nsString, nsString> negative_type;
> +typedef std::pair<CounterValue, nsString> additive_symbol;

underscore_separated names look out-of-place in Gecko; could we use CamelCase instead, please.

@@ +23,5 @@
> +// This limitation will be applied to some systems, and pad descriptor. Any
> +// initial representation generated by symbolic or additive which is longer
> +// than this limitation will be dropped. If any pad is longer than this, the
> +// whole counter text will be dropped as well.
> +#define LENGTH_LIMIT 100

It looks like LENGTH_LIMIT is applied in terms of UTF-16 code units, but the spec says UAs "must support representations at least 60 Unicode codepoints long", which could require as many as 120 code units.

@@ +336,5 @@
> +      aResult.first = gTradChineseNegative;
> +      break;
> +
> +    default:
> +      aResult.first.AssignLiteral("-");

Instead of AssignLiteral (which will do a runtime conversion from ASCII to UTF16), use NS_LITERAL_STRING("-").

@@ +440,5 @@
> +{
> +  switch (mStyle) {
> +    case NS_STYLE_LIST_STYLE_DECIMAL_LEADING_ZERO:
> +      aWidth = 2;
> +      aSymbol.AssignLiteral("0");

NS_LITERAL_STRING

@@ +670,5 @@
> +  }
> +  aResult = mPrefix;
> +}
> +
> +static const PRUnichar gSuffixSuppressPadding[] = {

s/PRUnichar/char16_t/

@@ +676,5 @@
> +  0x3002, // ideographic full stop
> +  0xFF0C, // fullwidth comma
> +  0xFF0E, // fullwidth full stop
> +  0xFF1A, // fullwidth colon
> +  0x0000

No need for the null terminator here...

@@ +680,5 @@
> +  0x0000
> +};
> +
> +/* virtual */ void
> +CustomCounterStyle::GetSuffix(nsAString& aResult, bool&aSuppressPadding)

needs space after bool&

@@ +690,5 @@
> +      value.GetStringValue(mSuffix);
> +      // Default padding should be suppressed if the suffix ends with some
> +      // chars inside which there has been a wide space at the end.
> +      if (!mSuffix.IsEmpty()) {
> +        for (int i = 0; gSuffixSuppressPadding[i]; ++i) {

...if you use i < mozilla::ArrayLength(gSuffixSuppressPadding) for the loop condition.

@@ +700,5 @@
> +      }
> +    } else if (IsOverrideSystem()) {
> +      GetOverride()->GetSuffix(mSuffix, mIsSuppressPadding);
> +    } else {
> +      mSuffix.AssignLiteral(".");

NS_LITERAL_STRING

@@ +774,5 @@
> +      default: {
> +        if (IsOverrideSystem()) {
> +          GetOverride()->GetNegative(mNegative);
> +        } else {
> +          mNegative.first.AssignLiteral("-");

NS_LITERAL_STRING

@@ +1004,5 @@
> +    case eCSSUnit_Null:
> +      if (!IsOverrideSystem()) {
> +        SetSpeakAsAuto();
> +        return nullptr;
> +      } else {

No need for 'else' when the 'if' branch ends with a return.

@@ +1213,5 @@
> +}
> +
> +struct BuiltinCounterStyleTable
> +{
> +  static const auto kLength = 256;

256 seems surprisingly large. How many entries do we really need here?

(Is there any reason not to adjust the NS_STYLE_LIST_STYLE_... constants from MOZ_CJK_HEAVENLY_STEM onwards so as to remove the huge gap we currently have, and thus allow this table to be *much* smaller?)

@@ +1233,5 @@
> +  // There are no more than 256 builtin counter styles
> +  BuiltinCounterStyle mStyles[kLength];
> +};
> +
> +static BuiltinCounterStyleTable gBuiltinStyleTable;

I'd prefer to keep the construction of this table out of the startup path, by making it a pointer to a singleton object that's created on first use.

@@ +1241,5 @@
> +  gBuiltinStyleTable[NS_STYLE_LIST_STYLE_NONE];
> +
> +/* static */ CounterStyle*
> +nsCounterStyleManager::sDecimalStyle =
> +  gBuiltinStyleTable[NS_STYLE_LIST_STYLE_DECIMAL];

And these statics shouldn't be needed, AFAICS; Get{None,Decimal}Style can just pass the appropriate NS_STYLE_... constants to GetBuiltinStyle and return the result.

@@ +1327,5 @@
> +  bool mChanged;
> +};
> +
> +static PLDHashOperator
> +MarkAndClean(const nsAString& aKey, nsRefPtr<CounterStyle>& aData, void* aArg)

Please rename s/aData/aStyle/ here. It's a bit confusing to have both aData (a refptr to CounterStyle) and data (the MarkAndCleanData) used below.

@@ +1355,5 @@
> +  data->mChanged = data->mChanged || changed || removed;
> +  if (removed) {
> +    if (aData->IsCustomStyle()) {
> +      // The object has to be hold here so that they will not be released
> +      // before all pointers refer to it are reset.

Add to this comment: It will be released when the MarkAndCleanData goes out of scope at the end of CleanDirty().

::: layout/style/nsCounterStyleManager.h
@@ +17,5 @@
> +
> +#include "mozilla/NullPtr.h"
> +#include "mozilla/Attributes.h"
> +
> +typedef int32_t CounterValue;

It's not clear to me that we gain anything by having this typedef, rather than simply using int32_t values.

Comment 103

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #102)
> ::: layout/generic/nsBulletFrame.cpp
> @@ +793,5 @@
> >    char16_t unit10K[2];
> >    uint8_t lang;
> >    bool informal;
> >  };
> > +extern const char16_t gJapaneseNegative[] = {
> 
> No "extern" on these definitions, please; they're actually defined here, not
> elsewhere. Just removing the "static" should be enough to make them visible;
> "extern" is only needed on the declarations in nsCounterStyleManager.cpp.

Nope, without expliticly "extern" declaration, "const" implies "static". So it is necessary to add "extern" here or other place in the same file before the definition.

> @@ +23,5 @@
> > +// This limitation will be applied to some systems, and pad descriptor. Any
> > +// initial representation generated by symbolic or additive which is longer
> > +// than this limitation will be dropped. If any pad is longer than this, the
> > +// whole counter text will be dropped as well.
> > +#define LENGTH_LIMIT 100
> 
> It looks like LENGTH_LIMIT is applied in terms of UTF-16 code units, but the
> spec says UAs "must support representations at least 60 Unicode codepoints
> long", which could require as many as 120 code units.

Oops, I forgot the surrogates.

> @@ +1213,5 @@
> > +}
> > +
> > +struct BuiltinCounterStyleTable
> > +{
> > +  static const auto kLength = 256;
> 
> 256 seems surprisingly large. How many entries do we really need here?
> 
> (Is there any reason not to adjust the NS_STYLE_LIST_STYLE_... constants
> from MOZ_CJK_HEAVENLY_STEM onwards so as to remove the huge gap we currently
> have, and thus allow this table to be *much* smaller?)

Since a bunch of constants will get removed in patch 4, maybe we can renumber the constants and shrink the table there. What do you think?

> @@ +1233,5 @@
> > +  // There are no more than 256 builtin counter styles
> > +  BuiltinCounterStyle mStyles[kLength];
> > +};
> > +
> > +static BuiltinCounterStyleTable gBuiltinStyleTable;
> 
> I'd prefer to keep the construction of this table out of the startup path,
> by making it a pointer to a singleton object that's created on first use.

Hopefully it could be a compile-time initialization in the future (when C++14 comes, all the constructors here can be marked constexpr). OTOH, making it a singleton object will add an extra check for every access. As the startup code itself depends on the table (required by generating RuleNode for any list), I don't think it could accelerate the startup.

> ::: layout/style/nsCounterStyleManager.h
> @@ +17,5 @@
> > +
> > +#include "mozilla/NullPtr.h"
> > +#include "mozilla/Attributes.h"
> > +
> > +typedef int32_t CounterValue;
> 
> It's not clear to me that we gain anything by having this typedef, rather
> than simply using int32_t values.

I think it is a more semantic name. It is possible that one day we decide to move to int64_t or even some bigint implementation instead of int32_t. Then we will hopefully only need to change this typedef instead of every reference.

I'll wait for the decision about the namespace before the next update. If the decision is putting the classes into mozilla::css, the patch 3 will also need to be updated. You may not want to start reviewing that patch before I update it.

Comment 104

[Jonathan Kew [:jfkthame]]

:dbaron, what's your preference - should we be putting the new classes here into the mozilla::css namespace, or staying with the old-style ns prefix?

Comment 105

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I probably prefer new stuff be in mozilla:: but in mozilla::css::.

(Maybe I'm ok with mozilla::css::... but I strongly dislike the renaming of things like "CSSDeclaration" to "Declaration" that was part of it when such objects are known as "CSSDeclaration" in the DOM.)

Comment 106

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, I think AssignLiteral() is probably preferable to Assign(NS_LITERAL_STRING()).

Comment 107

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #106)
> Also, I think AssignLiteral() is probably preferable to
> Assign(NS_LITERAL_STRING()).

Hmm, that's not what the coding style guide seems to say:

https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style#Use_NS_LITERAL_STRING.28.29_to_avoid_runtime_string_conversion

(But perhaps that shouldn't apply in the case of extremely short strings?)

Comment 108

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #105)
> I probably prefer new stuff be in mozilla:: but in mozilla::css::.

(Presumably that should read "...but /not/ in...")

I suggested mozilla::css based on what other classes in /layout/style seem to be doing (e.g. mozilla::css::CommonAnimationManager), but it's not clear to me that we've really decided quite what we're doing about namespaces.

Comment 109

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, I meant "not".

I either want to get rid of the mozilla::css namespace entirely, or use it only for structures that are part of CSS stylesheet representation.

Comment 110

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #107)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #106)
> > Also, I think AssignLiteral() is probably preferable to
> > Assign(NS_LITERAL_STRING()).
> 
> Hmm, that's not what the coding style guide seems to say:
> 
> https://developer.mozilla.org/en/docs/Developer_Guide/
> Coding_Style#Use_NS_LITERAL_STRING.28.29_to_avoid_runtime_string_conversion
> 
> (But perhaps that shouldn't apply in the case of extremely short strings?)

After investigating the code, I think the best way should be AssignLiteral(MOZ_UTF16("-")). Use AssignLiteral(char[]) on nsAString is least preferable since it causes a runtime conversion. Use Assign(NS_LITERAL_STRING("-")) will finally call AssignLiteral(char16_t[]). Hence the best way is to call AssignLiteral(char16_t[]) directly.

Comment 111

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #101)
> Yes, I think auto makes sense for types that are particularly complex to
> write out, but not for simple types.  (I also dislike that it tends to
> discourage const-ness.)

Do you think it is proper to use auto for some integer variables which store values returned from methods like Length()? For perfectly exactness, the type is long as well (something like nsTArray<nsString>::size_type) although their backend is simple (uint32_t in the example before). I think it should be a recommended way of using auto as it keeps the declaration simple while has no implicit casting.

Comment 112

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style

1. Make list items float to prevent content from being out of the reftest viewport.
2. Add tests for too long representation detection with codepoints outside BMP.

Comment 113

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #108)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #105)
> > I probably prefer new stuff be in mozilla:: but in mozilla::css::.
> 
> (Presumably that should read "...but /not/ in...")
> 
> I suggested mozilla::css based on what other classes in /layout/style seem
> to be doing (e.g. mozilla::css::CommonAnimationManager), but it's not clear
> to me that we've really decided quite what we're doing about namespaces.

According to the coding style: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces , it seems only namespace mozilla should be used in this case, and the whole .cpp file should be wrapped in the namespace. This also matches dbaron's opinion in comment 109.

Comment 114

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

This version of patch follows most of the comments above. In addition, the refcount mechanism of CounterStyle is altered so that the builtin style table consumes less memory, and CounterStyle and BuiltinCounterStyle could be literal type. (As mentioned before, BuiltinCounterStyleTable can be literal type as well when C++14 comes. Then, this table will be able to be initialized in compile-time.)

Since this new version renames file nsCounterStyleManager.{h,cpp} to CounterStyleManager.{h,cpp}, nearly all patches after it are affected. However, most of them do not contain other changes, so I prefer not to update them now.

Comment 115

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - link to other parts

Comment 116

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

Sorry for being annoying. One typo I forgot to fix.

Comment 117

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4.5 - shrink builtin style table

Comment 118

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Additional changes to patch 1

I tend to merge this patch to patch 1 but as these changes were not mentioned in your comments, I separate them out so that it could be easier for you to review. I'll merge the two once they all get review granted.

Comment 119

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Additional changes to patch 2

This is some changes I tend to merge into patch 2.

Comment 120

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8402254 [details] [diff] [review]
patch 1 - parse @counter-style rule, r=bz

You probably want "r=dbaron,bz" rather than just "r=bz" at this point.

>+PECounterASWeight=Additive tuples must be specified in order of descending weight.

Instead, how about either:
>PECounterASWeight=Each weight in the additive-symbols() function must be less than the previous weight.
or:
>PECounterASWeight=The weights in the additive-symbols() function must be in order from largest to smallest.

In CSSParserImpl::ParseCounterDescriptorValue, in the
eCSSCounterDesc_Symbols case, you should only call aValue.SetListValue()
in one of the two places where you currently call it.  It doesn't really
matter which.

From comment 90:
>In GetSpeakAs and GetAttribute (or its new name) you can also
>move the Truncate() call to the top.
(the new name is GetDescriptor)

r=dbaron with that

Comment 121

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8409494 [details] [diff] [review]
Additional changes to patch 1

> // nsIDOMCSSCounterStyleRule methods
> NS_IMETHODIMP
> nsCSSCounterStyleRule::GetName(nsAString& aName)
> {
>-  aName = mName;
>+  aName.Truncate();
>+  nsStyleUtil::AppendEscapedCSSIdent(mName, aName);
>   return NS_OK;
> }

I feel like this is a bit inconsistent with the way other CSSOM APIs are specified, but I guess it's what the spec says.

r=dbaron

Comment 122

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #111)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #101)
> > Yes, I think auto makes sense for types that are particularly complex to
> > write out, but not for simple types.  (I also dislike that it tends to
> > discourage const-ness.)
> 
> Do you think it is proper to use auto for some integer variables which store
> values returned from methods like Length()? For perfectly exactness, the
> type is long as well (something like nsTArray<nsString>::size_type) although
> their backend is simple (uint32_t in the example before). I think it should
> be a recommended way of using auto as it keeps the declaration simple while
> has no implicit casting.

I think using auto in that context makes sense.

Comment 123

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And one other comment on |auto|; I think there are probably more cases where I'd be comfortable with |const auto& v| or |auto& v| than plain old |auto v|.

Comment 124

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - parse @counter-style rule, r=dbaron,bz

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #120)
> Comment on attachment 8402254 [details] [diff] [review]
> patch 1 - parse @counter-style rule, r=bz
> 
> In CSSParserImpl::ParseCounterDescriptorValue, in the
> eCSSCounterDesc_Symbols case, you should only call aValue.SetListValue()
> in one of the two places where you currently call it.  It doesn't really
> matter which.

I replace the first call with nullptr, and use that variable as a flag directly instead of the additional variable "first".

In addition, some switch-case block are reindented so that all "case"s/"default" are one level deeper than their corresponding "switch".

I think these changes should be okay without further review.

Comment 125

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Additional changes to patch 2

Fix some problems.

Comment 126

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - move builtin styles code

Update because of changes to patch 2

Comment 127

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style (WIP)

Add cases to test codepoints outside BMP which is mentioned in comment 102.

Comment 128

[Xidorn Quan [:xidorn] UTC+10]

try server result: https://tbpl.mozilla.org/?tree=Try&rev=fec12cd82fab

Comment 129

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8408908 [details] [diff] [review]
patch 2 - computation of counter style

nsPresContext.cpp:

>+  // Since there are methods in nsPreContext have the same name as the classes,

"nsPreContext" -> "nsPresContext"

(Also, I prefer wrapping comments at 72 characters.)

nsBulletFrame.cpp:

I'm not a big fan of these |extern|s, but given that they go away
in patch 6, they're ok.

nsStyleConsts.h:

>+#define NS_STYLE_COUNTER_SPEAKAS_STYLE      255 // refer to another style

I'd prefer name that more clearly indicates that this means it's
a reference to another style.

CounterStyleManager.h:

I tend to think the #include guards should have a mozilla_ prefix,
even though the file isn't currently exported, because if it were
exported it would be mozilla/CounterStyleManager.h.  I'm not sure
about this, though.  (It's not that important, either.)

>+#include <new>
>+#include <utility>
>+#include "nsTArray.h"
>+#include "nsString.h"

I don't see where you're using things in these headers in
CounterStyleManager.h .  Can you remove these or move them to the .cpp
file appropriate?

>+  NS_IMETHOD_(MozExternalRefCountType) AddRef() = 0;
>+  NS_IMETHOD_(MozExternalRefCountType) Release() = 0;

These can just be virtual rather than NS_IMETHOD_.  With corresponding
changes in the cpp file, of course.

>+  bool CleanDirty();

CleanDirty is a very strange name.  Could you give this function
a clearer name and document it better?  Maybe CounterStylesChanged()?

Please use nsSubstring instead of nsAString throughout.

CounterStyleManager.cpp:

>+// The spec requires user agents to support at least 60 Unicode codepoints for
>+// counter text. However, this constant only limits the length of UTF-16 string.
>+// So it has to be at least 120, since codepoints outside BMP will need 2 chars.

"length" -> "length in 16-bit units"
"outside BMP" -> "outside the BMP"
"2 chars" -> "2 16-bit units"


In GetSymbolicCounterText and GetAlphabeticCounterText, it seems odd
that you handle aOrdinal == 0 but give a fatal assert for negative
values.  It seems like CustomCounterStyle::IsOrdinalInRange is going to
prevent 0 from getting in as well, no?  Maybe it would be better to
handle any <= value, but have a non-fatal assertion that
IsOrdinalInRange has not been checked?  (Safety seems worthwhile
in code that's not performance-critical.)

In GetSymbolicCounterText you should also assert that n > 0.
(GetAlphabeticCounterText and GetNumericCounterText already assert about
aSymbols.Length().)

In GetAlphabeticCounterText and GetNumericCounterText:

>+  int32_t indexes[std::numeric_limits<CounterValue>::digits];

Please rewrite this using nsAutoTArray<int32_t, ...> so that you're
not at risk of a stack buffer overflow.

In GetAdditiveCounterText:

Please put the first parameter on the same line as the function name,
and wrap others to match that position.

>+  for (decltype(aSymbols.Length()) i = 0; i < aSymbols.Length(); ++i) {

Please use an iEnd variable rather than calling aSymbols.Length() every
time.

>+    if (weight == 0) {
>+      continue;
>+    }

This may as well be break rather than continue.

I think it would be clearer to make AdditiveSymbol a struct with
meaningful member names rather than using std::pair whose member names
are first and last.  (say, weight and symbol)

The same goes for NegativeType (say, using symbolBefore and symbolAfter)?

class BuiltinCounterStyle:

Please mark AddRef and Release as MOZ_OVERRIDE as well.

Please make AddRef return 2 rather than 1.  (That's the convention
for classes with dummy reference counts; AddRef and Release both make
it looks like there's somebody else holding a reference.)

In BuiltinCounterStyle::GetSpokenCounterText, you should fix the
XXX comment that you introduce here in patch 6, I believe.  It looks
like you don't currently.

In BuiltinCounterStyle::GetSpokenCounterText, maybe add a comment
explaining that you're following the rules for the 'speak-as' descriptor?

Should BuiltinCounterStyle::IsNegativeCapable really be returning
true for cjk-decimal?

class CustomCounterStyle:

CustomCounterStyle should not use MOZ_COUNT_CTOR and MOZ_COUNT_DTOR
since it uses NS_INLINE_DECL_REFCOUNTING.

But BuiltinCounterStyle should use MOZ_COUNT_CTOR and MOZ_COUNT_DTOR.

The names ResetAll, ResetOverridable, and ResetPointers are confusing.
The names, or comments, should explain why there are three different
functions and when to use which one.  Perhaps ResetOverridable should be
merged into ResetPointers, ResetPointers should be renamed
OtherStylesChanged, and ResetAll should be renamed to ThisStyleChanged?

Please put mManager before mRule and mRuleGeneration so that the
pointers are next to each other and you don't have holes in the data
structure on 64-bit.

You need some way to ensure that the mManager pointers get cleared
when the CounterStyleManager goes away.  One option would be to
get rid of the reference-counting of counter styles and just delete
all of them when the CounterStyleManager clears its table.  Otherwise
you need some way to clear those pointers.  (It's not yet clear to me
why you chose to make counter styles reference-counted.)

It's also not clear to me how ownership of the mFallback, mSpeakAsCounter,
mOverride, and mOverrideRoot pointers is manager.  It rather looks like
you're just deleting all counter styles whenever the CounterStyleManager
clears its table.  If so, there should be a comment above those pointers
explaining that, and you should get rid of the reference counting
(and add back the MOZ_COUNT_CTOR and MOZ_COUNT_DTOR).

For mPad, instead of std::pair, please use a struct with meaningful
variable names.

Please don't use SetIsVoid on strings; it was designed to support the
DOM API of string-or-null, and it shouldn't be used for anything else.
I'd rather you used a separate bool member variable (or a single integer
with some dirty bits) if you need it.  In fact, you can use additional
bits in mMarks.

Please also rename mMarks to mFlags to be consistent with our normal
naming conventions.

Is the concept expressed by mSuppressPadding covered in the spec?  If
not, could you send comments to www-style explaining how it should be
covered?

>+static nsCSSValue kInfinite(
>+    NS_STYLE_COUNTER_RANGE_INFINITE, eCSSUnit_Enumerated);

Please don't use a static constructor.

Instead, please write a function:

static inline bool
IsCSSValueInfinite(const nsCSSValue& aValue)
{
  return aValue.GetUnit() == eCSSUnit_Enumerated &&
         aValue.GetIntValue() == NS_STYLE_COUNTER_RANGE_INFINITE;
}

and then use that inside IsOrdinalInRange.

>+      CounterValue lowerBound = item->mXValue == kInfinite ?
>+        std::numeric_limits<CounterValue>::min() :
>+        item->mXValue.GetIntValue();
>+      CounterValue upperBound = item->mYValue == kInfinite ?
>+        std::numeric_limits<CounterValue>::max() :
>+        item->mYValue.GetIntValue();
>+      if (aOrdinal >= lowerBound && aOrdinal <= upperBound) {
>+        return true;
>+      }

Likewise, please restructure this to avoid doing the comparison
at all when the value is infinite, instead of playing tricks with
std::numeric_limits.

Should IsOrdinalInRange really always return true for the 'fixed'
system?  The spec makes me think not.  (This should have tests added
to cover it.)

Also, the spec seems to say that the 'additive' system should support
negative values, so the handling of that system also seems wrong.
(This should have tests added to cover it.)

Please add a brief comment in CustomCounterStyle::CallFallbackStyle to
explain that its purpose is to handle fallback loops.

The interaction between IsNegativeCapable and IsOrdinalInRange is
a little bit confusing; could you add some comments to the header file
to clarify?

In GetSymbols and GetAdditiveSymbols:
  >+    for (const nsCSSValueList* item = values.GetListValue();
  >+         item != nullptr; item = item->mNext) {
please replace "item != nullptr" with just "item".

I find the speak-as computation code quite confusing.  First, the name
ParseSpeakAs is inappropriate for a function that's not doing parsing.
But it's not clear from the function names what the difference between
ParseSpeakAs and ComputeSpeakAs is; I'd like to see better names and
probably also better comments.  It's also not clear why all of these
functions have *different* checks against
NS_STYLE_COUNTER_SPEAKAS_STYLE.   It's not clear what the caching in
mSpeakAs and mSpeakAsCounter means -- and different functions seem to
cache in these variables in different ways.  I haven't yet reviewed
these functions (SetSpeakAsAuto, ParseSpeakAs, ComputeSpeakAs,
GetSpeakAsCounter) in detail because I'd like them to be more
understandable first.

In ComputeOverride:

>+  if (nextCounter->IsCustomStyle()) {
>+    mMarks |= MARK_OVERRIDE_VISITED;
>+    target = static_cast<CustomCounterStyle*>(nextCounter)->ComputeOverride();
>+    mMarks &= ~MARK_OVERRIDE_VISITED;
>+  }

Please assert before adding MARK_OVERRIDE_VISITED that it isn't
already set.

+  if (target) {
+    mOverride = nextCounter;
+    return this;
+  } else {
+    mOverride = CounterStyleManager::GetDecimalStyle();
+    if (mMarks & MARK_OVERRIDE_LOOP) {
+      mMarks &= ~MARK_OVERRIDE_LOOP;
+      return this;
+    } else {
+      return nullptr;
+    }
+  }

Please assert in the if (target) branch that MARK_OVERRIDE_LOOP is
not set.

Please add a comment explaining how the MARK_OVERRIDE_LOOP /
MARK_OVERRIDE_VISITED code implements this sentence in the spec:
  # If one or more @counter-style rules form a cycle with their override
  # values, all of the counter styles participating in the cycle must be
  # treated as if they were overriding the decimal counter style
  # instead.
(My understanding is that what you're doing is setting the mOverride
of everything *in* the loop to decimal, and making ComputeOverride
return null when it's returning to another participant in the loop but
return this when it's returning to something outside the loop.)

In GetOverrideRoot, could you use a while loop with condition
(overridden->IsCustomStyle()) rather than using recursion?

In GetCounterText:
>+      nsAutoString symbol;
This can just be nsString, since it's always going to share a
refcounted or literal buffer.

BuiltinCounterStyleTable:

>+  static MOZ_CONSTEXPR_VAR uint32_t kLength = 256;

Instead of doing this, please adjust the NS_STYLE_LIST_STYLE_* constants
to be consecutive, add an NS_STYLE_LIST_STYLE_MAX constant, and use
that.

Also please make mStyles private.

>+static BuiltinCounterStyleTable gBuiltinStyleTable;

Please make this a BuiltinCounterStyleTable*, initialize it from
a function called from nsLayoutStatics::Initialize, and free it
from a function called from nsLayoutStatics::Shutdown.  (We want
to avoid static constructors, both because code that runs in them
is hard to use debugging tools with because of locks that are held
while running them, and for some other reasons.)

Why do you create a builtin counter style for the empty string in 
CounterStyleManager::InitializeCacheTable()?  Can it be removed?
If not, please explain in code comments.

Please comment in CounterStyleManager::BuildCounterStyle that it is
intentional that the predefined names are case-insensitive but the
user-defined names are case-sensitive.

>+  MarkAndCleanData* data = reinterpret_cast<MarkAndCleanData*>(aArg);

static_cast, not reinterpret_cast.

>+  bool removed = false;

Please call this remove rather than removed.

>+      // The object has to be hold here so that they will not be released

"hold" -> "held"
"they" -> "it" (or, alternately, "object has" -> "objects have", and other
                changes in the next sentence)

>+      // before all pointers refer to it are reset. It will be released when

"pointers refer" -> "pointers that refer"

Could you rename MarkAndClean and MarkAndCleanData -- there's no
marking involved that I can see.  Maybe InvalidateOldData and
(ugh) InvalidateOldDataData?

Maybe also rename ResetStyleData to InvalidateLinksToOtherStyles?


Marking as review- since I'd like to look at how you address a
number of the more complex issues above, but most of this looks good.

Comment 130

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One other thought about the reference counting -- maybe the reference-counting with weak back-pointers to the CounterStyleManager is ok if you document that the CounterStyle objects are local to the CounterStyleManager and never supposed to be handed out to other objects (since they have weak pointers to the CounterStyleManager).

Comment 131

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #129)
> Comment on attachment 8408908 [details] [diff] [review]
> patch 2 - computation of counter style
> 
> CounterStyleManager.h:
> 
> I tend to think the #include guards should have a mozilla_ prefix,
> even though the file isn't currently exported, because if it were
> exported it would be mozilla/CounterStyleManager.h.  I'm not sure
> about this, though.  (It's not that important, either.)
> 
> >+#include <new>
> >+#include <utility>
> >+#include "nsTArray.h"
> >+#include "nsString.h"
> 
> I don't see where you're using things in these headers in
> CounterStyleManager.h .  Can you remove these or move them to the .cpp
> file appropriate?

nsString.h is put for type nsAString, but it seems that a declaration is sufficient? <utility> is necessary for std::pair, and <new> is added because of bug 981264. However, as you suggest using struct instead of pair in all places, they could be removed.

> In GetSymbolicCounterText and GetAlphabeticCounterText, it seems odd
> that you handle aOrdinal == 0 but give a fatal assert for negative
> values.  It seems like CustomCounterStyle::IsOrdinalInRange is going to
> prevent 0 from getting in as well, no?  Maybe it would be better to
> handle any <= value, but have a non-fatal assertion that
> IsOrdinalInRange has not been checked?  (Safety seems worthwhile
> in code that's not performance-critical.)

Reasons:
1. range may be specified by authors via 'range' descriptor, which may cover zero, so the zero is logically possible to reach here; but
2. for negative-capable systems, negative values should be processed in GetCounterText, so logically they will never be passed to these functions.

Though these algorithms never accept zero, processing zero in some earlier stage will increase the complexity. Other systems can also fail to process some value in the generating algorithm. So I think this is the most suitable place to reject the zero.

> In BuiltinCounterStyle::GetSpokenCounterText, you should fix the
> XXX comment that you introduce here in patch 6, I believe.  It looks
> like you don't currently.

This XXX comment has been moved in "Additional changes to patch 2", and patch 6 fixes that comment in the new place. Should I merge these two patches in the next submission?

> Should BuiltinCounterStyle::IsNegativeCapable really be returning
> true for cjk-decimal?

Yes, because system of cjk-decimal is numeric, and numeric is negative-capable. Doing this is for the exactly correct behavior when someone overrides it. Though, this code will be removed in patch 4.

> Is the concept expressed by mSuppressPadding covered in the spec?  If
> not, could you send comments to www-style explaining how it should be
> covered?

In fact, I don't think it is a spec affair. In my understanding, it is more an implementation detail about how to display it better. Currently, Firefox and Chrome have been using different way to generate the space between number and content. Hence, I don't think it is necessary to discuss it in the css group.

> >+      CounterValue lowerBound = item->mXValue == kInfinite ?
> >+        std::numeric_limits<CounterValue>::min() :
> >+        item->mXValue.GetIntValue();
> >+      CounterValue upperBound = item->mYValue == kInfinite ?
> >+        std::numeric_limits<CounterValue>::max() :
> >+        item->mYValue.GetIntValue();
> >+      if (aOrdinal >= lowerBound && aOrdinal <= upperBound) {
> >+        return true;
> >+      }
> 
> Likewise, please restructure this to avoid doing the comparison
> at all when the value is infinite, instead of playing tricks with
> std::numeric_limits.

Why not using std::numeric_limits here? I found it could make the code much more complex without this method.

> Should IsOrdinalInRange really always return true for the 'fixed'
> system?  The spec makes me think not.  (This should have tests added
> to cover it.)

Not always, only when the range is not specified by the author. The spec says the default 'auto' for 'range' descriptor means "for cyclic, numeric, and fixed systems, the range is negative infinity to positive infinity."

> Also, the spec seems to say that the 'additive' system should support
> negative values, so the handling of that system also seems wrong.
> (This should have tests added to cover it.)

'additive' system supports negative values by add negative signs in GetCounterText. The algorithm itself doesn't generate representation for negatives directly.

> The interaction between IsNegativeCapable and IsOrdinalInRange is
> a little bit confusing; could you add some comments to the header file
> to clarify?

OK, I think you have been confused :)
I'd like to describe it to you here first.

IsNegativeCapable means whether it is possible to generate representation of negative values by adding negative sign to the initial representation of their absolute value. This capability only depends on the system, and completely unrelated to the range. System, such as cyclic and fixed, which could generate negative representation via its own algorithm is not negative-capable.

IsOrdinalInRange checks whether an ordinal is in the range of the given counter style. It depends on the value specified in 'range' descriptor by author. If the value is not specified, some default value is used.

For example, a counter style
@counter-style { system: symbolic; range: -1 1; symbols: x; }
since symbolic system is negative-capable, by prepending a negative sign, this counter style could generate representation for -1 (which is -x). However, if no range is specified, since the default value of range for symbolic system is '0 infinite', it will fallback when -1 is passed in.

Contrarily, a counter style
@counter-style { system: fixed 1; range: -1 1; symbols: x; }
will always fallback when -1 passed in, not because it is beyond the range specified in 'range' descriptor, but it is beyond the range accepted by the generating algorithm.

Similarly, a counter style
@counter-style { system: additive; additive-symbols: 3 x, 2 y; }
cannot generate representation for 4 because it will be rejected by the algorithm.

> I find the speak-as computation code quite confusing.  First, the name
> ParseSpeakAs is inappropriate for a function that's not doing parsing.
> But it's not clear from the function names what the difference between
> ParseSpeakAs and ComputeSpeakAs is; I'd like to see better names and
> probably also better comments.  It's also not clear why all of these
> functions have *different* checks against
> NS_STYLE_COUNTER_SPEAKAS_STYLE.   It's not clear what the caching in
> mSpeakAs and mSpeakAsCounter means -- and different functions seem to
> cache in these variables in different ways.  I haven't yet reviewed
> these functions (SetSpeakAsAuto, ParseSpeakAs, ComputeSpeakAs,
> GetSpeakAsCounter) in detail because I'd like them to be more
> understandable first.

I agree that it might be the most intricate part in the file. I'll try my best to make it clearer.

> In ComputeOverride:
> 
> >+  if (nextCounter->IsCustomStyle()) {
> >+    mMarks |= MARK_OVERRIDE_VISITED;
> >+    target = static_cast<CustomCounterStyle*>(nextCounter)->ComputeOverride();
> >+    mMarks &= ~MARK_OVERRIDE_VISITED;
> >+  }
> 
> Please assert before adding MARK_OVERRIDE_VISITED that it isn't
> already set.

I don't think it is necessary, since if the MARK_OVERRIDE_VISITED is set, the function will return in just several lines before.

> Please add a comment explaining how the MARK_OVERRIDE_LOOP /
> MARK_OVERRIDE_VISITED code implements this sentence in the spec:
>   # If one or more @counter-style rules form a cycle with their override
>   # values, all of the counter styles participating in the cycle must be
>   # treated as if they were overriding the decimal counter style
>   # instead.
> (My understanding is that what you're doing is setting the mOverride
> of everything *in* the loop to decimal, and making ComputeOverride
> return null when it's returning to another participant in the loop but
> return this when it's returning to something outside the loop.)

Your understanding is correct. I'll add some comments.

> In GetOverrideRoot, could you use a while loop with condition
> (overridden->IsCustomStyle()) rather than using recursion?

I prefer do it recursively since it could make mOverrideRoot in the whole chain be set, which may accelerate when part of a chain is shared between multiple counter styles.

> BuiltinCounterStyleTable:
> 
> >+  static MOZ_CONSTEXPR_VAR uint32_t kLength = 256;
> 
> Instead of doing this, please adjust the NS_STYLE_LIST_STYLE_* constants
> to be consecutive, add an NS_STYLE_LIST_STYLE_MAX constant, and use
> that.

This will be solved in patch 4, since a bunch of constants will be removed in that patch.

> >+static BuiltinCounterStyleTable gBuiltinStyleTable;
> 
> Please make this a BuiltinCounterStyleTable*, initialize it from
> a function called from nsLayoutStatics::Initialize, and free it
> from a function called from nsLayoutStatics::Shutdown.  (We want
> to avoid static constructors, both because code that runs in them
> is hard to use debugging tools with because of locks that are held
> while running them, and for some other reasons.)

According to this, I think I could simply make it an array of BuiltinCounterStyle instead.

As I mentioned in comment 103, when C++14 comes, we could come back to use this structure since it is then possible to initialize the table in compile-time.

> Why do you create a builtin counter style for the empty string in 
> CounterStyleManager::InitializeCacheTable()?  Can it be removed?
> If not, please explain in code comments.

Because I use empty string to indicate 'none' for list-style-type (which can be seen in patch 3). I'll add the comment.

> >+  bool removed = false;
> 
> Please call this remove rather than removed.

"removed" here means the rule of the given style has been removed. So I think removed is a better name.

Comment 132

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #130)
> One other thought about the reference counting -- maybe the
> reference-counting with weak back-pointers to the CounterStyleManager is ok
> if you document that the CounterStyle objects are local to the
> CounterStyleManager and never supposed to be handed out to other objects
> (since they have weak pointers to the CounterStyleManager).

The only possible case that a CounterStyle is no longer owned by the CounterStyleManager but it is still referenced in somewhere else is that, the CounterStyle is removed from the manager because of a style change. Before this change affects the whole tree, some frames or rule nodes may still have reference to the removed CounterStyle. But CounterStyleManager should always overlive CounterStyle objects as it is owned by nsPresContext.

I'll add some document, but where should I add to?

Comment 133

[Jim Michaels]

hi folks, for those of you who wish to calculate the integer number of digits in a number, it's
/* js - this function works with integers, positive, negative, and 0, and tells you the number of digits required. */
function NumberOfDigits(n, base, countMinusSign) {
    //type checking
    if ('number'!=typeof(number)) {
        alert("NumberOfDigits() was fed type "+typeof(n)+" for n, should be Number");
        return 0;//failure
    }
    if ('number'!=typeof(base)) {
        alert("NumberOfDigits() was fed type "+typeof(base)+" for base, should be Number");
        return 0;//failure
    }
    if ('boolean'!=typeof(countMinusSign)) {
        alert("NumberOfDigits() was fed type "+typeof(countMinusSign)+" for countMinusSign, should be Boolean");
        return 0;//failure
    }
    //zero special case
    if (0 == number) {
        return 1;
    }
    //equation
    return Math.ceil(Math.log(Math.abs(number) + 1) / Math.log(base)) + 
           ((countMinusSign && (number < 0))?1:0);
}

this is from my page at http://JesusnJim.com/programming/number-of-digits-in-a-number-for-any-given-base.html#floatingpoint

Comment 134

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #131)
> nsString.h is put for type nsAString, but it seems that a declaration is
> sufficient?

You can also use nsStringFwd.h if you only need forward declarations.

> Reasons:
> 1. range may be specified by authors via 'range' descriptor, which may cover
> zero, so the zero is logically possible to reach here; but
> 2. for negative-capable systems, negative values should be processed in
> GetCounterText, so logically they will never be passed to these functions.
> 
> Though these algorithms never accept zero, processing zero in some earlier
> stage will increase the complexity. Other systems can also fail to process
> some value in the generating algorithm. So I think this is the most suitable
> place to reject the zero.

OK, that sounds like a good reason.

> This XXX comment has been moved in "Additional changes to patch 2", and
> patch 6 fixes that comment in the new place. Should I merge these two
> patches in the next submission?

If I've reviewed both by that time, sure.

> > Is the concept expressed by mSuppressPadding covered in the spec?  If
> > not, could you send comments to www-style explaining how it should be
> > covered?
> 
> In fact, I don't think it is a spec affair. In my understanding, it is more
> an implementation detail about how to display it better. Currently, Firefox
> and Chrome have been using different way to generate the space between
> number and content. Hence, I don't think it is necessary to discuss it in
> the css group.

If it's something that one needs to know to implement list styling in a Web-compatible way or in a way that is viewed as correct by authors or users, it should be in the spec.  I don't see why you think it's not.  (Our default presumption should be that behavior should be defined by the spec unless there's a good reason not for it not to be, and I don't see one it what you've said so far.)

> > >+      CounterValue lowerBound = item->mXValue == kInfinite ?
> > >+        std::numeric_limits<CounterValue>::min() :
> > >+        item->mXValue.GetIntValue();
> > >+      CounterValue upperBound = item->mYValue == kInfinite ?
> > >+        std::numeric_limits<CounterValue>::max() :
> > >+        item->mYValue.GetIntValue();
> > >+      if (aOrdinal >= lowerBound && aOrdinal <= upperBound) {
> > >+        return true;
> > >+      }
> > 
> > Likewise, please restructure this to avoid doing the comparison
> > at all when the value is infinite, instead of playing tricks with
> > std::numeric_limits.
> 
> Why not using std::numeric_limits here? I found it could make the code much
> more complex without this method.

  const nsCSSValue& lowerBound = item->mXValue;
  const nsCSSValue& upperBound = item->mYValue;
  if ((IsCSSValueInfinite(lowerBound) ||
       aOrdinal >= lowerBound.GetIntValue()) &&
      (IsCSSValueInfinite(upperBound) ||
       aOrdinal <= upperBound.GetIntValue())) {
    return true;
  }

doesn't seem much more complex.

> > Should IsOrdinalInRange really always return true for the 'fixed'
> > system?  The spec makes me think not.  (This should have tests added
> > to cover it.)
> 
> Not always, only when the range is not specified by the author. The spec
> says the default 'auto' for 'range' descriptor means "for cyclic, numeric,
> and fixed systems, the range is negative infinity to positive infinity."
> 
> > Also, the spec seems to say that the 'additive' system should support
> > negative values, so the handling of that system also seems wrong.
> > (This should have tests added to cover it.)
> 
> 'additive' system supports negative values by add negative signs in
> GetCounterText. The algorithm itself doesn't generate representation for
> negatives directly.
> 
> > The interaction between IsNegativeCapable and IsOrdinalInRange is
> > a little bit confusing; could you add some comments to the header file
> > to clarify?
> 
> OK, I think you have been confused :)
> I'd like to describe it to you here first.

I figured most of that out by the end, but I still think you need to add code comments explaining it.

> IsNegativeCapable means whether it is possible to generate representation of
> negative values by adding negative sign to the initial representation of
> their absolute value. This capability only depends on the system, and
> completely unrelated to the range. System, such as cyclic and fixed, which
> could generate negative representation via its own algorithm is not
> negative-capable.
> 
> IsOrdinalInRange checks whether an ordinal is in the range of the given
> counter style. It depends on the value specified in 'range' descriptor by
> author. If the value is not specified, some default value is used.
> 
> For example, a counter style
> @counter-style { system: symbolic; range: -1 1; symbols: x; }
> since symbolic system is negative-capable, by prepending a negative sign,
> this counter style could generate representation for -1 (which is -x).
> However, if no range is specified, since the default value of range for
> symbolic system is '0 infinite', it will fallback when -1 is passed in.
> 
> Contrarily, a counter style
> @counter-style { system: fixed 1; range: -1 1; symbols: x; }
> will always fallback when -1 passed in, not because it is beyond the range
> specified in 'range' descriptor, but it is beyond the range accepted by the
> generating algorithm.
> 
> Similarly, a counter style
> @counter-style { system: additive; additive-symbols: 3 x, 2 y; }
> cannot generate representation for 4 because it will be rejected by the
> algorithm.

These seem like good code comments to add, although I'm not sure that a code comment needs quite so many examples.

It's probably also good to say explicitly that fallback occurs *either* if the value is outside of the range *or* if it can't be generated by the system, and that these are two independent concepts.

I think I got confused by reading BuiltinCounterStyle::IsOrdinalInRange first, where they're not treated as independent.  Perhaps it's worth adding a comment there?

> I don't think it is necessary, since if the MARK_OVERRIDE_VISITED is set,
> the function will return in just several lines before.

OK, although it might still be useful documentation of that.

> > In GetOverrideRoot, could you use a while loop with condition
> > (overridden->IsCustomStyle()) rather than using recursion?
> 
> I prefer do it recursively since it could make mOverrideRoot in the whole
> chain be set, which may accelerate when part of a chain is shared between
> multiple counter styles.

Could you add a comment saying that, then?

> According to this, I think I could simply make it an array of
> BuiltinCounterStyle instead.

That's fine, as long as there's no static initialization involved.

> > Why do you create a builtin counter style for the empty string in 
> > CounterStyleManager::InitializeCacheTable()?  Can it be removed?
> > If not, please explain in code comments.
> 
> Because I use empty string to indicate 'none' for list-style-type (which can
> be seen in patch 3). I'll add the comment.

Why not use 'none'?

> > >+  bool removed = false;
> > 
> > Please call this remove rather than removed.
> 
> "removed" here means the rule of the given style has been removed. So I
> think removed is a better name.

It means that the style will be removed towards the end of the function; I think it's bad to use the past tense for an action that hasn't taken place yet.  (It gets set to true both when we have a new @counter-style rule for something we currently have a BuiltinCounterStyle for or when we don't have an @counter-style rule for a style we currently have a CustomCounterStyle for.)


(In reply to Xidorn Quan from comment #132)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #130)
> > One other thought about the reference counting -- maybe the
> > reference-counting with weak back-pointers to the CounterStyleManager is ok
> > if you document that the CounterStyle objects are local to the
> > CounterStyleManager and never supposed to be handed out to other objects
> > (since they have weak pointers to the CounterStyleManager).
> 
> The only possible case that a CounterStyle is no longer owned by the
> CounterStyleManager but it is still referenced in somewhere else is that,
> the CounterStyle is removed from the manager because of a style change.
> Before this change affects the whole tree, some frames or rule nodes may
> still have reference to the removed CounterStyle. But CounterStyleManager
> should always overlive CounterStyle objects as it is owned by nsPresContext.
> 
> I'll add some document, but where should I add to?

Somewhere in the header file -- probably near the functions that return CounterStyle* -- document that all references must be guaranteed to be dropped by the time the Pres context goes away?

Comment 135

[Jim Michaels]

to be technical about it, @counter-style require a descriptor where the descriptor is <list-style-type>. proper syntax is
@counter-style <list-style-type> {
}
for example,
@counter-style decimal-zero-padded-4-width {
    system: numeric;
    symbols: '\30' '\31' '\32' '\33 '\34' '\35' '\36' '\37' '\38' '\39';
    pad: 4 "0"; /can I do 0 width here? does this auto-fill width for whole set?*/
}

http://www.w3.org/TR/css-counter-styles-3/#at-counter-style

Comment 136

[Jim Michaels]

hmm. there was no auto value for pad, and its default is 0 "" and according to spec it's simply the number of symbols, even if the symbol is multiple characters. so 0 is simply a width of 0 (empty). http://www.w3.org/TR/css-counter-styles-3/#pad

this also means that 
@counter-style strange-zero-padded-8-width {
    system: numeric;
    symbols: '01' '23' '45' '67 '89';/*strange numbering system but just an example of 2-character*/
    pad: 4 "00"; /* I *think* it's necessary to double the digits here also or you could get funny results*/
}
this would produce 8 characters for an incrementing numeric-style counter using 4 symbols.
like 00006701

Comment 137

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What are comment 135 and comment 136 in response to?

Comment 138

[Jim Michaels]

comment 135 and comment 136 are in response to seeing @counter-style {} in comment 131.

Comment 139

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8408906 [details] [diff] [review]
patch 3 - link to other parts

This patch was difficult to review, because the decision to have
mCounterStyle be a member of nsStyleList surprised me (though I'm ok
with it, although I might not have made the same choice), and it wasn't
until the end of the patch; it would have been easier if the
invalidation code connecting the frame constructor, pres shell, pres
context, etc., were in a higher patch than that.  But too late for that.

(That decision does complicate the invalidation / flushing / rebuilding
code, since the style data depend on the counter style computation.
e.g., without that, you wouldn't need FlushCounterStyles to call
PostRebuildAllStyleDataEvent, and you could also have your new call
happen later in PresShell::FlushPendingNotifications.)


nsCSSFrameConstructor.cpp:

  Instead of aStyleContext->PresContext(), use mPresShell->GetPresContext()

  In nsCSSFrameConstructor::NotifyCounterStylesAreDirty(), please fix
  the text of the assertion.

nsCounterManager.h:

  Please rename mCounterStyle to mCounterFunction instead of to
  mCounterInfo.


(I probably didn't review the invalidation code in the frame constructor
and pres shell very well, because I was reviewing it with incorrect
assumptions initially, and I only went back to it relatively quickly.
But at this point I'm not going to worry about it; hopefully you're
testing all the right cases.)


Since the style data are shared across frames, I think it's relatively
important to enforce const-correctness, though.  So you should make
nsStyleList::mCounterStyle private, and add a public getter that
returns a |const CounterStyle*| instead of just a |CounterStyle*|,
and then update the users to use that.  This might require that some
additional methods on CounterStyle be const methods (in patch 2).

I think using the empty string in nsStyleList::mListStyleType to
represent 'none' is silly; just use the string 'none'.  If you
initialize it from an NS_LITERAL_STRING in the default constructor, the
string buffer should be shared.  Then the computed style data are
correct and we don't need to worry about edge cases.  (You'll
need to adjust nsBlockFrame::BulletIsEmpty, nsBulletFrame::IsSelfEmpty,
and probably more.)  It would probably be a good idea to add a helper
method bool ListStyleTypeIsNone() const {} to nsStyleList.

I think the bidi stuff in nsBulletFrame::GetListItemText is pretty
suspicious, and more so with the new prefix and suffix support.  I
think it's probably worth investigating whether it's correct, but that
can happen in a FOLLOWUP BUG rather than this one.  (All caps so I can
find it later.)

Please rename nsBlockFrame::GetBulletText to GetSpokenBulletText to
make it clear that it's for the spoken text (since there's now a
distinction).

I don't see any reason you need to declare class CounterStyle in
nsBulletFrame.h.  Please move that to the .cpp file.

In nsHTMLReflowState.cpp, could you just change
  const nsStyleList* styleList = aFrame->StyleList();
to
  auto listStyleType = aFrame->StyleList()->CounterStyle()->GetStyle();
and then replace the "styleList->mListStyleType" with "listStyleType",
instead of using a (much larger) switch?

nsCSSParser.cpp:

In CSSParserImpl::ParseListStyle:

I'd prefer to represent "none" in a less-special way here as well,
i.e., SetStringValue(NS_LITERAL_STRING("none"), eCSSUnit_Ident)
instead of SetNoneValue().

>+  } else if (values[2].GetUnit() == eCSSUnit_Ident) {
>+    nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(
>+        nsDependentString(values[2].GetStringBufferValue()));
>+    if (keyword != eCSSKeyword_UNKNOWN &&
>+        nsCSSProps::FindIndexOfKeyword(
>+            keyword, nsCSSProps::kListStylePositionKTable) >= 0) {
>+      return false;
>+    }

I don't understand why you're adding this.  Is there something in
the spec that says to reject custom values from the shorthand?

In CSSParserImpl::ParseListStyleType:

Again, if you can drop the VARIANT_NONE, that would be less
special-casing of none.

nsCSSPropList.h:
  Please remove the VARIANT_* line and just make it 0 now that
  you're using CSS_PROPERTY_VALUE_PARSER_FUNCTION.

nsStyleStruct.cpp:

nsStyleList's default constructor seems like it needs to set
mCounterStyle to the correct object for having NS_STYLE_LIST_STYLE_DISC.
I think this requires that it take an nsPresContext* argument, as many
of the constructors already do.  Note that this requires you change
the constructor argument to the COMPUTE_START_INHERITED macro in
nsRuleNode::ComputeStyleList.

nsRuleNode.cpp:

Instead of your change to SetDefaultOnRoot, pass the pres context
to the constructor of nsStyleList.

So one rule of Compute*Data methods is that when the value is
eCSSUnit_Null, you're not allowed to touch the data.  It's possible that
you might not be breaking anything in the way you're touching the data
in this particular case, but with the nsStyleStruct.cpp fix above you
should be able to just follow the rule quite easily, by putting your
entire switch **and the if after it** inside a check that the unit is
not null.  Or, perhaps preferably, just make the BuildCounterStyle
call explicitly in both the Initial and Ident cases.

You should also be able to remove the eCSSUnit_None case given the
changes I mentioned for nsCSSParser.

(And I think you can also remove the special empty-string table
that I commented on before.)

nsComputedDOMStyle.cpp:

>-                            "'none' should be handled  as enumerated value");
>+                            "'none' should be handled as identity value");

I'm not sure what an identity value is, but now it should be handled
as a string value.

>+          nsAutoString type;

nsString rather than nsAutoString.

>+            str.Append(type);

This needs nsStyleUtil::AppendEscapedCSSIdent.

Please add cases in property_database.js for list-style-type and
for content that use counter names that require escaping.

>+  const nsString& type = StyleList()->mListStyleType;
>+  // want SetIdent
>+  val->SetString(type.IsEmpty() ? NS_LITERAL_STRING("none") : type);

With my suggested changes you can simplify this as well, to
just val->SetString(StyleList()->mListStyleType);

r=dbaron with those changes

Comment 140

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8410673 [details] [diff] [review]
Additional changes to patch 2

r=dbaron

Comment 141

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #139)
> Comment on attachment 8408906 [details] [diff] [review]
> patch 3 - link to other parts
> 
> This patch was difficult to review, because the decision to have
> mCounterStyle be a member of nsStyleList surprised me (though I'm ok
> with it, although I might not have made the same choice), and it wasn't
> until the end of the patch; it would have been easier if the
> invalidation code connecting the frame constructor, pres shell, pres
> context, etc., were in a higher patch than that.  But too late for that.
> 
> (That decision does complicate the invalidation / flushing / rebuilding
> code, since the style data depend on the counter style computation.
> e.g., without that, you wouldn't need FlushCounterStyles to call
> PostRebuildAllStyleDataEvent, and you could also have your new call
> happen later in PresShell::FlushPendingNotifications.)

What would you decide?

My consideration is that, storing pointer to CounterStyle object could accelerate the rendering by avoiding frames to call BuildCounterStyle every time when they are asked to display. In addition, the rendering code could be simplified. I make the nsStyleList own CounterStyle because it is possible that a CounterStyle is no longer referenced by the manager but still refered by a style structure, as I mentioned in comment 132.

In addition, as anonymous counter styles, which will be implemented in bug 966168, do not interact with other counter styles, it is not necessary for them to be managed by the manager. Hence, with the ability of nsStyleList to own CounterStyle, the implementation of bug 966168 could be simplified as well.

> Since the style data are shared across frames, I think it's relatively
> important to enforce const-correctness, though.  So you should make
> nsStyleList::mCounterStyle private, and add a public getter that
> returns a |const CounterStyle*| instead of just a |CounterStyle*|,
> and then update the users to use that.  This might require that some
> additional methods on CounterStyle be const methods (in patch 2).

I'm affraid that it is hard to do so. Since CounterStyle lazy-loads the value of its fields in the getters, it is impossible to mark most of the methods const. Furthermore, it is even impossible to get rid of lazy-loading, because we cannot solve the override and speak-as loops in constructing stage of the objects. Besides, I don't think it is useful to make it const, since there is no way to change the effective data of a CounterStyle object except altering the related rules.

However, making mCounterStyle private and providing a public getter to access it seem to be a good idea.

Comment 142

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #139)
> Comment on attachment 8408906 [details] [diff] [review]
> patch 3 - link to other parts
> 
> >+  } else if (values[2].GetUnit() == eCSSUnit_Ident) {
> >+    nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(
> >+        nsDependentString(values[2].GetStringBufferValue()));
> >+    if (keyword != eCSSKeyword_UNKNOWN &&
> >+        nsCSSProps::FindIndexOfKeyword(
> >+            keyword, nsCSSProps::kListStylePositionKTable) >= 0) {
> >+      return false;
> >+    }
> 
> I don't understand why you're adding this.  Is there something in
> the spec that says to reject custom values from the shorthand?

I think it is still undecided. This is based on some old comments from Simon and Tab who said that the keywords should be rejected. I'd like to see the decision before changing this behavior.

Comment 143

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you explain how part 4 preserves the case-insensitivity of the previously-builtin names?  I don't see any code to do that, but maybe I'm missing it.

(Would it make sense to make all @counter-style rules in UA sheets have case-insensitive names?)

Comment 144

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, never mind, it's the ToLowerCase() in CSSParserImpl::ParseCounterStyleName.

Comment 145

[Xidorn Quan [:xidorn] UTC+10]

BTW, according to the discussion at [1], patch 4 should be further changed, and the {lower,upper}-{alpha,roman} can be totally removed from native code like most other styles. Maybe I should do it in a followup patch of patch 4 (say patch 4.1) since it will touch code in HTML parsing part?

[1]: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25496

Comment 146

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8391605 [details] [diff] [review]
patch 4 - rewrite some builtin counter styles

For the -moz-persian style in counterstyles.css, use "symbols" rather
than "SYMBOLS".

Could you explain what the deal is with lower/upper-roman/alpha ?  I'd 
rather not have code to do them both ways; we should just have one or
the other.  Does using attribute mapping code and a non-custom constant
require that we keep the builtin counter style code?  If so, I'd just
keep them that way and not add @counter-style for them.  Alternatively,
if there's a way to make those constants use the @counter-style code,
that's fine too, although I'd want to review it.

r=dbaron with that

Comment 147

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8408926 [details] [diff] [review]
patch 4.5 - shrink builtin style table

r=dbaron

Comment 148

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8389785 [details] [diff] [review]
patch 5 - fix fallback problem introduced in patch 2

Hmmm.  I don't like this approach because we really shouldn't expose these new constants to the Web.  (But authors should be allowed to use them as their own names.)  I haven't yet thought of the right way to fix that, though.

(Other than that, this is fine, though.)

Comment 149

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, -moz-x- seems like an unnecessarily complicated prefix, once we're not exposing it.  I'd prefer either -x- (since we already use it for that) or -internal- (since it makes more sense).

Comment 150

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #146)
> Comment on attachment 8391605 [details] [diff] [review]
> patch 4 - rewrite some builtin counter styles
> 
> Could you explain what the deal is with lower/upper-roman/alpha ?  I'd 
> rather not have code to do them both ways; we should just have one or
> the other.  Does using attribute mapping code and a non-custom constant
> require that we keep the builtin counter style code?  If so, I'd just
> keep them that way and not add @counter-style for them.  Alternatively,
> if there's a way to make those constants use the @counter-style code,
> that's fine too, although I'd want to review it.

I misunderstanded the way attribute mapping works. I supposed that custom styles should not be applied to attribute mapping, so I separated them in two different ways. If attribute behavior could be affected by custom styles, then we don't need the builtin part anymore. The non-custom constant could just be converted into the identifier in nsRuleNode (or we could simply remove the constants and let attribute mapping code generate identifier value directly, or even remove the mapping code and use ua.css instead).

However, if attribute mapping should not be affected, I think it is necessary to do them in both ways, or the builtin code has to be changed to meet the identical behavior defined in the spec. For example, people could override upper-roman and specify the range to 0 4000. Then, for ordinal 4000, the native code will fallback and produce '4000', but according to the definition in the spec, it should produce 'MMMM' (though incorrect, it is the behavior).

Comment 151

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #149)
> Also, -moz-x- seems like an unnecessarily complicated prefix, once we're not
> exposing it.  I'd prefer either -x- (since we already use it for that) or
> -internal- (since it makes more sense).

I prepend -moz- here to declare the namespace so that authors will have a lower chance to accidentally redefine these styles. Or we can simply remove the -x- and leave -moz- there?

The current use of -x- seems to be in CSS properties, and it is said that the parser will refused to parse the properties prepended with -x-. But this is impossible for counter styles. The spec does not permit any identifier other than some keywords to be excluded from possible names. And distinguishing parsing ua styles and author-provided styles may complexify parsing and computation code.

Comment 152

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #148)
> Comment on attachment 8389785 [details] [diff] [review]
> patch 5 - fix fallback problem introduced in patch 2
> 
> Hmmm.  I don't like this approach because we really shouldn't expose these
> new constants to the Web.  (But authors should be allowed to use them as
> their own names.)  I haven't yet thought of the right way to fix that,
> though.
> 
> (Other than that, this is fine, though.)

Maybe I can add another class in patch 2, say DependentBuiltinCounterStyle, which depends on CounterStyleManager, and behaves as overriding the builtin style internally, or directly inherits BuiltinCounterStyle?

Comment 153

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8410679 [details] [diff] [review]
patch 7 - tests for @counter-style (WIP)

It lacks a test for interaction between override and speak-as. (That is, a counter style may have 'speak-as' refers to another counter style which overrides the former.)

I'll add it later in a separate patch and merge them when they both get granted.

Comment 154

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - computation of counter style

The modified patch 2.

I plan to create a new patch, say patch 2.5, which will be merged into patch 2, to implement what I described in comment 152 to replace the approach of patch 5.

Comment 155

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2.5 - add dependent builtin style class

As mentioned in comment 154, this patch implements DependentBuiltinCounterStyle which is like BuiltinCounterStyle but managed in the same way as CustomCounterStyle. The new class is introduced to replace the mechanism used in patch 5.

Comment 156

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8410679 [details] [diff] [review]
patch 7 - tests for @counter-style (WIP)

You might not want to review this patch now, since I'd like to add some more tests.

Comment 157

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8410675 [details] [diff] [review]
patch 6 - move builtin styles code

I'd be justified in marking this review- solely on the basis that
the patch description is incorrect, and it does substantially more than
move code.

It's best to have patches that move code be separate from other patches.
Please do that in the future.

That said, in this case I chopped up the diff and reviewed it.

The commit message should mention that it:
 * changes the square character from U+25AA to U+25FE to match the spec
 * changes all the text generating functions to assign rather than append
 * removes CJK negative handling from the counter generation functions
   since it's no longer used there
 * fixes indentation and naming conventions
 * changes the buffer size in DecimalToText
(Really, all 5 of those should have *each* been in a separate patch,
distinct from the moving.)

r=dbaron

Comment 158

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #157)
> Comment on attachment 8410675 [details] [diff] [review]
> patch 6 - move builtin styles code
> 
> I'd be justified in marking this review- solely on the basis that
> the patch description is incorrect, and it does substantially more than
> move code.
> 
> It's best to have patches that move code be separate from other patches.
> Please do that in the future.

I'm sorry for not spliting the changes.

Since the reason mentioned in comment 145, some more code will be removed in patch 4, which will affect this patch. Should I do the separation in that point or simply remove the corresponding code in this patch and mark it review+?

Comment 159

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Changes to patch 3

It is the changed part of patch 3.

I'm not sure if you feel comfortable with some code in this patch, especially the changes in nsStyleStruct.h.

Comment 160

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - rewrite some builtin counter styles

In this patch, {lower,upper}-{roman,alpha} are also removed from the native code. But the constants are still reserved for html attribute mapping.

It is possible to let the attribute mapping generate identifier value directly and completely remove the constants, but it might be worth to happen in a followup patch instead of this.

Comment 161

[Xidorn Quan [:xidorn] UTC+10]

> >+  NS_IMETHOD_(MozExternalRefCountType) AddRef() = 0;
> >+  NS_IMETHOD_(MozExternalRefCountType) Release() = 0;
> 
> These can just be virtual rather than NS_IMETHOD_.  With corresponding
> changes in the cpp file, of course.

Hmm, this change causes compile error in windows. I'll change it back in next submission of patch 2.

Comment 162

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #161)
> > >+  NS_IMETHOD_(MozExternalRefCountType) AddRef() = 0;
> > >+  NS_IMETHOD_(MozExternalRefCountType) Release() = 0;
> > 
> > These can just be virtual rather than NS_IMETHOD_.  With corresponding
> > changes in the cpp file, of course.
> 
> Hmm, this change causes compile error in windows. I'll change it back in
> next submission of patch 2.

Ah, ok.  I guess that's because you're combining it with NS_INLINE_DECL_REFCOUNTING.  So, yes, best to just change it back.  (Though it might be nice to have a variant of NS_INLINE_DECL_REFCOUNTING that contained a MOZ_OVERRIDE.)

Comment 163

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2.5 - add dependent builtin style class

Adds some comments & assertions.

Comment 164

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - tests for @counter-style

Add tests of redefining builtin styles to reftest, and add a11y mochitest for complex speak-as cases.

Comment 165

[Xidorn Quan [:xidorn] UTC+10]

try server for latest version: https://tbpl.mozilla.org/?tree=Try&rev=29bd8bcf5fea

Since this patchset is close to be landed, I think it might be necessary to consider how to deal with the problem caused by bug 989718. Maybe we should append a patch to disable the affected reftests on Android?

Comment 166

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - move builtin styles code with minor changes. r=dbaron

Add the comments according to comment 157.
Since the original patch 5 is no longer needed, this patch takes the part number of that patch.

Comment 167

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8415865 [details] [diff] [review]
patch 2 - computation of counter style

(see previous review comments in comment 129)

You missed these from comment 129:

>Please make this a BuiltinCounterStyleTable*, initialize it from
>a function called from nsLayoutStatics::Initialize, and free it
>from a function called from nsLayoutStatics::Shutdown.  (We want
>to avoid static constructors, both because code that runs in them
>is hard to use debugging tools with because of locks that are held
>while running them, and for some other reasons.)

>But BuiltinCounterStyle should use MOZ_COUNT_CTOR and MOZ_COUNT_DTOR.


Please revert the change of the refcounting methods back to
NS_IMETHOD_(MozExternalRefcountType), per comment 162.

In BuiltinCounterStyle::GetPrefix and GetSuffix, it's probably
better to leave the aResult name (following our normal convention)
rather than switch to aPrefix and aSuffix as you did between the
patches.  Same for CustomCounterStyle.

Please rename CheckReference to CheckReferenceCount or CheckRefCount.

>+    NS_ABORT_IF_FALSE(mSpeakAsCounter,
>+                      "mSpeakAsCounter should have been inited.");

Please write out "initialized" instead of using "inited".

>+static inline bool
>+IsRangeValueInfinite(const nsCSSValue& aValue)
>+{
>+  return aValue.GetUnit() == eCSSUnit_Enumerated &&
>+         aValue.GetKeywordValue() == NS_STYLE_COUNTER_RANGE_INFINITE;
>+}

This should be GetIntValue, not GetKeywordValue.

This actually points out similar mistakes in patch 1:  every occurrence
of GetKeywordValue there should also be GetIntValue.

>+  // If it recursively fallbacks to this counter style again,

"fallbacks" -> "falls back"

>+        // It will make mOverrideRoot in the whole override chain be set

"It will" -> "This will" (I assume; otherwise I don't know what "it"
is)

>+        // recursively. This could accelerate when part of a chain is

"accelerate" -> "save work" (?)




(Also, I ended up having to re-review all the changes in 8410673 because
you merged them in, so they showed up in the diff between patches.  It
would have been better to wait to merge them in.)


comments on the header file:

>+   * cyclic and fixed in which negative signs should not be applied is

"is" -> "are"


Maybe rename IsNegativeCapable to SystemUsesNegativeSign?
depending on response to
http://lists.w3.org/Archives/Public/www-style/2014May/0053.html


And, finally, comments on the bit that I didn't review before, i.e.,
GetSpeakAsAutoValue, ParseRawSpeakAs, and ComputeSpeakAs:

Please rename ParseRawSpeakAs to ComputeRawSpeakAs.

>+// This method corresponds to the first stage of computation of value

"of value" -> "of the value"

>+// of speak-as. It will extract value from rule and possibly recursively

"extract value from rule" -> "extract the value from the rule"

>+// call itself on overridden style to figure out the raw value. To keep

"on overridden style" -> "on the overridden style"

>+// things clear, this method is designed to have no side effect itself

"no side effect" -> "no side effects"
and remove "itself"


>+  // Since counter styles may consist a loop on speak-as, and a counter
>+  // style may be refered in speak-as by its overridden style, and they
>+  // may even be in the same loop, it is possible that we cannot figure
>+  // out speak-as of the style it overrides before we figure out speak-
>+  // as of this style.

This sentence has multiple problems with English (spelling, grammar, and
usage), and I'm also not sure what parts of it mean.  I'll attempt to rewrite
it, but I'm actually not sure what you meant by parts of it:

  Since the speak-as values of counter styles may form a loop, and a
  counter style may have a speak-as value referring to a style that
  overrides it and does not override its speak-as, and a counter style
  may have a speak-as value referring to a style that overrides it and
  whose speak-as value loops back to it, it is possible that we cannot
  figure out the speak-as of the style that this style overrides before
  we figure out the speak-as of this style.

> Hence, it is necessary to divide computation of
>+  // speak-as into two stages:
>+  // 1. figure out the raw value, by ParseRawSpeakAs, and
>+  // 2. eliminate loop, by ComputeSpeakAs.
>+  // See comments before defination of these methods for details.

"defination" -> "definition" (spelling)
"definition" -> "the definitions"

I'm assuming that my proposal in
http://lists.w3.org/Archives/Public/www-style/2014May/0054.html will be
accepted.  If it's not, then GetSpeakAsAuto is wrong and needs to be
more complex, since it would need to pass a CounterStyle* through as
well like ParseRawSpeakAs does.  But I believe my proposal matches what
you implemented and makes sense, but what you implemented does not
quite match the spec.

(But if it is accepted, you'll need a minor change to
CustomCounterStyle::IsOrdinalInRange for the range change proposed in
http://lists.w3.org/Archives/Public/www-style/2014May/0055.html .)

r=dbaron with that

Comment 168

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8416969 [details] [diff] [review]
patch 2.5 - add dependent builtin style class

In CounterStyleManager.h, could you add "for fallback" to the end
of the comment?

It's not clear to me why you changed the test in InvalidateOldStyle
from IsCustomStyle to IsDependentStyle.  IsDependentStyle says
whether a style depends on other styles, but the function cares about
whether other styles depend on it.  (IsDependentStyle also returns
false for custom styles, but that seems fine; see later comments.)
Can you just undo that change?

>+  NS_ABORT_IF_FALSE(aStyle >= 0 && aStyle < NS_STYLE_LIST_STYLE__MAX,

For clarity, switch "aStyle >= 0" to "0 <= aStyle" so that you have
something that looks more like a <= b < c.

IsDependentStyle should be a method on BuiltinCounterStyle instead of
CounterStyle.  The only remaining caller that this would be a problem
for is CheckReference(Count), which can just go back to putting the
check inside an if (aStyle->IsCustomStyle()).

r=dbaron with that

Comment 169

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #167)
> Comment on attachment 8415865 [details] [diff] [review]
> patch 2 - computation of counter style
> 
> (see previous review comments in comment 129)
> 
> You missed these from comment 129:
> 
> >Please make this a BuiltinCounterStyleTable*, initialize it from
> >a function called from nsLayoutStatics::Initialize, and free it
> >from a function called from nsLayoutStatics::Shutdown.  (We want
> >to avoid static constructors, both because code that runs in them
> >is hard to use debugging tools with because of locks that are held
> >while running them, and for some other reasons.)
> 
> >But BuiltinCounterStyle should use MOZ_COUNT_CTOR and MOZ_COUNT_DTOR.

I have made it a static array of BuiltinCounterStyle, and be initialized in CounterStyleManager::InitializeBuiltinCounterStyles. Since BuiltinCounterStyle satisfy conditions of constant initialization (see C++11 spec 3.6.2/2), it should be initialized in compile time. No runtime static constructor should be involved. And since it is a static array, it need not be freed anymore.

MOZ_COUNT_CTOR and DTOR will break the constexpr constraint.

Comment 170

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8416880 [details] [diff] [review]
Changes to patch 3

nsRuleNode.cpp:

>     case eCSSUnit_Ident:
>-      typeValue->GetStringValue(list->mListStyleType);
>-      list->mCounterStyle = nullptr;
>+      list->SetListStyleType(
>+          nsDependentString(typeValue->GetStringBufferValue()), mPresContext);

This breaks string buffer reference counting (otherwise you'd share
the one string buffer specified in the style sheet to every style
struct).  Instead, do:

  case eCSSUnit_Ident: {
    nsString typeIdent;
    typeValue->GetStringValue(typeIdent);
    list->SetListStyleType(typeIdent, mPresContext);····
    break;
  }



Also, in the enumerated case, change GetKeywordValue to GetIntValue.

r=dbaron with that

Comment 171

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #169)
> I have made it a static array of BuiltinCounterStyle, and be initialized in
> CounterStyleManager::InitializeBuiltinCounterStyles. Since
> BuiltinCounterStyle satisfy conditions of constant initialization (see C++11
> spec 3.6.2/2), it should be initialized in compile time. No runtime static
> constructor should be involved. And since it is a static array, it need not
> be freed anymore.
> 
> MOZ_COUNT_CTOR and DTOR will break the constexpr constraint.

ok.

Comment 172

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #168)
> Comment on attachment 8416969 [details] [diff] [review]
> patch 2.5 - add dependent builtin style class
> 
> In CounterStyleManager.h, could you add "for fallback" to the end
> of the comment?

As you didn't quote anything, I'm not quite clear where should I add this comment.
 
> It's not clear to me why you changed the test in InvalidateOldStyle
> from IsCustomStyle to IsDependentStyle.  IsDependentStyle says
> whether a style depends on other styles, but the function cares about
> whether other styles depend on it.  (IsDependentStyle also returns
> false for custom styles, but that seems fine; see later comments.)
> Can you just undo that change?

The comment says this function returns whether a style depends on the manager (not other styles), and this function does return true for custom styles.

> IsDependentStyle should be a method on BuiltinCounterStyle instead of
> CounterStyle.  The only remaining caller that this would be a problem
> for is CheckReference(Count), which can just go back to putting the
> check inside an if (aStyle->IsCustomStyle()).

Can you accept the description above? Do you still think it is better to have it in BuiltinCounterStyle and undo the change in InvalidateOldStyle?

Comment 173

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #172)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #168)
> > Comment on attachment 8416969 [details] [diff] [review]
> > patch 2.5 - add dependent builtin style class
> > 
> > In CounterStyleManager.h, could you add "for fallback" to the end
> > of the comment?
> 
> As you didn't quote anything, I'm not quite clear where should I add this
> comment.

+  // A style is dependent if it depends on the counter style manager.
+  // Custom styles are certainly dependent. In addition, some builtin
+  // style are dependent as well.

(the only change in CounterStyleManager.h in that patch)

> > It's not clear to me why you changed the test in InvalidateOldStyle
> > from IsCustomStyle to IsDependentStyle.  IsDependentStyle says
> > whether a style depends on other styles, but the function cares about
> > whether other styles depend on it.  (IsDependentStyle also returns
> > false for custom styles, but that seems fine; see later comments.)
> > Can you just undo that change?
> 
> The comment says this function returns whether a style depends on the
> manager (not other styles), and this function does return true for custom
> styles.
> 
> > IsDependentStyle should be a method on BuiltinCounterStyle instead of
> > CounterStyle.  The only remaining caller that this would be a problem
> > for is CheckReference(Count), which can just go back to putting the
> > check inside an if (aStyle->IsCustomStyle()).
> 
> Can you accept the description above? Do you still think it is better to
> have it in BuiltinCounterStyle and undo the change in InvalidateOldStyle?

What you have is ok; I missed the
+    case NS_STYLE_LIST_STYLE_CUSTOM:

Maybe, in IsDependentStyle, add comments saying which subclass of CounterStyle we get for the types (i.e., CUSTOM gets CustomCounterStyle, the other ones listed get DependentBuiltinCounterStyle, and everything else gets BuiltinCounterStyle).

Comment 174

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #165)
> Since this patchset is close to be landed, I think it might be necessary to
> consider how to deal with the problem caused by bug 989718. Maybe we should
> append a patch to disable the affected reftests on Android?

The tests should not be disabled, but it's reasonable to mark them as expected to fail.  Then when the bug is fixed the failure marking has to get removed, since there will be an unexpected pass if it's not.

(This affects only the most negative integer, right?)

Comment 175

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8416976 [details] [diff] [review]
patch 7 - tests for @counter-style

I'm not going to get to reviewing the tests before I leave, so I'm going to hand off this review request to Jonathan.

I'm also going to ask David Bolter to review the a11y tests (although he's certainly free to delegate that to somebody else).

And I'll stick a feedback? back in my own queue to check that it's done when I'm back.

Comment 176

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch for marking reftests affected by bug 989718 known fail

This patch marks two reftests as known fail in ARM platform because of bug 989718.

I'm going to merge this patch into patch 3 once it gets review granted.

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #174)
> (In reply to Xidorn Quan from comment #165)
> > Since this patchset is close to be landed, I think it might be necessary to
> > consider how to deal with the problem caused by bug 989718. Maybe we should
> > append a patch to disable the affected reftests on Android?
> 
> The tests should not be disabled, but it's reasonable to mark them as
> expected to fail.  Then when the bug is fixed the failure marking has to get
> removed, since there will be an unexpected pass if it's not.
> 
> (This affects only the most negative integer, right?)

Right, it affects only the most negative one.

Comment 177

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8416976 [details] [diff] [review]
patch 7 - tests for @counter-style

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

r=me Thanks! (nit below)

::: accessible/tests/mochitest/name/a11y.ini
@@ +12,5 @@
>  [test_list.html]
>  [test_markup.html]
>  [test_svg.html]
>  [test_tree.xul]
> +[test_counterstyle.html]

(nit: This list should stay alphabetical)

Comment 178

[Jonathan Kew [:jfkthame]]

Comment on attachment 8416976 [details] [diff] [review]
patch 7 - tests for @counter-style

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

One key thing seems to be missing here: add an 'include' of the new counter-style directory in the top-level layout/reftests/reftest.list, otherwise they won't get run.

Which means that unfortunately, the new reftests have not actually been run by the tryserver jobs above. E.g. looking at the full reftest logs from comment #165, there are no /counter-style/ tests to be found. They're probably fine (I haven't looked through all the files yet), but please push a new try job with all the reftests enabled, so we can see how that goes.

Comment 179

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #178)
> Comment on attachment 8416976 [details] [diff] [review]
> patch 7 - tests for @counter-style
> 
> Review of attachment 8416976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One key thing seems to be missing here: add an 'include' of the new
> counter-style directory in the top-level layout/reftests/reftest.list,
> otherwise they won't get run.
> 
> Which means that unfortunately, the new reftests have not actually been run
> by the tryserver jobs above. E.g. looking at the full reftest logs from
> comment #165, there are no /counter-style/ tests to be found. They're
> probably fine (I haven't looked through all the files yet), but please push
> a new try job with all the reftests enabled, so we can see how that goes.

What a silly mistake...

It shows some failures after link that part in: https://tbpl.mozilla.org/?tree=Try&rev=a70c73bdfaba
I'll try to fix them.

Comment 180

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - tests for @counter-style, r=dbolter

Updated patch of tests.

With the patch I submitted in bug 1011461, all tests are passed: https://tbpl.mozilla.org/?tree=Try&rev=2c7cc0549db1

Comment 181

[Jonathan Kew [:jfkthame]]

Comment on attachment 8424306 [details] [diff] [review]
patch 6 - tests for @counter-style, r=dbolter

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

This looks like an awesome collection of tests! I admit I didn't cross-check every detail against the spec, but they look logical and thorough, and I'm confident you're testing the right things.

I expect a few details may end up being updated again as a result of the CSS WG decision on handling spacing between the marker and list item as part of the suffix, but that's fine - we can handle that in the followup bug for that fix.

One request before we land all this: Some of the reftest *-ref files (e.g. system-cyclic-ref.html) have comments that divide them into sections, indicating how they correspond to the counter styles in the testcase, but others (e.g. system-additive-ref.html) don't have these. If you could include comments like that in each of the references, I think it'd be really helpful for later readers when comparing the test and ref files.

::: layout/reftests/reftest.list
@@ +371,5 @@
>  # encodings
>  include ../../dom/encoding/test/reftest/reftest.list
> +
> +# counter-style
> +include counter-style/reftest.list

I realize this file isn't 100% sorted, but please move this up so it's next to the existing entry for counters instead of appending here.

Comment 182

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The CSS WG discussed the counter styles spec and made a number of decision; tentative minutes are at:
http://krijnhoetmer.nl/irc-logs/css/20140519#l-1492

Comment 183

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #182)
> The CSS WG discussed the counter styles spec and made a number of decision;
> tentative minutes are at:
> http://krijnhoetmer.nl/irc-logs/css/20140519#l-1492

Hmmm... So many decisions... Those decisions will affect every patch in the set. Do I need to request review for these changes?

One more question about the 'speak-as: spell-out': is there any existing function which can generate the spell-out form of a given string?

Comment 184

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #183)
> (In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from
> comment #182)
> > The CSS WG discussed the counter styles spec and made a number of decision;
> > tentative minutes are at:
> > http://krijnhoetmer.nl/irc-logs/css/20140519#l-1492
> 
> Hmmm... So many decisions... Those decisions will affect every patch in the
> set. Do I need to request review for these changes?

I think it's probably better to do an additional patch on top of these and then request review for that, rather than modifying the existing patches.

> One more question about the 'speak-as: spell-out': is there any existing
> function which can generate the spell-out form of a given string?

Ask David Bolter?

Comment 185

[Jonathan Kew [:jfkthame]]

Comment on attachment 8420523 [details] [diff] [review]
patch for marking reftests affected by bug 989718 known fail

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

Please also add a comment "# bug 989718" to the affected lines in the manifest.

Comment 186

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #184)
> (In reply to Xidorn Quan from comment #183)
> > (In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from

> > One more question about the 'speak-as: spell-out': is there any existing
> > function which can generate the spell-out form of a given string?
> 
> Ask David Bolter?

Hi. Testing the literal expectations (via testName) seems right to me (which you already do), but maybe I'm not understanding the question?

Comment 187

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Bolter [:davidb] from comment #186)
> (In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from
> comment #184)
> > (In reply to Xidorn Quan from comment #183)
> > > (In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from
> 
> > > One more question about the 'speak-as: spell-out': is there any existing
> > > function which can generate the spell-out form of a given string?
> > 
> > Ask David Bolter?
> 
> Hi. Testing the literal expectations (via testName) seems right to me (which
> you already do), but maybe I'm not understanding the question?

We have a new value called "spell-out" for "speak-as", and this value requires the UA spell out the generated counter representation letter-by-letter in the document language. Say, if we have a counter value generates "αβγ", then it should be pronounced as "alpha beta gamma" in English document. Is there any existing function to do this transformation?

Comment 188

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Or, more realistically, 'spell-out' means that the string "at" should be read as "eh tee" rather than as the word "at", because it's part of a list that's going aq, ar, as, at, au, av.  But there's an alternative that says the opposite, for example if the list markers are "one", "two", "three", ...)

Comment 189

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #188)
> (Or, more realistically, 'spell-out' means that the string "at" should be
> read as "eh tee" rather than as the word "at", because it's part of a list
> that's going aq, ar, as, at, au, av.  But there's an alternative that says
> the opposite, for example if the list markers are "one", "two", "three", ...)

I'm not aware of an existing function for these cases (nor the "alpha beta gamma" case).

Comment 190

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Bolter [:davidb] from comment #189)
> (In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from
> comment #188)
> > (Or, more realistically, 'spell-out' means that the string "at" should be
> > read as "eh tee" rather than as the word "at", because it's part of a list
> > that's going aq, ar, as, at, au, av.  But there's an alternative that says
> > the opposite, for example if the list markers are "one", "two", "three", ...)
> 
> I'm not aware of an existing function for these cases (nor the "alpha beta
> gamma" case).

Given this, I'd like to leave that value unsupported. As 'spell-out' seems to be a part of CSS Speech spec, it should be implemented at some future time. We can add that value back then.

Comment 191

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #190)
> Given this, I'd like to leave that value unsupported. As 'spell-out' seems
> to be a part of CSS Speech spec, it should be implemented at some future
> time. We can add that value back then.

That's sounds ok with me for now, though you should file a followup bug on it.  (I'm assuming the speech code will do something reasonable with "B." "C.", etc.)

What remains to get this landed?  Just adding a patch on top to implement the name changes in the WG resolutions (without the spell-out value)?

Comment 192

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #191)
> (In reply to Xidorn Quan from comment #190)
> > Given this, I'd like to leave that value unsupported. As 'spell-out' seems
> > to be a part of CSS Speech spec, it should be implemented at some future
> > time. We can add that value back then.
> 
> That's sounds ok with me for now, though you should file a followup bug on
> it.  (I'm assuming the speech code will do something reasonable with "B."
> "C.", etc.)
> 
> What remains to get this landed?  Just adding a patch on top to implement
> the name changes in the WG resolutions (without the spell-out value)?

1. I found some decisions made in the discussion [1] haven't been reflected in the draft, including the new name for 'override', the interaction between 'pad' and 'negative'. Should I change that according to the logs without waiting for the draft?
2. I haven't seen the concrete solution for [2] in the spec.
3. I was waiting for patch of bug 1013160 to be landed (It should be landed soon)

I have started integrating these patches on top of the latest trunk, and will soon submit some additional patches for review. Since the patches have not been landed, I prefer posting changes on each patches separately, is that good?

I still need some time to change and test the new code. Sorry for the delay.

[1]: http://krijnhoetmer.nl/irc-logs/css/20140519#l-1492
[2]: http://lists.w3.org/Archives/Public/www-style/2014May/0187.html

Comment 193

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's fine.  I just really wanted to make sure you weren't waiting on something from one of us.

Comment 194

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And I think it's fine to make changes based on the logs, but it's worth keeping track of them to make sure the changes make it into the spec.)

Comment 195

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4.1

I noticed that @import rules are removed from ua.css and added to nsLayoutStylesheetCache, so I do the same thing to counterstyles.css.

Comment 196

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1.1

Changes will be merged into patch 1

Comment 197

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2.1

Changes will be merged into patch 2

Comment 198

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6.1

Changes will be merged into patch 6

Comment 199

[Xidorn Quan [:xidorn] UTC+10]

These incremental patches follows the newly updated draft and the discussion in minutes of CSSWG, so it should meet the next LCWD which will be published soon.

Some trivial changes are not included in these patches, especially those changes for resolving conflicts. Hence, the patches here is not able to be cleanly merged. I will merge related patches and submit a tidy version of the patchset after all patches get granted.

Try server result can be found here: https://tbpl.mozilla.org/?tree=Try&rev=893371485571 . There is one problem in this result, which is the R15 of B2G Emulator. I think it is a bug of B2G. It incorrectly moves U+3099, voiced sound mark of japanese kana, by 1em. And there seems to be some other rendering bug causes the difference. Should I file a bug for it, or is there any existing bug related to it? Should I mark this test skip-if(B2G)?

Comment 200

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan (UTC+10) from comment #199)
> These incremental patches follows the newly updated draft and the discussion
> in minutes of CSSWG, so it should meet the next LCWD which will be published
> soon.
> 
> Some trivial changes are not included in these patches, especially those
> changes for resolving conflicts. Hence, the patches here is not able to be
> cleanly merged. I will merge related patches and submit a tidy version of
> the patchset after all patches get granted.
> 
> Try server result can be found here:
> https://tbpl.mozilla.org/?tree=Try&rev=893371485571 . There is one problem
> in this result, which is the R15 of B2G Emulator. I think it is a bug of
> B2G. It incorrectly moves U+3099, voiced sound mark of japanese kana, by
> 1em. And there seems to be some other rendering bug causes the difference.
> Should I file a bug for it, or is there any existing bug related to it?
> Should I mark this test skip-if(B2G)?

I believe the problem here occurs because the Japanese font used by default on B2G (MotoyaLMaru) doesn't support U+3099, and so this character falls back to Droid Sans Fallback; but breaking the font run between the base character and the mark results in poor positioning. U+3099 ends up as a zero-width glyph, but it's positioned relative to the origin in such a way that it overstrikes the *following* base character. :(

The test would probably pass if you explicitly specified "Droid Sans Fallback" in the font-family list (though I haven't actually tested this).

(Normally, I'd suggest that

  symbols: \304B\3099 \304D\3099 \304F\3099 \3051\3099 \3053\3099;

is not a good way to express this particular style; it'd be better to write

  symbols: \304C \304E \3050 \3052 \3054;

But I assume you did this deliberately to test the behavior of padding when a combining mark is present.)

Another possible fix might be to use Latin with accents instead of Japanese in this test; something like

  symbols: a a\0301 a\0302 a\0303 a\0304;

Comment 201

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #200)
> I believe the problem here occurs because the Japanese font used by default
> on B2G (MotoyaLMaru) doesn't support U+3099, and so this character falls
> back to Droid Sans Fallback; but breaking the font run between the base
> character and the mark results in poor positioning. U+3099 ends up as a
> zero-width glyph, but it's positioned relative to the origin in such a way
> that it overstrikes the *following* base character. :(
> 
> The test would probably pass if you explicitly specified "Droid Sans
> Fallback" in the font-family list (though I haven't actually tested this).
> 
> (Normally, I'd suggest that
> 
>   symbols: \304B\3099 \304D\3099 \304F\3099 \3051\3099 \3053\3099;
> 
> is not a good way to express this particular style; it'd be better to write
> 
>   symbols: \304C \304E \3050 \3052 \3054;
> 
> But I assume you did this deliberately to test the behavior of padding when
> a combining mark is present.)
> 
> Another possible fix might be to use Latin with accents instead of Japanese
> in this test; something like
> 
>   symbols: a a\0301 a\0302 a\0303 a\0304;

This solution sounds reasonable. I will use them instead. Nevertheless, I think it is still a defect should be fixed in Firefox OS, since U+3099 is a symbol exists since Unicode 1.0, and should be properly supported in Japanese environment.

Comment 202

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2.1

Modify some comments according to the suggestion in comment 167 in addition.

Comment 203

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6.1

Modify the test according to comment 200.

The new test server result: https://tbpl.mozilla.org/?tree=Try&rev=f618e6f497c0

Comment 204

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8438384 [details] [diff] [review]
patch 6.1

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

r=me for the a11y parts.

::: accessible/tests/mochitest/name/test_counterstyle.html
@@ +83,5 @@
> +      function testRule(aRule, aNames, aTodo)
> +      {
> +        testName(aRule + "-1", aNames[0], null, aTodo);
> +        testName(aRule + "-7", aNames[1], null, aTodo);
> +        testName(aRule + "-29", aNames[2], null, aTodo);

nit: no need for spacer lines here.

@@ +141,5 @@
> +      for (var j = 0; j < values.length; j++) {
> +        var item = document.createElement("li");
> +        item.id = rule.name + '-' + values[j];
> +        item.value = values[j];
> +        item.textContent = values[j];

nit: no need for the spacer lines here.

Comment 205

[David Bolter [:davidb] (NeedInfo me for attention)]

Ignore my nits above - sorry - it was just weird line wrap on my end.

Comment 206

[alexander :surkov (:asurkov)]

Comment on attachment 8438384 [details] [diff] [review]
patch 6.1

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

::: accessible/tests/mochitest/name/test_counterstyle.html
@@ +79,5 @@
>    <script type="application/javascript">
>  
>      function doTest()
>      {
> +      function testRule(aRule, aNames, aTodo)

is there follow up bug for these todos?

Comment 207

[Xidorn Quan [:xidorn] UTC+10]

(In reply to alexander :surkov from comment #206)
> Comment on attachment 8438384 [details] [diff] [review]
> patch 6.1
> 
> Review of attachment 8438384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/name/test_counterstyle.html
> @@ +79,5 @@
> >    <script type="application/javascript">
> >  
> >      function doTest()
> >      {
> > +      function testRule(aRule, aNames, aTodo)
> 
> is there follow up bug for these todos?

I'm going to open one after patches of this bug get landed. Should I open it in advance? I think it is not actually a bug before we have these patches landed.

Comment 208

[Jonathan Kew [:jfkthame]]

Comment on attachment 8438384 [details] [diff] [review]
patch 6.1

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

I haven't attempted to review all the updates here line-by-line, but overall it looks fine; if tryserver is happy with it, so am I. :)

Comment 209

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan (UTC+10) from comment #201)
>  Nevertheless, I
> think it is still a defect should be fixed in Firefox OS, since U+3099 is a
> symbol exists since Unicode 1.0, and should be properly supported in
> Japanese environment.

True, although the fact that common Japanese fonts don't support it implies that it's probably not widely used.

Anyhow, I'd suggest filing a bug about this so that we don't entirely forget it. The fix would be a font update of some kind - either improving or replacing the Japanese font used on FxOS.

Comment 210

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan (UTC+10) from comment #207)
> I'm going to open one after patches of this bug get landed. Should I open it
> in advance? I think it is not actually a bug before we have these patches
> landed.

Yes, open the bug now (explaining the situation), and put the bug number in a comment next to the todo.

Comment 211

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8437631 [details] [diff] [review]
patch 1.1

Instead of doing this:

>+        if (aValue.GetUnit() == eCSSUnit_Enumerated &&
>+            aValue.GetIntValue() == NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT) {
>+          // Currently spell-out is not supported, so it is explicitly
>+          // rejected here rather than mis-parsed as a identifier.
>+          return false;
>+        }

could you just comment out the spell-out value in the keyword table:

> const KTableValue nsCSSProps::kCounterSpeakAsKTable[] = {
>+  eCSSKeyword_bullets, NS_STYLE_COUNTER_SPEAKAS_BULLETS,
>+  eCSSKeyword_numbers, NS_STYLE_COUNTER_SPEAKAS_NUMBERS,
>+  eCSSKeyword_words, NS_STYLE_COUNTER_SPEAKAS_WORDS,
>+  eCSSKeyword_spell_out, NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT,
>   eCSSKeyword_UNKNOWN, -1
> };

with the same comment?

r=dbaron with that

Comment 212

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8432057 [details] [diff] [review]
patch 4.1

You should also remove the ua.css change from the existing patch 4.

r=dbaron with that

Comment 213

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8438381 [details] [diff] [review]
patch 2.1

>+  } else if (IsExtendsSystem() && value.GetUnit() == eCSSUnit_None) {

I don't understand this value.GetUnit() test.  Why isn't this
case used just "if (IsExtendsSystem())"?

(Maybe explain in a comment if there's a good reason.)



This patch would have been a bit easier to review if it had been split
into one patch for each conceptual change (renaming to extends, renaming
to use-negative-sign, IsOrdinalInAutoRange, etc.).

I probably didn't review the interesting changes (e.g., IsOrdinalInAutoRange)
that carefully as a result.

but r=dbaron anyway, with the comment at top

Comment 214

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-4) from comment #212)
> Comment on attachment 8432057 [details] [diff] [review]
> patch 4.1
> 
> You should also remove the ua.css change from the existing patch 4.
> 
> r=dbaron with that

Yes, I have done that locally. Since that caused a conflict, I removed that directly in the original patch.

Comment 215

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-4) from comment #211)
> Comment on attachment 8437631 [details] [diff] [review]
> patch 1.1
> 
> Instead of doing this:
> 
> >+        if (aValue.GetUnit() == eCSSUnit_Enumerated &&
> >+            aValue.GetIntValue() == NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT) {
> >+          // Currently spell-out is not supported, so it is explicitly
> >+          // rejected here rather than mis-parsed as a identifier.
> >+          return false;
> >+        }
> 
> could you just comment out the spell-out value in the keyword table:
> 
> > const KTableValue nsCSSProps::kCounterSpeakAsKTable[] = {
> >+  eCSSKeyword_bullets, NS_STYLE_COUNTER_SPEAKAS_BULLETS,
> >+  eCSSKeyword_numbers, NS_STYLE_COUNTER_SPEAKAS_NUMBERS,
> >+  eCSSKeyword_words, NS_STYLE_COUNTER_SPEAKAS_WORDS,
> >+  eCSSKeyword_spell_out, NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT,
> >   eCSSKeyword_UNKNOWN, -1
> > };
> 
> with the same comment?

Commenting out it here will make spell-out be parsed as a custom-ident, which can have potential problem on compatibility as author may have a counter-style with this name. I want to avoid this case, so I explicitly reject the value instead of let it fallthrough silently. Are you happy with this description?

Comment 216

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, ok, so maybe instead change "misparsed as an identifier" to "parsed as a custom identifier"?

Comment 217

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-4) from comment #213)
> Comment on attachment 8438381 [details] [diff] [review]
> patch 2.1
> 
> >+  } else if (IsExtendsSystem() && value.GetUnit() == eCSSUnit_None) {
> 
> I don't understand this value.GetUnit() test.  Why isn't this
> case used just "if (IsExtendsSystem())"?
> 
> (Maybe explain in a comment if there's a good reason.)

It was your proposal, isn't it? Explicitly specifying 'range: auto' is different from omitting the descriptor for extends system.

Comment 218

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 1 - parse @counter-style rule. r=dbaron,bz

Comment 219

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 2 - computation of counter style. r=dbaron

Comment 220

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 3 - link to other parts. r=dbaron

Comment 221

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 4 - rewrite some builtin counter styles. r=dbaron

Comment 222

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 5 - move computation code with minor changes. r=dbaron

Comment 223

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 6 - tests for @counter-style. r=jfkthame,davidb

Comment 224

[Xidorn Quan [:xidorn] UTC+10]

https://tbpl.mozilla.org/?tree=Try&rev=5fbebeef4737

Comment 225

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d21febcb08a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/001216bdc605
https://hg.mozilla.org/integration/mozilla-inbound/rev/fffcb4bbc8b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bffb0b2f7c2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6576396848f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/34456c194587

Comment 226

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d21febcb08a3
https://hg.mozilla.org/mozilla-central/rev/001216bdc605
https://hg.mozilla.org/mozilla-central/rev/fffcb4bbc8b1
https://hg.mozilla.org/mozilla-central/rev/bffb0b2f7c2f
https://hg.mozilla.org/mozilla-central/rev/6576396848f1
https://hg.mozilla.org/mozilla-central/rev/34456c194587

Comment 227

[Sebastian Zartner [:sebo]]

Wouldn't this be worth mentioning in the release notes for Firefox 33?

Sebastian

Comment 228

[Florian Bender]

Are the patches in this bug useful on their own and the feature enabled by default? Otherwise, what else needs to be done for this work to be usable by web devs?

Release Note Request (optional, but appreciated)
[Why is this notable]: new CSS feature
[Suggested wording]: Implemented @counter-style rule from CSS3 Counter Styles specification.
[Links (documentation, blog post, etc)]: (tbd)

Comment 229

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Florian Bender from comment #228)
> Are the patches in this bug useful on their own and the feature enabled by
> default? Otherwise, what else needs to be done for this work to be usable by
> web devs?
> 
> Release Note Request (optional, but appreciated)
> [Why is this notable]: new CSS feature
> [Suggested wording]: Implemented @counter-style rule from CSS3 Counter
> Styles specification.
> [Links (documentation, blog post, etc)]: (tbd)

Yes, the feature has been enabled by default. Documentation link is the URL of this bug.

Comment 230

[Florian Bender]

Thanks for the prompt info!

Comment 231

[Sylvestre Ledru [:Sylvestre]]

Thanks. Added in the release notes with "@counter-style rule from CSS3 Counter Styles specification implemented"