Merge ServoDeclarationBlock and DeclarationBlock

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: emilio, Assigned: canaltinova)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

No description provided.
Flags: needinfo?(emilio)
Priority: -- → P3
Nazim wanted to take this while waiting for answers in other bugs. Thanks Nazim!
Flags: needinfo?(emilio) → needinfo?(canaltinova)
Thanks Emilio!
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
Flags: needinfo?(canaltinova)
Comment on attachment 8982000 [details]
Bug 1464496 - Part 1: Merge ServoDeclarationBlock and DeclarationBlock

https://reviewboard.mozilla.org/r/248044/#review254150

Looks great, thank you!

r=me with these, feel free to land or re-request review at your will :)

Thanks again!

::: layout/style/DeclarationBlock.h:36
(Diff revision 1)
> -protected:
> -  DeclarationBlock()
> +public:
> +  explicit DeclarationBlock(
> +    already_AddRefed<RawServoDeclarationBlock> aRaw)
>      : mImmutable(false)
>      , mIsDirty(false)
> +    , mRaw(aRaw)

nit: moving mRaw before the two booleans allows better packing, maybe do that either here or as a followup commit?

::: layout/style/DeclarationBlock.h:97
(Diff revision 1)
>    void UnsetDirty() { mIsDirty = false; }
>  
>    /**
>     * Copy |this|, if necessary to ensure that it can be modified.
>     */
> -  inline already_AddRefed<DeclarationBlock> EnsureMutable();
> +  already_AddRefed<DeclarationBlock> EnsureMutable();

nit: There's no reason this can't remain being inline, right?

::: layout/style/DeclarationBlock.h:135
(Diff revision 1)
> -  inline void ToString(nsAString& aString) const;
> +  static already_AddRefed<DeclarationBlock>
> +  FromCssText(const nsAString& aCssText, URLExtraData* aExtraData,
> +              nsCompatibility aMode, css::Loader* aLoader);
> +
> +  RawServoDeclarationBlock* Raw() const { return mRaw; }
> +  RawServoDeclarationBlock* const* RefRaw() const {

nit: Let's move the brace to the following line in the inline functions, to be consistent.

::: layout/style/DeclarationBlock.h:177
(Diff revision 1)
> -  inline bool RemoveProperty(const nsAString& aProperty);
> +  bool RemoveProperty(const nsAString& aProperty);
>    // Returns whether the property was removed.
> -  inline bool RemovePropertyByID(nsCSSPropertyID aProperty);
> +  bool RemovePropertyByID(nsCSSPropertyID aProperty);
>  
>  private:
> +  ~DeclarationBlock() {}

nit: You can use `= default;`

::: layout/style/DeclarationBlock.cpp:40
(Diff revision 1)
> +                              URLExtraData* aExtraData,
> +                              nsCompatibility aMode,
> +                              css::Loader* aLoader)
> +{
> +  NS_ConvertUTF16toUTF8 value(aCssText);
> +  // FIXME (bug 1343964): Figure out a better solution for sending the base uri to servo

nit: While you're here, you may as well remove this given that bug is fixed.

::: layout/style/DeclarationBlock.cpp:59
(Diff revision 1)
> +
> +void
> +DeclarationBlock::GetPropertyValueByID(nsCSSPropertyID aPropID,
> +                                       nsAString& aValue) const
> +{
> +  Servo_DeclarationBlock_GetPropertyValueById(mRaw, aPropID, &aValue);

nit: Given you're including ServoBindings.h in the header, you may as well inline these and the other functions in there.

::: servo/components/style/gecko/wrapper.rs:1226
(Diff revision 1)
>  
>      fn smil_override(&self) -> Option<ArcBorrow<Locked<PropertyDeclarationBlock>>> {
>          unsafe {
>              let slots = self.extended_slots()?;
>  
> -            let base_declaration: &structs::DeclarationBlock =
> +            let declaration: &structs::DeclarationBlock =

lovely :)
Attachment #8982000 - Flags: review?(emilio) → review+
Comment on attachment 8982161 [details]
Bug 1464496 - Part 2: Move the mRaw before the two booleans for better packing

https://reviewboard.mozilla.org/r/248174/#review254372

You could've just landed it with rs=emilio fwiw.

Thanks!
Attachment #8982161 - Flags: review?(emilio) → review+
Thanks for the review Emilio!
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d6e582ce9bab
Part 1: Merge ServoDeclarationBlock and DeclarationBlock r=emilio
https://hg.mozilla.org/integration/autoland/rev/edc3670ec282
Part 2: Move the mRaw before the two booleans for better packing r=emilio
https://hg.mozilla.org/mozilla-central/rev/d6e582ce9bab
https://hg.mozilla.org/mozilla-central/rev/edc3670ec282
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.