1271510 - check computed outline-width value in nsStyleOutline::CalcDifference

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In nsStyleOutline::CalcDifference we currently check mOutlineWidth for differences when returning substantive change hints.  Slightly better would be to check mCachedOutlineWidth (which I'm renaming here to mComputedOutlineWidth) since that is the field that is used for layout.  This will help with stylo, where Servo will write into mComputedOutlineWidth and leave mOutlineWidth empty.

mOutlineWidth, which stores a value half way between specified and computed value, is only used for (a) an implementation detail for Gecko style struct computation (see the comment above nsStyleBorder::mBorder, which applies to this field too) and (b) for extracting the current computed value for animation (this is the field listed in nsCSSPropList.h).  (a) does not apply in stylo, where Servo manages producing style structs for us, and (b) is something to deal with once we work out how animation will work in stylo.

(b) is also related to bug 1271489.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Rename nsStyleOutline::mCachedOutlineWidth to mComputedOutlineWidth.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Look at mComputedOutlineWidth in nsStyleOutline::CalcDifference.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)
> In nsStyleOutline::CalcDifference we currently check mOutlineWidth for
> differences when returning substantive change hints.  Slightly better would
> be to check mCachedOutlineWidth (which I'm renaming here to
> mComputedOutlineWidth) since that is the field that is used for layout.

This is what we currently do for nsStyleBorder::mBorder/mComputedBorder, too.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8750577 [details] [diff] [review]
Part 1: Rename nsStyleOutline::mCachedOutlineWidth to mComputedOutlineWidth.

Review of attachment 8750577 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure "Computed" is the correct label here.

At least -- we have this comment currently:
> 1319   // Note that this is a specified value.  You can get the actual values
> 1320   // with GetOutlineWidth.  You cannot get the computed value directly.
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#1319

Right now, your patch makes the above-quoted comment contradict the code.  (It says "You cannot get the computed value directly", AND YET we have a getter that would now explicitly "return mComputedOutlineWidth;")

This comment is making a distinction between the "actual value" (which is the variable you're renaming in this patch, the thing returned by the GetOutlineWidth getter) vs. the "computed value" (which the comment declares you "cannot get...directly").

So: perhaps this variable should really be renamed to mActualOutlineWidth?

Comment 5

[Daniel Holbert [:dholbert]]

In particular: it seems like nsStyleOutline::RecalcData sets this variable (currently called mCachedOutlineWidth) to "0" if border-style is none.  This is not a computed-style thing -- this variable stores something more complex than simply the "computed" outline-width.

So "Actual" (or something like that) does seem like a more correct name than "computed"...

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8750578 [details] [diff] [review]
Part 2: Look at mComputedOutlineWidth in nsStyleOutline::CalcDifference.

Review of attachment 8750578 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.cpp
@@ +567,5 @@
>                             mOutlineStyle != NS_STYLE_BORDER_STYLE_NONE;
>    bool outlineIsVisible  = aOther.mComputedOutlineWidth > 0 &&
>                             aOther.mOutlineStyle != NS_STYLE_BORDER_STYLE_NONE;
>    if (outlineWasVisible != outlineIsVisible ||
> +      (outlineIsVisible &&

Do we need to check/compare these "outlineWasVisible"/"outlineIsVisible" booleans at all anymore?

The variable that you're making us change to inspect here (called mComputedOutlineWidth at this point in the queue) *already includes information about whether mOutlineStyle is NONE*, via nsStyleOutline::RecalcData().

Before this patch, we *did* need to check |outlineIsVisible| because mOutlineWidth (a "specified value") may contain something nonzero & pixel-valued which does not actually produce a visible outline. But if we switch to comparing mComputedOutlineWidth, that's no longer the case...

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> In particular: it seems like nsStyleOutline::RecalcData sets this variable
> (currently called mCachedOutlineWidth) to "0" if border-style is none.  This
> is not a computed-style thing -- this variable stores something more complex
> than simply the "computed" outline-width.
> 
> So "Actual" (or something like that) does seem like a more correct name than
> "computed"...

I think you are right that "actual" is more accurate.  Though note that the clamping to 0 if outline-style is none is something that the spec requires of the computed value, so it is still close to the computed value.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Do we need to check/compare these "outlineWasVisible"/"outlineIsVisible"
> booleans at all anymore?

Good point, we don't!

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 1: Rename nsStyleOutline::mCachedOutlineWidth to mActualOutlineWidth.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 2: Improve nsStyleOutline::CalcDifference.

Two other improvements here: we no longer return RepaintFrame when mOutline{Style,Color,Radius} changed when the outline was not visible, and we no longer return UpdateOverflow|SchedulePaint when mTwipsPerPixel is different, since that is only used in RecalcData so its effects are already incorporated into mActualOutlineWidth.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8751036 [details] [diff] [review]
Part 1: Rename nsStyleOutline::mCachedOutlineWidth to mActualOutlineWidth.

Review of attachment 8751036 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed:

::: layout/style/nsStyleStruct.h
@@ +1317,5 @@
>    nsStyleCorners  mOutlineRadius; // [reset] coord, percent, calc
>  
> +  // This is the specified value of outline-width, but with length values
> +  // computed to absolute.  mActualOutlineWidth stores the outline-width
> +  // value used by layout.  (We must store this value for the same

It's ambiguous what you mean by "this value" here -- mOutlineWidth vs. mActualOutlineWidth.

(I suspect you really are talking about mOutlineWidth, since the comment is right before the declaration of that variable.  On the other hand, the most recent thing that this comment has mentioned, just before this point, is mActualOutlineWidth.  So, "this value" could easily be interpreted as referring to either of these variables here.)

Maybe just clarify by mentioning the name of the variable -- e.g.:
  [...]
  //  value used by layout. (We must store this value -- mOutlineWidth --
  //  for the same [etc.]
  [...]

@@ +1319,5 @@
> +  // This is the specified value of outline-width, but with length values
> +  // computed to absolute.  mActualOutlineWidth stores the outline-width
> +  // value used by layout.  (We must store this value for the same
> +  // style struct resolution reasons that we do nsStyleBorder::mBorder;
> +  // see that fields comment.)

s/fields/field's/

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8751037 [details] [diff] [review]
Part 2: Improve nsStyleOutline::CalcDifference.

Review of attachment 8751037 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, just one commit message nit:
> Bug 1271510 - Part 2: Improve nsStyleOutline::CalcDifference

This is a bit vague. :)

Maybe clarify by adding: "...to make better use of mActualOutlineWidth."  (since that's the central theme of this patch's changes to this function, I think -- all the other changes are a product of the fact that we're now checking that variable as a first-class thing.)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1657b330198c
https://hg.mozilla.org/integration/mozilla-inbound/rev/631073c3d1ac

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1657b330198c
https://hg.mozilla.org/mozilla-central/rev/631073c3d1ac