1277154 - stop using bitfields in nsStyleSVG

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

The bitfields in nsStyleSVG are confusing the rust bindgen tool used for stylo.  Let's change it to a uint and some bit operations.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 2

[Cameron McCormack (:heycam)]

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8758545 [details] [diff] [review]
patch

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

I'm not a fan of replacing bitfields with bit operations... But if it makes things easier for stylo to handle, that's probably fine.

::: layout/style/nsStyleStruct.h
@@ +3283,5 @@
> +    STROKE_WIDTH_CONTEXT       = 0x40,  // stroke-width: context-value
> +    FILL_OPACITY_SOURCE_SHIFT   = 0,
> +    STROKE_OPACITY_SOURCE_SHIFT = 2,
> +  };
> +  uint8_t          mContextFlags;     // [inherited]

Could we make this field as well as the enum above private?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> I'm not a fan of replacing bitfields with bit operations... But if it makes
> things easier for stylo to handle, that's probably fine.

Yeah, unfortunately bitfield layout and alignment is not portable. :(

> ::: layout/style/nsStyleStruct.h
> > +  uint8_t          mContextFlags;     // [inherited]
> 
> Could we make this field as well as the enum above private?

Yes, good idea.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch r=xidorn

Comment 6

[Carsten Book [:Tomcat]]

has problems to apply :

renamed 1277154 -> a.patch
applying a.patch
patching file layout/style/nsStyleStruct.cpp
Hunk #2 FAILED at 868
1 out of 3 hunks FAILED -- saving rejects to file layout/style/nsStyleStruct.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh a.patch

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb39628ebe69
Remove bitfields from nsStyleSVG to help with rust bindgen. r=xidorn

Comment 8

[Phil Ringnalda (:philor)]

Looks like you'll need to have a talk with svg-glyph-objectvalue.svg before you can do that, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=29374382&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f565e74077ba

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8758597 [details] [diff] [review]
patch r=xidorn

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

::: layout/style/nsRuleNode.cpp
@@ +9388,5 @@
>  
>    case eCSSUnit_Inherit:
>    case eCSSUnit_Unset:
>      conditions.SetUncacheable();
> +    svg->SetStrokeDashoffsetFromObject(parentSVG->StrokeDashoffsetFromObject());

Looks like this is where the issue is... It should be Dasharray rather than Dashoffset.

Sorry that I didn't catch this...

@@ +9396,5 @@
>    case eCSSUnit_Enumerated:
>      MOZ_ASSERT(strokeDasharrayValue->GetIntValue() ==
>                       NS_STYLE_STROKE_PROP_CONTEXT_VALUE,
>                 "Unknown keyword for stroke-dasharray");
> +    svg->SetStrokeDashoffsetFromObject(true);

Same here.

@@ +9402,5 @@
>      break;
>  
>    case eCSSUnit_Initial:
>    case eCSSUnit_None:
> +    svg->SetStrokeDashoffsetFromObject(false);

Same here.

@@ +9408,5 @@
>      break;
>  
>    case eCSSUnit_List:
>    case eCSSUnit_ListDep: {
> +    svg->SetStrokeDashoffsetFromObject(false);

Same here.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

I'd suggest using MozReview next time. I believe this kind of mistakes are easier to catch with MozReview, since it has inline change highlight on top of line changes.

Comment 11

[Cameron McCormack (:heycam)]

Thanks, yes MozReview's inter-line change highlighting is good.

Comment 12

[Cameron McCormack (:heycam)]

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

Comment 13

[Cameron McCormack (:heycam)]

Attachment: patch v1.1 r=xidorn

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c8ec387dd2
Remove bitfields from nsStyleSVG to help with rust bindgen. r=xidorn

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I think this patch is what caused a bunch of reftest bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=29447290&repo=mozilla-inbound

I see it being green in the try run, but maybe there's something newer that caused this to make the conflict?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5f1c93108c

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

That was just because heycam forgot to fold the fix into the patch, and uploaded the problematic patch again :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d10039dcf721
Remove bitfields from nsStyleSVG to help with rust bindgen. r=xidorn

Comment 18

[Cameron McCormack (:heycam)]

Third time lucky. :(

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9adc60621a4a
followup, actually fix the bustage

Comment 20

[Cameron McCormack (:heycam)]

Thank you philor.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d10039dcf721
https://hg.mozilla.org/mozilla-central/rev/9adc60621a4a