Closed
Bug 966168
Opened 10 years ago
Closed 9 years ago
Implement symbols() CSS function
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 2 open bugs, )
Details
(Keywords: css3, dev-doc-complete)
Attachments
(1 file, 5 obsolete files)
50.96 KB,
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
Implement symbols() function defined in CSS Counter Styles Level 3, which can be used to define anonymous counter styles.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Why there is an eCSSUnit_Function with many independent CSS function units? What's the difference between them? Should we merge all of them into a unified mechanism? What is the preferable method to add this new function, using eCSSUnit_Function or adding a new unit? I think we may need a new list similar to nsCSSPropList (maybe call CSSFuncList) to provide a unified mechanism to process CSS functions.
Flags: needinfo?(dbaron)
Comment 2•10 years ago
|
||
Why do you need a unified mechanism? eCSSUnit_Function is for when it makes more sense to have a set of functions all have the same unit(and distinguish them as keywords); other units are for when it makes more sense to have a specific unit for that function.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8445122 -
Flags: review?(jfkthame)
Comment 4•10 years ago
|
||
Comment on attachment 8445122 [details] [diff] [review] patch Review of attachment 8445122 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, but I think David should take a look, as I'm not really familiar with all the CSS parsing stuff, etc. ::: layout/style/CounterStyleManager.cpp @@ +1706,5 @@ > + > +/* virtual */ void > +AnonymousCounterStyle::GetNegative(NegativeType& aResult) > +{ > + aResult.before.AssignLiteral(MOZ_UTF16("-")); Any reason to use this rather than simply aResult.before = '-'; (by analogy with GetSuffix above)? ::: layout/style/nsCSSParser.cpp @@ +7114,5 @@ > + > + bool first = true; > + nsCSSValueList* item = symbols.SetListValue(); > + for (;;) { > + // Should also include VARIANT_IMAGE File a followup bug to add support for this? ::: layout/style/nsRuleNode.cpp @@ +7063,5 @@ > break; > } > + case eCSSUnit_Symbols: > + // XXX We should save the raw CSS value so that > + // it can be serialized in nsComputedDOMStyle. What do we currently get?
Attachment #8445122 -
Flags: review?(jfkthame) → review?(dbaron)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #4) > Comment on attachment 8445122 [details] [diff] [review] > patch > > Review of attachment 8445122 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good to me, but I think David should take a look, as I'm not > really familiar with all the CSS parsing stuff, etc. > > ::: layout/style/CounterStyleManager.cpp > @@ +1706,5 @@ > > + > > +/* virtual */ void > > +AnonymousCounterStyle::GetNegative(NegativeType& aResult) > > +{ > > + aResult.before.AssignLiteral(MOZ_UTF16("-")); > > Any reason to use this rather than simply > > aResult.before = '-'; > > (by analogy with GetSuffix above)? I'd prefer replacing the statement in GetSuffix, as it seems that assigning a single char could also cause an additional memory allocating in nsString. It could be a problem for GetNegative as it uses nsString to store the symbols, while it might not a true problem for GetSuffix as the caller could use nsAutoString to receive the result. > ::: layout/style/nsCSSParser.cpp > @@ +7114,5 @@ > > + > > + bool first = true; > > + nsCSSValueList* item = symbols.SetListValue(); > > + for (;;) { > > + // Should also include VARIANT_IMAGE > > File a followup bug to add support for this? Maybe bug 1024179 is enough? I think they are highly related, and could be fixed together. > ::: layout/style/nsRuleNode.cpp > @@ +7063,5 @@ > > break; > > } > > + case eCSSUnit_Symbols: > > + // XXX We should save the raw CSS value so that > > + // it can be serialized in nsComputedDOMStyle. > > What do we currently get? We currently get an empty string for this case.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Xidorn Quan (UTC+10) from comment #5) > (In reply to Jonathan Kew (:jfkthame) from comment #4) > > Comment on attachment 8445122 [details] [diff] [review] > > patch > > > > ::: layout/style/nsRuleNode.cpp > > @@ +7063,5 @@ > > > break; > > > } > > > + case eCSSUnit_Symbols: > > > + // XXX We should save the raw CSS value so that > > > + // it can be serialized in nsComputedDOMStyle. > > > > What do we currently get? > > We currently get an empty string for this case. I have an idea to fix it now.
Assignee | ||
Updated•10 years ago
|
Attachment #8445122 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•9 years ago
|
||
The updated patch. dbaron: It seems you are busy now. I'm fine with either you redirecting the review request to someone else, or doing the review when you are back from your busyness.
Attachment #8445122 -
Attachment is obsolete: true
Attachment #8479601 -
Flags: review?(dbaron)
Comment 8•9 years ago
|
||
Comment on attachment 8479601 [details] [diff] [review] patch nsCounterManager.cpp: >+ mCounterStyle = manager->BuildCounterStyle( >+ nsDependentString(style.GetStringBufferValue())); This should instead use: nsString ident; /* or other appropriate name for variable */ style.GetStringValue(ident); manager->BuildCounterStyle(ident); This reference counts the string buffer instead of building a dependent buffer, which allows further sharing the buffer with the cache table. symbols-function-invalid{,-ref}.html: You should make the first declaration be lower-greek or similar, so that it's more clear that that's the one declaration that's used rather than one of the later ones. CounterStyleManager.cpp: >+/* virtual */ bool >+AnonymousCounterStyle::IsOrdinalInAutoRange(CounterValue aOrdinal) >+{ >+ return IsOrdinalInRange(aOrdinal); >+} Maybe return AnonymousCounterStyle::IsOrdinalInRange(aOrdinal) so that you don't go through virtual dispatch a second time? >+/* virtual */ uint8_t >+AnonymousCounterStyle::GetSpeakAs() >+{ >+ switch (mSystem) { >+ case NS_STYLE_COUNTER_SYSTEM_ALPHABETIC: >+ return NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT; >+ case NS_STYLE_COUNTER_SYSTEM_CYCLIC: >+ return NS_STYLE_COUNTER_SPEAKAS_BULLETS; >+ default: >+ return NS_STYLE_COUNTER_SPEAKAS_NUMBERS; >+ } >+} Maybe put this in a common function to share with the second half of CustomCounterStyle::GetSpeakAsAutoValue? (In general, I wonder if it might make sense to share a little more code with CustomCounterStyle, at the cost of having slightly weaker assertions about mSystem? A bunch of these functions could be shared a bit better. Maybe worth doing in a followup patch?) nsCSSParser.cpp: >+ bool ParseCounterStyleName(nsCSSValue& aValue); I'm not happy about having three functions with the same name that aren't true overloads (i.e., same semantics but different parameters). Is there any better naming that could be done here? (If you were starting from the beginning, it also would have been nice to put that refactoring in a separate patch.) CSSParserImpl::ParseSymbols: >+ if (!ParseEnum(type, nsCSSProps::kCounterSystemKTable)) { >+ type.SetIntValue(NS_STYLE_COUNTER_SYSTEM_SYMBOLIC, eCSSUnit_Enumerated); >+ } else { >+ switch (type.GetIntValue()) { >+ case NS_STYLE_COUNTER_SYSTEM_CYCLIC: >+ case NS_STYLE_COUNTER_SYSTEM_NUMERIC: >+ case NS_STYLE_COUNTER_SYSTEM_ALPHABETIC: >+ case NS_STYLE_COUNTER_SYSTEM_SYMBOLIC: >+ case NS_STYLE_COUNTER_SYSTEM_FIXED: >+ break; >+ default: >+ SkipUntil(')'); >+ return false; >+ } >+ } You should create a separate nsCSSProps::...KTable with just the 5 values that are valid here. Then you can remove the entire else{} above. >+ // Should also include VARIANT_IMAGE Could you file a followup bug on adding support for image, and reference it in this comment? (Also, start the comment with "FIXME: ".) You also need to add "case ecSSUnit_Symbols:" to nsCSSValue::SizeOfExcludingThis. nsComputedDOMStyle.cpp: >+ tmp.Truncate(tmp.Length() - 1); >+ tmp.Append(')'); This could be a single call to ReplaceLiteral. nsRuleNode.cpp: >+ case eCSSUnit_Symbols: >+ list->SetListStyleType(NS_LITERAL_STRING(""), >+ mPresContext->CounterStyleManager()-> >+ BuildCounterStyle(typeValue->GetArrayValue())); >+ break; Please indent as: + case eCSSUnit_Symbols: + list->SetListStyleType(NS_LITERAL_STRING(""), + mPresContext->CounterStyleManager()-> + BuildCounterStyle(typeValue->GetArrayValue())); + break; (that is, with two more spaces on the line that's a continuation from ->.) I think you also need to revise nsComputedDOMStyle::DoGetContent. property_database.js: Please add tests to the counter() and counters() functions as well. reftests: Please add reftests covering symbols() inside of counter() and counters(). It might also make sense to add tests that the symbols() function is not accepted as a value of @counter-style descriptors: system: extends symbols(); speak-as: symbols(); fallback: symbols(); r=dbaron with that, although I'm a little suspicious that there may be additional code missing for the case of symbols() inside of counter() and counters(), in which case I'd like to review that code as well. (I should probably review the DoGetContent changes, at least.)
Attachment #8479601 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8493572 -
Flags: review?(dbaron)
Assignee | ||
Comment 10•9 years ago
|
||
Sorry for the noise. One change I forgot to include.
Attachment #8493572 -
Attachment is obsolete: true
Attachment #8493572 -
Flags: review?(dbaron)
Attachment #8493573 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•9 years ago
|
||
All tests in Linux x64 Opt are passed: https://tbpl.mozilla.org/?tree=Try&rev=fe8bb377a98f
Updated•9 years ago
|
Attachment #8493573 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 12•9 years ago
|
||
The merged version of the two patches.
Attachment #8479601 -
Attachment is obsolete: true
Attachment #8493573 -
Attachment is obsolete: true
Attachment #8494816 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d53b25dcf2d7
Keywords: checkin-needed
Comment 14•9 years ago
|
||
sorry had to backout for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=48842844&tree=Mozilla-Inbound
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 15•9 years ago
|
||
Looks like an #include problem affecting non-unified builds. You can add the configure flag --disable-unified-compilation in your mozconfig to reproduce and test this locally.
Assignee | ||
Comment 16•9 years ago
|
||
I add an "#include <nsCSSValue.h>" to CounterStyleManager.h, and it should be good now: https://tbpl.mozilla.org/?tree=Try&rev=9e1e941ba256
Attachment #8494816 -
Attachment is obsolete: true
Attachment #8495133 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da6f9114503
Keywords: checkin-needed
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1da6f9114503
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 19•9 years ago
|
||
We triaged recent bugs in need of doc (new process). Documenting this bug means updating this page (and Fx 35 for devs): https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type and creating this one: https://developer.mozilla.org/en-US/docs/Web/CSS/symbol
Comment 20•8 years ago
|
||
:jsx documented this a few months ago. Thank you https://developer.mozilla.org/en-US/Firefox/Releases/35#CSS https://developer.mozilla.org/en-US/docs/Web/CSS/symbols and https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•