1060090 - Refactor CSS <position>-handling code to be more granular (handling individual <position> representations), for usage beyond "background-position"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Rename ParseBackgroundPositionValues to ParsePositionValue

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

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: Rename ParseBackgroundPositionValues to ParsePositionValue, and drop unused 'aAcceptsInherit' arg

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

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: spin out function to generate computed style for a single <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.)

Comment 4

[Daniel Holbert [:dholbert]]

part 2 also updates some stale documentation for ComputeBackgroundPositionCoord (it currently says it takes "an nsCSSValue object", but really it takes two nsCSSValues)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 3: Refactor getComputedStyle() implementation for background-position

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.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 3 v2: Refactor getComputedStyle() implementation for background-position

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 4: Support custom (percent-valued) initial values for <position>

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

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: (wrong patch)

Last patch here -- this generalizes the <position>-related StyleAnimationValue code. (refactoring functionality out of 'background-position'-specific chunks into helper methods).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 5: Generalize StyleAnimationValue code for handling <position>

(sorry, attached the wrong file. Here's the real 'part 5', to generalize the animation code.)

Comment 10

[Daniel Holbert [:dholbert]]

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.

Comment 11

[Cameron McCormack (:heycam)]

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?

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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