Closed Bug 1277154 Opened 3 years ago Closed 3 years ago

stop using bitfields in nsStyleSVG

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file, 2 obsolete files)

The bitfields in nsStyleSVG are confusing the rust bindgen tool used for stylo.  Let's change it to a uint and some bit operations.
Attached patch patch (obsolete) — Splinter Review
Attachment #8758545 - Flags: review?(bugzilla)
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?
Attachment #8758545 - Flags: review?(bugzilla) → review+
(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.
Attached patch patch r=xidorn (obsolete) — Splinter Review
Attachment #8758545 - Attachment is obsolete: true
Keywords: checkin-needed
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
Flags: needinfo?(cam)
Keywords: checkin-needed
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb39628ebe69
Remove bitfields from nsStyleSVG to help with rust bindgen. r=xidorn
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 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.
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.
Thanks, yes MozReview's inter-line change highlighting is good.
Flags: needinfo?(cam)
Attachment #8758597 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c8ec387dd2
Remove bitfields from nsStyleSVG to help with rust bindgen. r=xidorn
Keywords: checkin-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
Flags: needinfo?(cam)
That was just because heycam forgot to fold the fix into the patch, and uploaded the problematic patch again :)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d10039dcf721
Remove bitfields from nsStyleSVG to help with rust bindgen. r=xidorn
Thank you philor.
https://hg.mozilla.org/mozilla-central/rev/d10039dcf721
https://hg.mozilla.org/mozilla-central/rev/9adc60621a4a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.