Closed Bug 1285235 Opened 3 years ago Closed 3 years ago

stylo: Add bindings for growing nsTArray, nsStyleImageLayers

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(3 files, 14 obsolete files)

2.79 KB, patch
manishearth
: review+
Details | Diff | Splinter Review
2.35 KB, text/plain
manishearth
: review+
Details
2.34 KB, patch
manishearth
: review+
Details | Diff | Splinter Review
Assignee: nobody → manishearth
Attachment #8768773 - Flags: review?(bobbyholley)
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.
Attachment #8768773 - Flags: review?(bobbyholley) → review+
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.
Attachment #8768774 - Flags: review?(bobbyholley) → review-
> 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.
Attachment #8768773 - Attachment is obsolete: true
Attachment #8769076 - Flags: review+
Updated patches
Attachment #8768774 - Attachment is obsolete: true
Attachment #8769077 - Flags: review?(bobbyholley)
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 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 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?
Attachment #8769077 - Flags: review?(bobbyholley) → review-

> 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
Attachment #8769076 - Attachment is obsolete: true
Attachment #8769613 - Flags: review?(bobbyholley)
Attachment #8769077 - Attachment is obsolete: true
Attachment #8769615 - Flags: review?(bobbyholley)
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.
Attachment #8769618 - Flags: feedback?(bobbyholley)
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
Attachment #8769618 - Attachment is obsolete: true
Attachment #8769618 - Flags: feedback?(bobbyholley)
(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 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.
Attachment #8769613 - Flags: review?(bobbyholley) → review+
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 (.
Attachment #8769615 - Flags: review?(bobbyholley) → review+
Attachment #8770041 - Flags: review?(ealvarez)
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.
Attachment #8770041 - Flags: review?(ealvarez) → review-
It worked. Needed to patch up tooling a bit on the servo side.
Attachment #8770041 - Attachment is obsolete: true
Attachment #8770074 - Flags: review?(ealvarez)
Yes, fixing this with the tooling is definitely preferable, I just couldn't think of a way to do it. Nice idea 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.
Attachment #8770040 - Flags: review?(ealvarez) → review+
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 :)
Attachment #8770074 - Flags: review?(ealvarez) → review+
Done. Renamed the third patch and included the suggested change in it instead of the second one.
Attachment #8770074 - Attachment is obsolete: true
Attachment #8770426 - Flags: review+
Fixing up the commit messages
Attachment #8769613 - Attachment is obsolete: true
Attachment #8770473 - Flags: review+
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.
Keywords: checkin-needed
Rebasing patches
Attachment #8770473 - Attachment is obsolete: true
Attachment #8770866 - Flags: review+
Pushed by cmccormack@mozilla.com:
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
Keywords: checkin-needed
Blocks: stylo
You need to log in before you can comment on or make changes to this bug.