Closed
Bug 1060090
Opened 10 years ago
Closed 10 years ago
Refactor CSS <position>-handling code to be more granular (handling individual <position> representations), for usage beyond "background-position"
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(5 files, 3 obsolete files)
5.49 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
5.61 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
4.89 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
3.84 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
10.16 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
The CSS Backgrounds spec defines the <position> CSS value type[1], and defines "background-position" as taking a comma-separated list of <position> values. (where each list entry corresponds to a different background image) We have code for parsing/computing/etc. <position> values, but it's currently all tied to "background-position". As a result, this code largely deals with objects corresponding to "list-of-<position>", instead of just <position>. I'm planning on adding support for "object-position"[3], which just takes a single <position> value[4]. (Not a list of <position>s.) So, I'd like to refactor our existing <position> code to be less background-specific and less list-dependent. [1] http://dev.w3.org/csswg/css-backgrounds/#position [2] http://dev.w3.org/csswg/css-backgrounds/#background-position [3] bug 1055285 [4] http://dev.w3.org/csswg/css-images-3/#object-position
Assignee | ||
Updated•10 years ago
|
Summary: Refactor out code for handling CSS <position> values, from being specific to "background-position" → Refactor CSS <position>-handling code to be less list-dependent, for usage beyond "background-position"
Assignee | ||
Comment 1•10 years ago
|
||
This patch is just a function-rename (and documentation tweak). It renames ParseBackgroundPositionValues() to ParsePositionValue(), because it parses a single CSS <position> value. Notably, I'm changing "Values" to "Value" in the function name. I think its name is plural in current mozilla-central because <position> takes several sub-values -- but I think it's clearer to have the name be singular, because otherwise, it sounds a bit like it parses multiple entire <position> values (which it does *not* do -- it just parses a single <position>).
Attachment #8480947 -
Flags: review?(cam)
Assignee | ||
Comment 2•10 years ago
|
||
(Just noticed that 'aAcceptsInherit' arg is always false, too, so we can just drop it. It makes sense for callers to handle 'inherit' where appropriate, and outsource *just* the <position>-parsing to this function.)
Attachment #8480953 -
Flags: review?(cam)
Assignee | ||
Updated•10 years ago
|
Attachment #8480947 -
Attachment is obsolete: true
Attachment #8480947 -
Flags: review?(cam)
Assignee | ||
Updated•10 years ago
|
Summary: Refactor CSS <position>-handling code to be less list-dependent, for usage beyond "background-position" → Refactor CSS <position>-handling code to be more granular (handling individual <position> representations), for usage beyond "background-position"
Assignee | ||
Comment 3•10 years ago
|
||
This refactors the nsRuleNode.cpp code for generating the computed "background-position" (from the specified style). Right now, the code for this expects to be passed a nsCSSValueList (a link in the <position>-list chain), from which it grabs out the <position> (a CSS array). This function refactors that functionality out so that we don't need to start with a nsCSSValueList. (so that we can use this function for object-position, which only has a single <position> instead of a list) This also renames the helper-method "ComputeBackgroundPositionCoord" to just "ComputePositionCoord". (i.e., compute the "x" or "y" coord of a single <position> value.)
Attachment #8480971 -
Flags: review?(cam)
Assignee | ||
Comment 4•10 years ago
|
||
part 2 also updates some stale documentation for ComputeBackgroundPositionCoord (it currently says it takes "an nsCSSValue object", but really it takes two nsCSSValues)
Assignee | ||
Comment 5•10 years ago
|
||
This refactors the nsComputedDOMStyle (i.e. getComputedStyle) to use a helper-function for a single <position> value, plus a helper-function for each of its components (its X and Y), to clean up the existing copypasted code there.
Attachment #8481011 -
Flags: review?(cam)
Assignee | ||
Comment 6•10 years ago
|
||
(Updating part 3 to drop an unnecessary blank line & fix a bug caught by a test failure -- this had a mXPosition that was supposed to be mYPosition.)
Attachment #8481011 -
Attachment is obsolete: true
Attachment #8481011 -
Flags: review?(cam)
Attachment #8483721 -
Flags: review?(cam)
Updated•10 years ago
|
Attachment #8480953 -
Flags: review?(cam) → review+
Updated•10 years ago
|
Attachment #8480971 -
Flags: review?(cam) → review+
Updated•10 years ago
|
Attachment #8483721 -
Flags: review?(cam) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Thanks for the reviews! I'm attaching two more patches here, and then this bug's done. This one, part 4, lets us customize the initial value of a <position>-valued property. Right now, Position::SetInitialValues() sets the member-vars to represent "0% 0%", which is appropriate for 'background-position', but not for 'object-position' (whose initial value will be "50% 50%"). I'm generalizing that function to let the initial (percentage) value to be customized. (We could generalize it further, but there's no need, as long as all <position>-based properties start out at "X% X%" for some X, which is currently the case.)
Attachment #8486137 -
Flags: review?(cam)
Assignee | ||
Updated•10 years ago
|
Attachment #8486137 -
Attachment description: part 4: Support different (percent-valued) initial values for <position> → part 4: Support nonzero (percent-valued) initial values for <position>
Assignee | ||
Updated•10 years ago
|
Attachment #8486137 -
Attachment description: part 4: Support nonzero (percent-valued) initial values for <position> → part 4: Support custom (percent-valued) initial values for <position>
Assignee | ||
Comment 8•10 years ago
|
||
Last patch here -- this generalizes the <position>-related StyleAnimationValue code. (refactoring functionality out of 'background-position'-specific chunks into helper methods).
Attachment #8486139 -
Flags: review?(cam)
Assignee | ||
Updated•10 years ago
|
Attachment #8486139 -
Attachment description: part 5: Generalize StyleAnimationValue code for handling <position> → (wrong patch)
Attachment #8486139 -
Attachment is obsolete: true
Attachment #8486139 -
Flags: review?(cam)
Assignee | ||
Comment 9•10 years ago
|
||
(sorry, attached the wrong file. Here's the real 'part 5', to generalize the animation code.)
Attachment #8486141 -
Flags: review?(cam)
Assignee | ||
Comment 10•10 years ago
|
||
NOTE: There's one thing about this StyleAnimationValue code that didn't initially make sense to me, & which merits a mention here -- the usage of nsCSSValue::Array. Basically: in specified-style *from a user-provided value*, a <position> is represented as a 4-entry nsCSSValue::Array, with two entries for optional edge keywords, and 2 entries for offsets from those edges. Now, in StyleAnimationValue.cpp, we're dealing with <position> values that have *already been normalized* to remove the need for edge keywords, so we technically don't need all four entries. But we still use a 4-entry array, and we just ignore two of the entries. This is what initially seemed odd to me -- we've got those two wasted entries. For efficiency, we theoretically *could* just use a nsCSSValuePair or something like that. But i think the overhead is probably worth it (or at least, it's not worth shaking things up), because: (a) it reduces the number of different ways that we represent these properties. (i.e. it makes us consistent with the nsCSSParser representation), which is good from a code-complexity/grokability standpoint. (b) It means StyleAnimationValue::UncomputeValue() can be trivial for 'background-position', since we've already got our value in 'specified style' form. So, I've left this as-is, and I added some documentation in "SetPositionValue" (at the end of the patch) to hopefully make it a little clearer for the next person to stumble on this code.
Updated•10 years ago
|
Attachment #8486137 -
Flags: review?(cam) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8486141 [details] [diff] [review] part 5: Generalize StyleAnimationValue code for handling <position> Review of attachment 8486141 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments addressed. ::: layout/style/StyleAnimationValue.cpp @@ +410,5 @@ > + "Expected two arrays"); > + > + PixelCalcValue calcVal[4]; > + > + nsCSSValue::Array* bgArray = aPos1.GetArrayValue(); Call this something like "posArray" since it will soon be used for something other than background-position. I'd be happy with just "pos", too. @@ +1900,5 @@ > +{ > + const nsCSSValue::Array* bgPos1 = aPos1.GetArrayValue(); > + const nsCSSValue::Array* bgPos2 = aPos2.GetArrayValue(); > + nsCSSValue::Array* bgPosRes = nsCSSValue::Array::Create(4); > + aResultPos.SetArrayValue(bgPosRes, eCSSUnit_Array); Rename these bgPos* variables too. Maybe bgPos1 should become posArray1 (to match the rename I suggest in CalcPositionSquareDistance; or just pos1 if you leave out the "Array" in that other function). Also, adjust the indentation in the function now that it's moved. @@ +1902,5 @@ > + const nsCSSValue::Array* bgPos2 = aPos2.GetArrayValue(); > + nsCSSValue::Array* bgPosRes = nsCSSValue::Array::Create(4); > + aResultPos.SetArrayValue(bgPosRes, eCSSUnit_Array); > + > + /* Only iterate over elements 1 and 3. The background position is s/background position/<position> value/ perhaps? @@ +2823,5 @@ > return true; > } > > +static void > +SetPositionValue(const nsStyleBackground::Position &aPos, nsCSSValue& aCSSValue) "&" next to the type. @@ +2825,5 @@ > > +static void > +SetPositionValue(const nsStyleBackground::Position &aPos, nsCSSValue& aCSSValue) > +{ > + nsRefPtr<nsCSSValue::Array> bgArray = nsCSSValue::Array::Create(4); s/bgArray/posArray/ or another name to match the other changes you make. @@ +2834,5 @@ > + // <position> representation is to store edge names. But for values > + // extracted from computed style (which is what we're dealing with here), > + // we'll just have a normalized "x,y" position, with no edge names needed. > + nsCSSValue &xValue = bgArray->Item(1), > + &yValue = bgArray->Item(3); I realise you're just moving code, but I'd rather see: nsCSSValue& xValue = ...; nsCSSValue& yValue = ...; to avoid needing the space before the "&". Make that change while moving it?
Attachment #8486141 -
Flags: review?(cam) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Thanks -- addressed all those comments, and landed: part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/35022df4f995 part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/b744556f8b6e part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8175b16840e part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/548a5f132ea1 part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/e056b5b87c29
Flags: in-testsuite-
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/35022df4f995 https://hg.mozilla.org/mozilla-central/rev/b744556f8b6e https://hg.mozilla.org/mozilla-central/rev/d8175b16840e https://hg.mozilla.org/mozilla-central/rev/548a5f132ea1 https://hg.mozilla.org/mozilla-central/rev/e056b5b87c29
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•