Closed Bug 1276085 Opened 8 years ago Closed 8 years ago

stylo: Allow setting linear-gradient as background-image

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: emilio, Unassigned)

References

Details

Attachments

(1 file)

Bug 1276085: followup: Remove Gecko_SetGradientStop now we have nsTArray bindings
58 bytes, text/x-review-board-request
heycam
: review+
Details 
I have a patch for this.
Attachment #8757072 - Flags: review?(cam)
Attachment #8757072 - Flags: review?(cam)
Comment on attachment 8757072 [details]
Bug 1276085: followup: Remove Gecko_SetGradientStop now we have nsTArray bindings

https://reviewboard.mozilla.org/r/55590/#review52764

Sorry I didn't get to this on Friday.

::: layout/style/ServoBindings.h:109
(Diff revision 1)
> +struct nsStyleImage;
> +struct nsStyleGradientStop;
> +class nsStyleGradient;
> +class nsStyleCoord;

I think we should move these up to the top of the file.  Can you do the same for the other couple of forward declarations that are scattered through the file?

::: layout/style/ServoBindings.cpp:283
(Diff revision 1)
>    aDest->mBinding = aSrc->mBinding;
>  }
>  
> +
> +void
> +Gecko_SetNullImageValue(nsStyleImage* aImage) {

Nit: brace on new line (and in the following functions too).

::: layout/style/ServoBindings.cpp:310
(Diff revision 1)
> +  if (!result)
> +    return nullptr;

No need to check for OOM, |new| is infallible by default.

::: layout/style/ServoBindings.cpp:313
(Diff revision 1)
> +  result->mShape = aShape;
> +  result->mSize = aSize;
> +  result->mRepeating = aRepeating;
> +  result->mLegacySyntax = aLegacySyntax;
> +
> +  result->mAngle.SetNoneValue();
> +  result->mBgPosX.SetNoneValue();
> +  result->mBgPosY.SetNoneValue();
> +  result->mRadiusX.SetNoneValue();
> +  result->mRadiusY.SetNoneValue();

I think we should avoid having to call into an FFI function per stop.  Since the bindings can give us access to nsTArray members, how about we have Gecko_CreateGradient just provide us with a fully initialized nsStyleGradient except for the mStops array, which we set to the right size with SetLength() otherwise leave uninitialized.  (That should still cause the nsStyleCoord constructor to be called for each nsStyleGradientStop::mLocation.) Then we can poke at its array elements directly from the rust side.

::: layout/style/ServoBindings.cpp:337
(Diff revision 1)
> +
> +  return result;
> +}
> +
> +void
> +Gecko_SetGradientStop(nsStyleGradient* aGradient,

Then we can remove this function.

::: layout/style/nsStyleStruct.h:231
(Diff revision 1)
>  
>    bool IsOpaque();
>    bool HasCalc();
>    uint32_t Hash(PLDHashNumber aHash);
>  
> -  NS_INLINE_DECL_REFCOUNTING(nsStyleGradient)
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsStyleGradient);

Nit: it's preferable to leave off the semicolon, since this expands to a number of declarations.
(In reply to Cameron McCormack (:heycam) from comment #2)
> Sorry I didn't get to this on Friday.

No problem :)

btw, let me know if you prefer me to upload raw patches, I was just testing mozreview.

> ::: layout/style/ServoBindings.h:109
> (Diff revision 1)
> > +struct nsStyleImage;
> > +struct nsStyleGradientStop;
> > +class nsStyleGradient;
> > +class nsStyleCoord;
> 
> I think we should move these up to the top of the file.  Can you do the same
> for the other couple of forward declarations that are scattered through the
> file?

Sure, sounds good.

