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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(5 files, 3 obsolete files)

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
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"
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)
(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)
Attachment #8480947 - Attachment is obsolete: true
Attachment #8480947 - Flags: review?(cam)
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"
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)
part 2 also updates some stale documentation for ComputeBackgroundPositionCoord (it currently says it takes "an nsCSSValue object", but really it takes two nsCSSValues)
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)
(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)
Depends on: 1063815
Attachment #8480953 - Flags: review?(cam) → review+
Attachment #8480971 - Flags: review?(cam) → review+
Attachment #8483721 - Flags: review?(cam) → review+
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)
Attachment #8486137 - Attachment description: part 4: Support different (percent-valued) initial values for <position> → part 4: Support nonzero (percent-valued) initial values for <position>
Attachment #8486137 - Attachment description: part 4: Support nonzero (percent-valued) initial values for <position> → part 4: Support custom (percent-valued) initial values for <position>
Attached patch (wrong patch) (obsolete) — Splinter Review
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)
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)
(sorry, attached the wrong file. Here's the real 'part 5', to generalize the animation code.)
Attachment #8486141 - Flags: review?(cam)
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.
Attachment #8486137 - Flags: review?(cam) → review+
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: