Closed Bug 1390255 Opened 2 years ago Closed 2 years ago

stylo: A few stylist-related cleanups.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file, 1 obsolete file)

This is work I did today with the aim at improving stylist performance.

Didn't help much, but I think cleanups are welcome, since I'm going to shuffle more stuff around soon.
Attachment #8897090 - Flags: review?(cam)
Attachment #8897091 - Flags: review?(cam)
Comment on attachment 8897090 [details]
style: Avoid multiple selector walk-ups during SelectorMap insertion.

https://reviewboard.mozilla.org/r/168394/#review173894

::: servo/components/style/selector_map.rs:405
(Diff revision 1)
>  /// Searches a compound selector from left to right. If the compound selector
>  /// is a pseudo-element, it's ignored.
>  ///
>  /// The first non-None value returned from |f| is returned.

This comment needs updating.

::: servo/components/style/selector_map.rs:416
(Diff revision 1)
> -                return Some(r)
> -            }
> +            match new_bucket {
> +                Bucket::ID(..) => return new_bucket,
> +                Bucket::Class(..) => {
> +                    current_bucket = new_bucket;
> -        }
> +                }
> +                Bucket::LocalName { .. } => {
> +                    if matches!(current_bucket, Bucket::Universal) {
> +                        current_bucket = new_bucket;
> -    }
> +                    }
> -
> -    None
> -}
> +                }
> -
> +                Bucket::Universal => {},
> -/// Retrieve the first ID name in the selector, or None otherwise.
> -#[inline(always)]
> -pub fn get_id_name(iter: SelectorIter<SelectorImpl>)
> -                   -> Option<Atom> {
> -    find_from_left(iter, |ss| {
> -        if let Component::ID(ref id) = *ss {
> -            return Some(id.clone());
> -        }
> +            }

What do you think about having a helper function like is_higher_priority_than() on Bucket, to make this choosing a bit clearer?  If you don't think it would be clearer, don't worry about it, but maybe comment in here (or in the updated function comment) that the job here is to find the highest priority bucket.
Attachment #8897090 - Flags: review?(cam) → review+
Comment on attachment 8897091 [details]
Bug 1390255: stylo: Remove unused Servo_StyleSet_Clear.

https://reviewboard.mozilla.org/r/168396/#review173902

Great to see the dirty state management getting even simpler!

::: servo/ports/geckolib/glue.rs:993
(Diff revision 2)
>      for origin in OriginSet::from(changed_origins).iter() {
>          data.stylesheets.force_dirty_origin(&origin);
> -        data.clear_stylist_origin(&origin);
>      }

Guess it might be nice to make force_dirty_origin take an OriginSet.
Attachment #8897091 - Flags: review?(cam) → review+
Comment on attachment 8897090 [details]
style: Avoid multiple selector walk-ups during SelectorMap insertion.

https://reviewboard.mozilla.org/r/168394/#review173894

> What do you think about having a helper function like is_higher_priority_than() on Bucket, to make this choosing a bit clearer?  If you don't think it would be clearer, don't worry about it, but maybe comment in here (or in the updated function comment) that the job here is to find the highest priority bucket.

Thought about that, but then you need to check again to early-return, etc. I left a comment though.
Comment on attachment 8897091 [details]
Bug 1390255: stylo: Remove unused Servo_StyleSet_Clear.

https://reviewboard.mozilla.org/r/168396/#review173902

> Guess it might be nice to make force_dirty_origin take an OriginSet.

Yup, I agree. I did that and renamed it to `force_dirty`.
Attachment #8897090 - Attachment is obsolete: true
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s eddeddef1d24 -d f25c91cca4c3: rebasing 413949:eddeddef1d24 "Bug 1390255: stylo: Remove unused Servo_StyleSet_Clear. r=heycam" (tip)
merging layout/style/ServoBindingList.h
warning: conflicts while merging layout/style/ServoBindingList.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/571aa752282a
stylo: Remove unused Servo_StyleSet_Clear. r=heycam
https://hg.mozilla.org/mozilla-central/rev/571aa752282a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.