> > +void
> > +Gecko_SetNullImageValue(nsStyleImage* aImage) {
> 
> Nit: brace on new line (and in the following functions too).

Got it, thanks!

> ::: layout/style/ServoBindings.cpp:310
> (Diff revision 1)
> > +  if (!result)
> > +    return nullptr;
> 
> No need to check for OOM, |new| is infallible by default.

Hmmm... We do check for null in the other part of the code where we create an nsStyleGradient (http://searchfox.org/mozilla-central/source/layout/style/nsRuleNode.cpp#1247), so I guess I should remove that one too?

> ::: layout/style/ServoBindings.cpp:313
> (Diff revision 1)
> > +  result->mShape = aShape;
> > +  result->mSize = aSize;
> > +  result->mRepeating = aRepeating;
> > +  result->mLegacySyntax = aLegacySyntax;
> > +
> > +  result->mAngle.SetNoneValue();
> > +  result->mBgPosX.SetNoneValue();
> > +  result->mBgPosY.SetNoneValue();
> > +  result->mRadiusX.SetNoneValue();
> > +  result->mRadiusY.SetNoneValue();
> 
> I think we should avoid having to call into an FFI function per stop.  Since
> the bindings can give us access to nsTArray members, how about we have
> Gecko_CreateGradient just provide us with a fully initialized
> nsStyleGradient except for the mStops array, which we set to the right size
> with SetLength() otherwise leave uninitialized.  (That should still cause
> the nsStyleCoord constructor to be called for each
> nsStyleGradientStop::mLocation.) Then we can poke at its array elements
> directly from the rust side.

Agreed, I'd have avoided this if I could. The problem is we still can't have access to nsTArray elements from Rust (we just can access to the first element of nsStyleAutoTArray), which is quite lame, but not an easy thing to fix, since nsTArray uses some C++ template features that can't be translated to rust easily.

If this thing could be a blocker for performance (which can very well be) we should probably try to write an equivalent type in Rust. I could try to hack on the libclang ast to make bindgen work for this case, but I remember this was not an easy thing precisely. I think that corresponds to another patch though (that btw I'll happily do, but when I'm done with exams :P).

> >  
> > -  NS_INLINE_DECL_REFCOUNTING(nsStyleGradient)
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsStyleGradient);
> 
> Nit: it's preferable to leave off the semicolon, since this expands to a
> number of declarations.

Ack'd.

I'm going to sleep now, I can have the patch with the changes (except the nsTArray thing) tomorrow :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> btw, let me know if you prefer me to upload raw patches, I was just testing
> mozreview.

mozreview's fine.

> > No need to check for OOM, |new| is infallible by default.
> 
> Hmmm... We do check for null in the other part of the code where we create
> an nsStyleGradient
> (http://searchfox.org/mozilla-central/source/layout/style/nsRuleNode.
> cpp#1247), so I guess I should remove that one too?

Yeah, that's code from before Gecko switched to infallible new (which was 3 or 4 years ago).  It can be removed.

> > I think we should avoid having to call into an FFI function per stop.
> 
> Agreed, I'd have avoided this if I could. The problem is we still can't have
> access to nsTArray elements from Rust (we just can access to the first
> element of nsStyleAutoTArray), which is quite lame, but not an easy thing to
> fix, since nsTArray uses some C++ template features that can't be translated
> to rust easily.

As I understand it, the layout of nsTArray is a pointer to some memory that contains an nsTArrayHeader directly followed by the actual array elements.  Currently we have:

pub struct nsTArray<T> {
    pub mBuffer: *mut T,
}

Can't we take that mBuffer and do some pointer arithmetic on it to get to the start of the array elements?  Is it that the bindgen will choke on processing nsTArray.h, so we wouldn't be able to write ::std::mem::size_of::<nsTArrayHeader>()?  Could do something like this in ServoBindings.h:

template<typename T>
class nsTArrayBuffer_Simple
{
  uint64_t mOpaqueHeader;
  T mElements;
};

template<typename T>
class nsTArray_Simple
{
  nsTArrayBuffer_Simple<T>* mBuffer;
public:
  ~nsTArray_Simple() {}
};

and then in ServoBindings.cpp static_assert that mOpaqueHeader has the right size?

> If this thing could be a blocker for performance (which can very well be) we
> should probably try to write an equivalent type in Rust. I could try to hack
> on the libclang ast to make bindgen work for this case, but I remember this
> was not an easy thing precisely. I think that corresponds to another patch
> though (that btw I'll happily do, but when I'm done with exams :P).

It's fine to leave nsTArray handling in general to later, but I wonder if the above will work.  It's probably not a blocker for performance, since we won't have too many calls to this, but if we can get it to work easily I think we should.
(In reply to Cameron McCormack (:heycam) from comment #4)
> Can't we take that mBuffer and do some pointer arithmetic on it to get to
> the start of the array elements?  Is it that the bindgen will choke on
> processing nsTArray.h, so we wouldn't be able to write
> ::std::mem::size_of::<nsTArrayHeader>()?  Could do something like this in
> ServoBindings.h:
> 
> template<typename T>
> class nsTArrayBuffer_Simple
> {
>   uint64_t mOpaqueHeader;
>   T mElements;
> };
> 
> template<typename T>
> class nsTArray_Simple
> {
>   nsTArrayBuffer_Simple<T>* mBuffer;
> public:
>   ~nsTArray_Simple() {}
> };
> 
> and then in ServoBindings.cpp static_assert that mOpaqueHeader has the right
> size?

Yes, thas was the kind of thing I was talking about when I said "writing an equivalent type in Rust", sorry for expressing myself badly (it's late night here :P).

I have to think about how we can do it to ideally have access to the length to prevent bad memory accesses from Rust, but yeah, I think it's doable, and not too difficult.

I'm *really* constrained with exams and schoolwork these two weeks though so... Would you be fine landing this with the nits addressed and a TODO comment about adding general nsTArray access and removing Gecko_SetGradientStop, so we don't forget about it and I can get to it when I'm done with them?

> It's fine to leave nsTArray handling in general to later, but I wonder if
> the above will work.  It's probably not a blocker for performance, since we
> won't have too many calls to this, but if we can get it to work easily I
> think we should.

Sure, I meant nsTArray access in general, not this in particular, which I'd expect to be a cold path.
Comment on attachment 8757072 [details]
Bug 1276085: followup: Remove Gecko_SetGradientStop now we have nsTArray bindings

https://reviewboard.mozilla.org/r/55590/#review52796

Sure, let's get this in first.  r=me with the non-nsTArray comments addressed.
Attachment #8757072 - Flags: review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b8a20047e9
stylo: Support creating and setting gradient stops from Servo r=heycam
Landed this, will make the nsTArray changes as a followup today :)
https://hg.mozilla.org/mozilla-central/rev/e1b8a20047e9
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8757072 [details]
Bug 1276085: followup: Remove Gecko_SetGradientStop now we have nsTArray bindings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55590/diff/1-2/
Attachment #8757072 - Attachment description: MozReview Request: Bug 1276085: stylo: Support creating and setting gradient stops from Servo → Bug 1276085: followup: Remove Gecko_SetGradientStop now we have nsTArray bindings
r=me on that followup to remove that function.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0560b158bb22
followup: Remove Gecko_SetGradientStop now we have nsTArray bindings r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: