Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 1 • 10 years ago

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.

	Gordon P. Hemsley [:GPHemsley]
	Comment 2 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 3 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 4 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 5 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 6 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 7 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 8 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 9 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 10 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 11 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 12 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 13 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 14 • 10 years ago

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.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 15 • 10 years ago

(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 :)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 16 • 10 years ago

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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 17 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 18 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 19 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 20 • 10 years ago

(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.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 26 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 27 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 28 • 10 years ago

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

The rewrite of patch 2 will influence this patch.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 29 • 10 years ago

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

Rewrite of patch 2 may also affect this patch.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 34 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 35 • 10 years ago

Sorry for uploading a wrong patch.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 36 • 10 years ago

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.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 37 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 38 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 44 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 45 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 46 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 47 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 48 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 49 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 50 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 51 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 52 • 10 years ago

Correct some bugs found by tests.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 53 • 10 years ago

Fix bug & integrate test modification

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 54 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 55 • 10 years ago

Change part number.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 57 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 58 • 10 years ago

Sorry, I forgot to refresh the patch...

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 59 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 60 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 61 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 62 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 63 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 65 • 10 years ago

Affected by change in patch 2.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 66 • 10 years ago

Affected by changes in patch 2.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 67 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 10 years ago

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

r=me on the DOM/classinfo bits

	Gordon P. Hemsley [:GPHemsley]
	Comment 69 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 70 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 71 • 10 years ago

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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 72 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 73 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 74 • 10 years ago

This patch solves the case-sensitivity problem.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 75 • 10 years ago

This version removes some useless conditions.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 76 • 10 years ago

Solve the case-sensitivity problem.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 77 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 78 • 10 years ago

Add one test for name case-sensitivity.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 79 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 80 • 10 years ago

(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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 81 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 82 • 10 years ago

(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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 83 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 84 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 85 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 86 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 87 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 88 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 89 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 90 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 91 • 10 years ago

(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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 92 • 10 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 93 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 94 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 95 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 96 • 10 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 97 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 98 • 10 years ago

(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.

	Jonathan Kew [:jfkthame]
	Comment 99 • 10 years ago

(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]

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 100 • 10 years ago

(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 :)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 101 • 10 years ago

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.)

	Jonathan Kew [:jfkthame]
	Comment 102 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 103 • 10 years ago

(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.

	Jonathan Kew [:jfkthame]
	Comment 104 • 10 years ago

: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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 105 • 10 years ago

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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 106 • 10 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 107 • 10 years ago

(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?)

	Jonathan Kew [:jfkthame]
	Comment 108 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 109 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 110 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 111 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 112 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 113 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 114 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 116 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 118 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 119 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 120 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 121 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 122 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 123 • 10 years ago

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|.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 124 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 125 • 10 years ago

Fix some problems.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 126 • 10 years ago

Update because of changes to patch 2

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 127 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 128 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 129 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 130 • 10 years ago

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).

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 131 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 132 • 10 years ago

(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?

	Jim Michaels
	Comment 133 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 134 • 10 years ago

(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?

	Jim Michaels
	Comment 135 • 10 years ago

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

	Jim Michaels
	Comment 136 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 137 • 10 years ago

What are comment 135 and comment 136 in response to?

	Jim Michaels
	Comment 138 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 139 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 140 • 10 years ago

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

r=dbaron

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 141 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 142 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 143 • 10 years ago

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?)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 144 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 145 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 146 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 147 • 10 years ago

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

r=dbaron

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 148 • 10 years ago

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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 149 • 10 years ago

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).

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 150 • 10 years ago

(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).

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 151 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 152 • 10 years ago

(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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 153 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 154 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 155 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 156 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 157 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 158 • 10 years ago

(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+?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 159 • 10 years ago

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.