1285235 - stylo: Add bindings for growing nsTArray, nsStyleImageLayers

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manish Goregaokar [:manishearth]]

Counterpart to https://github.com/servo/servo/pull/11851

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 1/2] Add bindings for growing nsTArrays

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 2/2] Add EnsureLengthAtLeast function to nsStyleImageLayers

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8768773 [details] [diff] [review]
[PATCH 1/2] Add bindings for growing nsTArrays

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

r=me with those changes.

::: layout/style/ServoBindings.cpp
@@ +542,4 @@
>    return result;
>  }
>  
> +void Gecko_ArrayEnsureCapacity(void* array, size_t capacity, size_t size) {

Nit: Return value goes on its own line, and we should use gecko naming for the arguments in the cpp file (aArray, aCapacity, aElemSize).

The only reason we don't use gecko naming in the header is that it makes weird bindings on the bindgen side. I'd like to find some more elegant solution to this.

@@ +542,5 @@
>    return result;
>  }
>  
> +void Gecko_ArrayEnsureCapacity(void* array, size_t capacity, size_t size) {
> +  auto *base = (nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils> *)array;

Please don't use auto* - if the semantics aren't obvious, we should just redeclare the type.

Also please use a c++-style reinterpret_cast here.

::: layout/style/ServoBindings.h
@@ +176,5 @@
>                           ThreadSafePrincipalHolder* principal);
>  void Gecko_CopyMozBindingFrom(nsStyleDisplay* des, const nsStyleDisplay* src);
>  
> +// `array` must be an nsTArray
> +void Gecko_ArrayEnsureCapacity(void* array, size_t capacity, size_t size);

Let's call this Gecko_EnsureTArrayCapacity, and label the second argument elemsize.

::: xpcom/glue/nsTArray.h
@@ +381,4 @@
>    typename ActualAlloc::ResultTypeProxy EnsureCapacity(size_type aCapacity,
>                                                         size_type aElemSize);
>  
> +protected:

This stuff is used all over the tree, so I think we should be careful about exposing new methods to callsites everywhere. How about declaring our FFI function as a friend instead? That seems more appropriate given what we're doing.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8768774 [details] [diff] [review]
[PATCH 2/2] Add EnsureLengthAtLeast function to nsStyleImageLayers

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

::: layout/style/ServoBindings.h
@@ +180,5 @@
>  
>  // `array` must be an nsTArray
>  void Gecko_ArrayEnsureCapacity(void* array, size_t capacity, size_t size);
> +
> +void Gecko_ImageLayers_EnsureLength(nsStyleImageLayers* layers, size_t len);

I'd like to avoid the double-underscore naming (there are a couple of them in the file which I'd like to get rid of eventually). How about Gecko_EnsureImageLayersLength?

