1320239 - Use nscoord for some nsStyleCoord in style structs that only carry nscoord

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jeremy Chen ([e-mail])]

At present, we only use nscoord value for the computed value of -webkit-text-stoke-width [1], maybe it's better to just use nscoord instead of nsStyleCoord.


[1] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/layout/style/nsRuleNode.cpp#4882-4899

Comment 1

[Astley Chen (inactive)]

mOutlineWidth & mStrokeWidth are both using nsStyleCoord to keep the computed value. What's the benefit to do so ?

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

Review commit: https://reviewboard.mozilla.org/r/95626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95626/

Comment 3

[Jeremy Chen ([e-mail])]

(In reply to Astley Chen [:astley] (UTC+8) from comment #1)
> mOutlineWidth & mStrokeWidth are both using nsStyleCoord to keep the
> computed value. What's the benefit to do so ?

Just found this while I was implementing stylo part of -webkit-text-stroke in https://github.com/servo/servo/pull/14358. Since the only reason we used the union type was to carry the coord distinction, I can't see why we shall use nsStyleCoord.

More discussion can be found in bug 443057 (as I specified in see also). I haven't checked mOutlineWidth & mStrokeWidth yet. If they're in the similar situation, then we should do this for them as well.

Comment 4

[Jeremy Chen ([e-mail])]

Comment on attachment 8814351 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95626/diff/1-2/

Comment 5

[Jeremy Chen ([e-mail])]

According to the latest SVG spec. [1], SVG's stroke-width should be computed to "absolute length or percentage". So, maybe this is not the case.

As to the latest CSS3 UI spec. [2], outline-width should be computed to "absolute length; 0 if the outline style is none.". So, maybe this is the case.

Hi Cameron, do you think this is something that we should do?


[1] https://svgwg.org/specs/strokes/#StrokeWidth
[2] https://drafts.csswg.org/css-ui/#outline-width

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> According to the latest SVG spec. [1], SVG's stroke-width should be computed
> to "absolute length or percentage". So, maybe this is not the case.

Agreed.

> As to the latest CSS3 UI spec. [2], outline-width should be computed to
> "absolute length; 0 if the outline style is none.". So, maybe this is the
> case.

Yes.  I don't see any need to store the keyword values in mOutlineWidth.  (Just like we don't for border-width.)

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8814351 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

https://reviewboard.mozilla.org/r/95626/#review95772

::: layout/style/nsRuleNode.cpp:4882
(Diff revision 2)
>  
>    // -webkit-text-stroke-color: color, string, inherit, initial
>    setComplexColor(aRuleData->ValueForWebkitTextStrokeColor(),
>                    &nsStyleText::mWebkitTextStrokeColor);
>  
>    // -webkit-text-stroke-width: length, inherit, initial, enum

Would be nice if we could share code for border-*-width, -webkit-text-stroke-width and column-rule-width.

Maybe we could have a SetLineWidthValue helper function, which takes parameters like SetValue, except with a lambda for the second parameter, so we could pass in the way to set the border/column-rule-width values, since they need a function call?

