1464496 - Merge ServoDeclarationBlock and DeclarationBlock

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Nazim wanted to take this while waiting for answers in other bugs. Thanks Nazim!

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks Emilio!

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1464496 - Part 1: Merge ServoDeclarationBlock and DeclarationBlock

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1464496 - Part 2: Move the mRaw before the two booleans for better packing

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

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8982000 [details]
Bug 1464496 - Part 1: Merge ServoDeclarationBlock and DeclarationBlock

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks for the review Emilio!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d6e582ce9bab
https://hg.mozilla.org/mozilla-central/rev/edc3670ec282