1344135 - stylo: ServoDeclarationBlock_SetPropertyById returns true in some cases when it should return false

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1344131 comment 2.  Servo's return boolean is "was this parsed successfully?" whereas the caller here wants "did the value actually change?".  Those are not the same thing at all.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Note to myself:
* make PropertyDeclarationBlock::set_parsed_declaration return whether anything is changed
* have glue.rs:set_property collect that and return

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1344135 - Return true in set_property only when declaration block is changed.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843822 [details]
Bug 1344135 - Return true in set_property only when declaration block is changed.

https://reviewboard.mozilla.org/r/117426/#review119062

::: servo/components/style/properties/declaration_block.rs:178
(Diff revision 1)
>      /// Adds or overrides the declaration for a given property in this block,
> -    /// without taking into account any kind of priority.
> +    /// without taking into account any kind of priority. Returns whether the
> +    /// declaration block is actually changed.
>      pub fn set_parsed_declaration(&mut self,
>                                    declaration: PropertyDeclaration,
> -                                  importance: Importance) {
> +                                  importance: Importance) -> bool {

While you're here, there's a TODO(emilio) in Servo's components/script/dom/csstyledeclaration.rs that you can resolve easily with this, if you did that, it'd be nice :)

::: servo/ports/geckolib/glue.rs:843
(Diff revision 1)
>          let mut declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations).write();
>          let importance = if is_important { Importance::Important } else { Importance::Normal };
> +        let mut changed = false;
>          for decl in decls.into_iter() {
> -            declarations.set_parsed_declaration(decl.0, importance);
> +            if declarations.set_parsed_declaration(decl.0, importance) {
> +                changed = true;

nit: you can do `changed |= declarations.set_parsed_declaration(..)`.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8843822 [details]
Bug 1344135 - Return true in set_property only when declaration block is changed.

https://reviewboard.mozilla.org/r/117426/#review119134

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

This has been fixed in https://github.com/servo/servo/pull/15837