Closed Bug 1344135 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox54 --- affected

People

(Reporter: bzbarsky, Assigned: xidorn)

References

Details

Attachments

(1 file)

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.
Blocks: 1344131
Flags: needinfo?(xidorn+moz)
Note to myself:
* make PropertyDeclarationBlock::set_parsed_declaration return whether anything is changed
* have glue.rs:set_property collect that and return
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
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 on attachment 8843822 [details]
Bug 1344135 - Return true in set_property only when declaration block is changed.

https://reviewboard.mozilla.org/r/117426/#review119134
Attachment #8843822 - Flags: review?(cam) → review+
This has been fixed in https://github.com/servo/servo/pull/15837
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.