stylo: Add bindings for filling nsStyleImageLayers

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

While we can do this directly from servo, we'd have to manually call copy constructors.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED

Comment 2

2 years ago
mozreview-review
Comment on attachment 8783528 [details]
Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

https://reviewboard.mozilla.org/r/73320/#review71102

Not a formal review at all, just passed by and saw the style nits :P

::: layout/style/ServoBindings.cpp:281
(Diff revision 1)
>    }
>    return attr->GetServoCSSDeclarationValue();
>  }
>  
> +void
> +Gecko_FillAllBackgroundLists(nsStyleImageLayers* aLayers, uint32_t aMaxLen) {

nit: brace in its own line.

::: layout/style/nsRuleNode.cpp:7189
(Diff revision 1)
>      aLayers[destLayer].mPosition.*aResultLocation =
>        aLayers[sourceLayer].mPosition.*aResultLocation;
>    }
>  }
>  
> +void FillAllBackgroundLists(nsStyleImageLayers& aImage,

nit: return type in its own line, brace too.
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8783528 [details]
Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

https://reviewboard.mozilla.org/r/73320/#review72334

::: layout/style/ServoBindings.h:244
(Diff revision 2)
>  void Gecko_CopyClipPathValueFrom(mozilla::StyleClipPath* dst, const mozilla::StyleClipPath* src);
>  
>  void Gecko_DestroyClipPath(mozilla::StyleClipPath* clip);
>  mozilla::StyleBasicShape* Gecko_NewBasicShape(mozilla::StyleBasicShapeType type);
>  
> +void Gecko_FillAllBackgroundLists(nsStyleImageLayers* layers, uint32_t maxLen);

Nit: "max_len" so we generate a Rust friendly argument name?

::: layout/style/nsRuleNode.h:1070
(Diff revision 2)
> +void FillAllBackgroundLists(nsStyleImageLayers& aLayers,
> +                            uint32_t aMaxItemCount);

Please make this a public static method of nsRuleNode.  (We should avoid having global namespace functions.  Putting it in |namespace mozilla| would be OK too, but I prefer sticking it on nsRuleNode so that callers can see where it's coming from / what it's related to.)

::: layout/style/nsRuleNode.cpp:7192
(Diff revision 2)
>  }
>  
> +void
> +FillAllBackgroundLists(nsStyleImageLayers& aImage, uint32_t aMaxItemCount)
> +{
> +    // Delete any extra items.  We need to keep layers in which any

Two space indent.
Attachment #8783528 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69fcb7b0e100
stylo: Add bindings for filling nsStyleImageLayers lists; r=heycam

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69fcb7b0e100
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.