::: layout/style/nsRuleNode.cpp:4896
(Diff revision 2)
> -      nsPresContext::GetBorderWidthForKeyword(webkitTextStrokeWidthValue->GetIntValue()));
> -  } else {
> -    SetCoord(*webkitTextStrokeWidthValue, text->mWebkitTextStrokeWidth,
> -             parentText->mWebkitTextStrokeWidth,
> -             SETCOORD_LH | SETCOORD_CALC_LENGTH_ONLY |
> -               SETCOORD_CALC_CLAMP_NONNEGATIVE |
> +      nsPresContext::GetBorderWidthForKeyword(webkitTextStrokeWidthValue->GetIntValue());
> +  } else if (webkitTextStrokeWidthValue->GetUnit() == eCSSUnit_Inherit ||
> +             webkitTextStrokeWidthValue->GetUnit() == eCSSUnit_Unset) {
> +    text->mWebkitTextStrokeWidth = parentText->mWebkitTextStrokeWidth;
> +    conditions.SetUncacheable();
> +  } else if (webkitTextStrokeWidthValue->GetUnit() == eCSSUnit_Initial ) {

Nit: space after "eCSSUnit_Initial" can be removed.

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8814351 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

https://reviewboard.mozilla.org/r/95626/#review95772

> Would be nice if we could share code for border-*-width, -webkit-text-stroke-width and column-rule-width.
> 
> Maybe we could have a SetLineWidthValue helper function, which takes parameters like SetValue, except with a lambda for the second parameter, so we could pass in the way to set the border/column-rule-width values, since they need a function call?

The idea of sharing the code does make sense.
Work on new patches, so clear review request for now.

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

This is the first patch which creates a helper function to compute a line-width
value to an absolute length. With this helper, we could share the computation
for other line-width value properties, e.g., border-width, column-rule-width.

Review commit: https://reviewboard.mozilla.org/r/95930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95930/

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1320239 - use ComputeLineWidthValue helper to set column-rule-width.

Review commit: https://reviewboard.mozilla.org/r/95932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95932/

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Bug 1320239 - use ComputeLineWidthValue helper to set border-width.

Review commit: https://reviewboard.mozilla.org/r/95934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95934/

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: Bug 1320239 - use nscoord instead of nsStyleCoord for outline-width.

Review commit: https://reviewboard.mozilla.org/r/95936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95936/

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8814351 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95626/diff/2-3/

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8814828 [details]
Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

https://reviewboard.mozilla.org/r/95624/#review95906

I've no idea how to create a single SetLineWidthValue helper function for all line-width value properties without complicating our code. So, I created a ComputeLineWidthValue helper just for sharing the common computation of line-width. If there's a better implementation design, I'd be happy to further polish these patches. Any feedback would be appreciated.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8814829 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set column-rule-width.

https://reviewboard.mozilla.org/r/95932/#review96292

::: layout/style/nsRuleNode.cpp:9161
(Diff revision 1)
> -  if (eCSSUnit_Initial == widthValue.GetUnit() ||
> -      eCSSUnit_Unset == widthValue.GetUnit()) {
> -    column->SetColumnRuleWidth(
> -        nsPresContext::GetBorderWidthForKeyword(NS_STYLE_BORDER_WIDTH_MEDIUM));
> -  }
> -  else if (eCSSUnit_Enumerated == widthValue.GetUnit()) {
> +    ComputeLineWidthValue<eUnsetInitial>(
> +      *aRuleData->ValueForColumnRuleWidth(),
> +      parent->GetComputedColumnRuleWidth(),
> +      nsPresContext::GetBorderWidthForKeyword(NS_STYLE_BORDER_WIDTH_MEDIUM),
> +      aContext, mPresContext, conditions);
> +  if (coord) {

Personally I'd prefer it checks `coord.isSome()` rather than checking `coord` directly. The latter could be confusing.

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> I've no idea how to create a single SetLineWidthValue helper function for
> all line-width value properties without complicating our code. So, I created
> a ComputeLineWidthValue helper just for sharing the common computation of
> line-width. If there's a better implementation design, I'd be happy to
> further polish these patches. Any feedback would be appreciated.

I think what you've got here looks good.  I was going to suggest using a lambda instead of returning the value, but I think returning the value looks fine.  (And I think Xidorn preferred me to do something similar for SetStyleImageRequest, too.)

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8814828 [details]
Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

https://reviewboard.mozilla.org/r/95930/#review96312

::: layout/style/nsRuleNode.cpp:1163
(Diff revision 1)
> +                      const nscoord& aParentCoord,
> +                      const nscoord& aInitialCoord,

Since nscoord is small enough to just pass around, we can probably just make these types nscoord directly.

::: layout/style/nsRuleNode.cpp:1194
(Diff revision 1)
> +                   "Parser should have rejected negative length!");
> +      len = 0;
> +    }
> +    return Some(len);
> +  } else {
> +    return Maybe<nscoord>(Nothing());

Does |return Nothing();| work here?

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8814828 [details]
Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

https://reviewboard.mozilla.org/r/95930/#review96320

::: layout/style/nsRuleNode.cpp:1194
(Diff revision 1)
> +                   "Parser should have rejected negative length!");
> +      len = 0;
> +    }
> +    return Some(len);
> +  } else {
> +    return Maybe<nscoord>(Nothing());

Can you also assert in here that unit == eCSSUnit_Null?

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8814829 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set column-rule-width.

https://reviewboard.mozilla.org/r/95932/#review96322

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8814830 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set border-width.

https://reviewboard.mozilla.org/r/95934/#review96324

::: layout/style/nsRuleNode.cpp:7517
(Diff revision 1)
>             StyleBoxDecorationBreak::Slice);
>  
>    // border-width, border-*-width: length, enum, inherit
>    nsStyleCoord coord;
>    {
> +    Maybe<nscoord> coord;

Nit: I would probably just to declare on the same line you assign to it, in the NS_FOR_CSS_SIDES loop.

::: layout/style/nsRuleNode.cpp:7537
(Diff revision 1)
>          NS_ASSERTION(eCSSUnit_Null == value.GetUnit(),
>                       "missing case handling border width");

This can disappear then, if the assertion is done up in ComputeLineWidthValue.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8814351 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

https://reviewboard.mozilla.org/r/95626/#review96328

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8814831 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for outline-width.

https://reviewboard.mozilla.org/r/95936/#review96330

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8814828 [details]
Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

https://reviewboard.mozilla.org/r/95930/#review96340

::: layout/style/nsRuleNode.cpp:1163
(Diff revision 1)
> +                      const nscoord& aParentCoord,
> +                      const nscoord& aInitialCoord,

Perhaps `const nscoord`?

Comment 24

[Jeremy Chen ([e-mail])]

Comment on attachment 8814830 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set border-width.

https://reviewboard.mozilla.org/r/95934/#review96324

> Nit: I would probably just to declare on the same line you assign to it, in the NS_FOR_CSS_SIDES loop.

Will do.

> This can disappear then, if the assertion is done up in ComputeLineWidthValue.

Yeah, let's move this assertion into ComputeLineWidthValue.

Comment 25

[Jeremy Chen ([e-mail])]

Comment on attachment 8814828 [details]
Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

https://reviewboard.mozilla.org/r/95930/#review96340

> Perhaps `const nscoord`?

Yes, I'll fix this as you and Cameron suggested. :-)

Comment 26

[Jeremy Chen ([e-mail])]

Comment on attachment 8814829 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set column-rule-width.

https://reviewboard.mozilla.org/r/95932/#review96292

> Personally I'd prefer it checks `coord.isSome()` rather than checking `coord` directly. The latter could be confusing.

Sounds good. Will do.
Thank you for the feedback. :-)

