Closed Bug 1297064 Opened 4 years ago Closed 4 years ago

stylo: Add bindings for filling nsStyleImageLayers

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file)

While we can do this directly from servo, we'd have to manually call copy constructors.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
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 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+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69fcb7b0e100
stylo: Add bindings for filling nsStyleImageLayers lists; r=heycam
https://hg.mozilla.org/mozilla-central/rev/69fcb7b0e100
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.