::: layout/style/nsStyleStruct.cpp
@@ +2372,4 @@
>    return false;
>  }
>  
> +void nsStyleImageLayers::EnsureLengthAtLeast(size_t len) {

Nit: aLen or aLength, consistent with the header.

@@ +2372,5 @@
>    return false;
>  }
>  
> +void nsStyleImageLayers::EnsureLengthAtLeast(size_t len) {
> +  auto old_length = mLayers.Length();

no snake case in gecko - oldLen?

@@ +2375,5 @@
> +void nsStyleImageLayers::EnsureLengthAtLeast(size_t len) {
> +  auto old_length = mLayers.Length();
> +  mLayers.EnsureLengthAtLeast(len);
> +  for (auto i = old_length; i < len; i++) {
> +    mLayers[i].Initialize(nsStyleImageLayers::LayerType::Background);

hm, it seems wrong to always initialize these as background layers, no? Might it make more sense to have an API that allows us to push layers of a given type (over FFI), and then manipulate the result from Rust?

::: layout/style/nsStyleStruct.h
@@ +747,4 @@
>  
>    bool HasLayerWithImage() const;
>  
> +  void EnsureLengthAtLeast(size_t length);

Nit: aLength.

Comment 5

[Manish Goregaokar [:manishearth]]

> hm, it seems wrong to always initialize these as background layers, no?
> Might it make more sense to have an API that allows us to push layers of a
> given type (over FFI), and then manipulate the result from Rust?
> 


So I tried adding a nsStyleImageLayers::LayerType fillType parameter to EnsureLengthAtLeast, so that it will initialize to a given type.

However, C++ doesn't let you forward-declare nested classes; so I'd have to include nsStyleStruct.h in ServoBindings.h to do this -- given that everything so far in that file has avoided including and stuck to forward-declaring, I'm not sure if that's the way to go forward with this.

For now, I'm just going to use an int parameter.

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 1/2] Add bindings for growing nsTArrays. v2

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 2/2] Add EnsureLengthAtLeast function to nsStyleImageLayers. v2

Updated patches

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8769077 [details] [diff] [review]
[PATCH 2/2] Add EnsureLengthAtLeast function to nsStyleImageLayers. v2

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

::: layout/style/ServoBindings.cpp
@@ +550,5 @@
>  }
>  
> +void Gecko_EnsureImageLayersLength(nsStyleImageLayers* aLayers, size_t aLen,
> +                                   uint8_t aFillType) {
> +  aLayers->EnsureLengthAtLeast(aLen, static_cast<nsStyleImageLayers::LayerType>(aFillType));

I don't really like this solution, but I'm not sure if there's anything better. An alternate solution is to move LayerType out into _LayerType or something, and typedef it within nsStyleImageLayers.

We could also just include nsStyleStruct.h in this file but like I said I assume there's a reason we're forward-declaring.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8769076 [details] [diff] [review]
[PATCH 1/2] Add bindings for growing nsTArrays. v2

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

::: layout/style/ServoBindings.cpp
@@ +543,5 @@
>  }
>  
> +void
> +Gecko_EnsureTArrayCapacity(void* aArray, size_t aCapacity, size_t aElemSize) {
> +  nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils> *base

Question: is just |auto| really not sufficient here? I was objecting to the use of auto*, whose semantics are not obvious. Given that the reintepret_cast explicitly states the type that the expression returns, it seems like |auto| would be the best thing to use here.

If that's not possible, please move this star to the left.

::: layout/style/ServoBindings.h
@@ +177,5 @@
>                           ThreadSafePrincipalHolder* principal);
>  void Gecko_CopyMozBindingFrom(nsStyleDisplay* des, const nsStyleDisplay* src);
>  
> +// `array` must be an nsTArray
> +void Gecko_EnsureTArrayCapacity(void* array, size_t capacity, size_t elem_size);

Make a note that anyone changing this signature should also change the signatures in nsTArray.h.

@@ +179,5 @@
>  
> +// `array` must be an nsTArray
> +void Gecko_EnsureTArrayCapacity(void* array, size_t capacity, size_t elem_size);
> +
> +void Gecko_ImageLayers_EnsureLength(nsStyleImageLayers* layers, size_t len);

This doesn't belong in this patch.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8769077 [details] [diff] [review]
[PATCH 2/2] Add EnsureLengthAtLeast function to nsStyleImageLayers. v2

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

It seems kinda wrong to use an "Ensure" API here, because we'd get different depending on the order of the following two calls:

Ensure(2, fillTypeFoo);
Ensure(2, fillTypeBar);

It seems to me like we should either (a) have the EnsureLengthAtLeast call leave the the new entries uninitialized, the way gecko does, or (b) use an explicit Append API that guarantees that N new elements will be appended.

In terms of the include issue - my preference would be to just move the definition into ServoBindings.h and include that from nsStyleStruct.h. In general including ServoBindings.h there should be a no-op because it's all functions and we compile with -ignore-functions. But we may not need to do that depending on what we decide above.