Comment 27

[Jeremy Chen ([e-mail])]

Comment on attachment 8814828 [details]
Bug 1320239 - create ComputeLineWidthValue helper function for line-width computed values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95930/diff/1-2/

Comment 28

[Jeremy Chen ([e-mail])]

Comment on attachment 8814829 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set column-rule-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95932/diff/1-2/

Comment 29

[Jeremy Chen ([e-mail])]

Comment on attachment 8814830 [details]
Bug 1320239 - use ComputeLineWidthValue helper to set border-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95934/diff/1-2/

Comment 30

[Jeremy Chen ([e-mail])]

Comment on attachment 8814351 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for -webkit-text-stroke-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95626/diff/3-4/

Comment 31

[Jeremy Chen ([e-mail])]

Comment on attachment 8814831 [details]
Bug 1320239 - use nscoord instead of nsStyleCoord for outline-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95936/diff/1-2/

Comment 32

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e244393c9c2dc8214517716d5ee8f7a33be9e9ec

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea48e1d56898
create ComputeLineWidthValue helper function for line-width computed values. r=heycam
https://hg.mozilla.org/integration/autoland/rev/11e8fbe5bf46
use ComputeLineWidthValue helper to set column-rule-width. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c4541983d7c4
use ComputeLineWidthValue helper to set border-width. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c0a1b4238bde
use nscoord instead of nsStyleCoord for -webkit-text-stroke-width. r=heycam
https://hg.mozilla.org/integration/autoland/rev/95c8f7a8569e
use nscoord instead of nsStyleCoord for outline-width. r=heycam

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea48e1d56898
https://hg.mozilla.org/mozilla-central/rev/11e8fbe5bf46
https://hg.mozilla.org/mozilla-central/rev/c4541983d7c4
https://hg.mozilla.org/mozilla-central/rev/c0a1b4238bde
https://hg.mozilla.org/mozilla-central/rev/95c8f7a8569e