1297064 - stylo: Add bindings for filling nsStyleImageLayers

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manish Goregaokar [:manishearth]]

While we can do this directly from servo, we'd have to manually call copy constructors.

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8783528 [details]
Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

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

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8783528 [details]
Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

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

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8783528 [details]
Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

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

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8783528 [details]
Bug 1297064 - stylo: Add bindings for filling nsStyleImageLayers lists;

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69fcb7b0e100
stylo: Add bindings for filling nsStyleImageLayers lists; r=heycam

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/69fcb7b0e100