::: layout/style/nsStyleStruct.cpp
@@ +2372,5 @@
>    return false;
>  }
>  
> +void
> +nsStyleImageLayers::EnsureLengthAtLeast(size_t aLength, nsStyleImageLayers::LayerType fillType) {

Nit: a-prefix the second arg.

::: layout/style/nsStyleStruct.h
@@ +747,4 @@
>  
>    bool HasLayerWithImage() const;
>  
> +  void EnsureLengthAtLeast(size_t aLength, nsStyleImageLayers::LayerType fillType);

Nit, a-prefix this second arg. And does it actually need to be fully-qualified?

Comment 11

[Manish Goregaokar [:manishearth]]

> In terms of the include issue - my preference would be to just move the definition into ServoBindings.h and include that from nsStyleStruct.h.

Move what definition? EnsureLengthAtLeast? The problem is that we can't accept a parameter of LayerType in a type-safe manner, we can only accept an integer and cast it (like I'm doing right now). If we wish to accept a parameter of LayerType, we have to include nsStyleStruct.h itself in the bindings file, or at least the entire definition of nsStyleImageLayers. I'm not very fond of moving core Gecko style system definitions into the bindings file, though if you think that's okay I'll be happy to do it :)

> It seems to me like we should either (a) have the EnsureLengthAtLeast call leave the the new entries uninitialized, the way gecko does, or (b) use an explicit Append API that guarantees that N new elements will be appended.

When you leave them uninitialized it doesn't work anymore -- so we'd still have to initialize from Rust. Append wfm. 


> Question: is just |auto| really not sufficient here?

It should be, I just wasn't aware of the style guidelines and thought that (like many other codebases), Gecko didn't like auto :)

I'll use it.

> And does it actually need to be fully-qualified?

Nope, my bad

Comment 12

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 1/3] Add bindings for growing nsTArrays. v3

Comment 13

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 2/3] Add EnsureLengthAtLeast function to nsStyleImageLayers for Stylo. v3

Comment 14

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 3/3] Forward-declare nsStyleImageLayers::LayerType in stylo bindings

The other two patches address all the comments.

This one is an optional patch which makes the forward-declaration work (which lets us be a bit more typesafe). It involves doing an extra typedef inside ServoBindings.cpp, which I'm not sure is acceptable style. Everything works without this patch so we can just exclude it.

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8769618 [details] [diff] [review]
[PATCH 3/3] Forward-declare nsStyleImageLayers::LayerType in stylo  bindings

Turns out this doesn't work, never mind

Comment 16

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> 
> > In terms of the include issue - my preference would be to just move the definition into ServoBindings.h and include that from nsStyleStruct.h.
> 
> Move what definition? EnsureLengthAtLeast? The problem is that we can't
> accept a parameter of LayerType in a type-safe manner, we can only accept an
> integer and cast it (like I'm doing right now). If we wish to accept a
> parameter of LayerType, we have to include nsStyleStruct.h itself in the
> bindings file, or at least the entire definition of nsStyleImageLayers. I'm
> not very fond of moving core Gecko style system definitions into the
> bindings file, though if you think that's okay I'll be happy to do it :)

I was suggesting moving the LayerType enum into ServoBindings.h, and then #including ServoBindings.h in nsStyleStruct.h, which would let us use the enum on both contexts. It would have to not be an inner type but that's fine.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8769613 [details] [diff] [review]
[PATCH 1/3] Add bindings for growing nsTArrays. v3

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

Looks great, thanks.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8769615 [details] [diff] [review]
[PATCH 2/3] Add EnsureLengthAtLeast function to nsStyleImageLayers  for Stylo. v3

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

Please do the suggestion from comment 16 unless there's some good reason that doesn't work, in which case this is ok.

::: layout/style/ServoBindings.cpp
@@ +552,5 @@
> +  aLayers->mLayers.EnsureLengthAtLeast(aLen);
> +}
> +
> +void Gecko_InitializeImageLayer(nsStyleImageLayers_Layer* aLayer, uint8_t aLayerType) {
> +  auto layer = reinterpret_cast<nsStyleImageLayers::Layer *> (aLayer);

Nit - no space between Layer and *, and no space between < and (.

Comment 19

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 2/3] Add EnsureLengthAtLeast function to nsStyleImageLayers for Stylo. v4

Comment 20

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 3/3] Move LayerType into ServoBindings.h

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8770041 [details] [diff] [review]
[PATCH 3/3] Move LayerType into ServoBindings.h

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

::: layout/style/ServoBindings.h
@@ +52,4 @@
>  struct nsStyleDisplay;
>  struct ServoDeclarationBlock;
>  
> +enum class nsStyleImageLayers_LayerType : uint8_t {

Sorry, I don't think I can sign-off this.

The correct way to do this is importing the type from the rust structs module to the bindings module, blacklisting it (see how we do the same for things like `SheetParsingMode`, blacklisting LayerType might work here, though you might need to modify the script to take the namespacing into account). If for some reason this is impossible, we should fix bindgen.

You *should* be able to include nsStyleStructs.h here if needed, and it shouldn't have any impact on the generated bindings.

Feel free to flag heycam for review though, I'm happy to concede if he thinks the opposite, but I think this should be fixed by tooling, not by hoisting code around.

::: layout/style/nsStyleStruct.h
@@ +490,4 @@
>      composite
>    };
>  
> +  typedef nsStyleImageLayers_LayerType LayerType;

Same here, I think the enum definition should live here, or at least in this file.

Comment 22

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 3/3] Move LayerType into ServoBindings.h. v2.

It worked. Needed to patch up tooling a bit on the servo side.

Comment 23

[Bobby Holley (:bholley)]

Yes, fixing this with the tooling is definitely preferable, I just couldn't think of a way to do it. Nice idea Emilio!

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8770040 [details] [diff] [review]
[PATCH 2/3] Add EnsureLengthAtLeast function to nsStyleImageLayers  for Stylo. v4

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

r=me with that.

::: layout/style/ServoBindings.cpp
@@ +552,5 @@
> +  aLayers->mLayers.EnsureLengthAtLeast(aLen);
> +}
> +
> +void Gecko_InitializeImageLayer(nsStyleImageLayers_Layer* aLayer, uint8_t aLayerType) {
> +  auto layer = reinterpret_cast<nsStyleImageLayers::Layer*>(aLayer);

So we can take rid of this reinterpret_cast too.

::: layout/style/ServoBindings.h
@@ +44,5 @@
>  class nsHTMLCSSStyleSheet;
>  struct nsStyleList;
>  struct nsStyleImage;
> +struct nsStyleImageLayers;
> +struct nsStyleImageLayers_Layer;

Probably we can just use the same mechanism to use nsStyleImageLayers::Layer instead?

Also, we should be able to take rid of a few forward declarations now I think. Bindgen should generate them automatically and are no longer needed to make the bindings file compile.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8770074 [details] [diff] [review]
[PATCH 3/3] Move LayerType into ServoBindings.h. v2.

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

Looks way better, r=me :)

Comment 26

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 3/3] Use fully qualified inner types instead of casting. v3

Done. Renamed the third patch and included the suggested change in it instead of the second one.

Comment 27

[Manish Goregaokar [:manishearth]]

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

Comment 28

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 1/3] Bug 1285235 - Add bindings for growing nsTArrays; r=bholley

Fixing up the commit messages

Comment 29

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 2/3] Bug 1285235 - Add EnsureLengthAtLeast function to nsStyleImageLayers for Stylo; r=bholley

Comment 30

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 3/3] Bug 1285235 - Use fully qualified inner types instead of casting; r=emilio

Comment 31

[Manish Goregaokar [:manishearth]]

The try build passes, and the tests so far are passing. The so-far-incomplete tests shouldn't fail, since I haven't touched any code that Firefox actually runs in non-stylo mode, they were included as a sanity check.

Comment 32

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 1/3] Bug 1285235 - Add bindings for growing nsTArrays; r=bholley

Rebasing patches

Comment 33

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 2/3] Bug 1285235 - Add EnsureLengthAtLeast function to nsStyleImageLayers for Stylo; r=bholley

Comment 34

[Manish Goregaokar [:manishearth]]

Attachment: [PATCH 3/3] Bug 1285235 - Use fully qualified inner types instead of casting; r=emilio

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad85150b5ad2
Add bindings for growing nsTArrays; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1584e420e6e
Add EnsureLengthAtLeast function to nsStyleImageLayers for Stylo; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb82d8367ebe
Use fully qualified inner types instead of casting; r=emilio

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad85150b5ad2
https://hg.mozilla.org/mozilla-central/rev/f1584e420e6e
https://hg.mozilla.org/mozilla-central/rev/bb82d8367ebe