Closed Bug 1388298 Opened 7 years ago Closed 7 years ago

stylo: Add a ServoRestyleManager API that processes all invalidations on the main thread.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

Bug 1388298 - Add an API to process all invalidations on the main thread.
59 bytes, text/x-review-board-request
emilio
: review+
Details
x.patch
2.15 KB, patch
Details | Diff | Splinter Review
Priority: -- → P4
Comment on attachment 8900664 [details]
Bug 1388298 - Add an API to process all invalidations on the main thread.

https://reviewboard.mozilla.org/r/172082/#review177364

::: servo/ports/geckolib/glue.rs:3663
(Diff revision 1)
> +    let per_doc_data = PerDocumentStyleData::from_ffi(set).borrow();
> +    let shared_style_context = create_shared_context(&global_style_data,
> +                                                     &guard,
> +                                                     &per_doc_data,
> +                                                     TraversalFlags::empty(),
> +                                                     unsafe { &*snapshots });

It'd be nice not to do this work only once... But probably not a huge deal. We can improve it if it becomes hot.

::: servo/ports/geckolib/glue.rs:3664
(Diff revision 1)
> +    let shared_style_context = create_shared_context(&global_style_data,
> +                                                     &guard,
> +                                                     &per_doc_data,
> +                                                     TraversalFlags::empty(),
> +                                                     unsafe { &*snapshots });
> +    let element = GeckoElement(element);

Let's:

`debug_assert!(element.has_snapshot())`.

And also:

`debug_assert!(!element.handled_snapshot())`.

::: servo/ports/geckolib/glue.rs:3666
(Diff revision 1)
> +                                                     &per_doc_data,
> +                                                     TraversalFlags::empty(),
> +                                                     unsafe { &*snapshots });
> +    let element = GeckoElement(element);
> +    let mut data = element.mutate_data();
> +    let mut data = data.as_mut().map(|d| &mut **d);

nit: You can probably assert that the element has data here...
Attachment #8900664 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8900664 [details]
Bug 1388298 - Add an API to process all invalidations on the main thread.

https://reviewboard.mozilla.org/r/172082/#review177364

> It'd be nice not to do this work only once... But probably not a huge deal. We can improve it if it becomes hot.

Err, to do this work only once, I meant :)
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 4 changes to 4 files
remote: (4b95ebddff8b modifies servo/ports/geckolib/glue.rs from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: 4b95ebddff8b)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote
Unfortunately you have to manually split the patch up into two -- one for the Gecko changes to land here, and one for the Servo changes, that you'll need to file a PR for on GitHub.  Once the Servo part merges, you can click autoland here to land the Gecko-only part.
Flags: needinfo?(wpan)
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 4 changes to 4 files
remote: (d54b504fe5f0 modifies servo/ports/geckolib/glue.rs from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: d54b504fe5f0)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote
Not sure why CI failed though.
Flags: needinfo?(wpan)
Don't know.  Maybe because the tree was closed earlier and homu got confused.  I told it to retry.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4142a9865b92
Add an API to process all invalidations on the main thread. r=emilio
I pushed this since this landed and this looked good. However, I think we may've missed something, which is propagating the dirty bits up the tree. So if you're experimenting with it, probably you need to fix that.
https://hg.mozilla.org/mozilla-central/rev/4142a9865b92
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Attached patch x.patch — — Splinter Review 
I guess this should do the work, am I correct? Probably need another bug for this though.
Attachment #8901721 - Flags: feedback?(emilio)
Comment on attachment 8901721 [details] [diff] [review]
x.patch

Review of attachment 8901721 [details] [diff] [review]:
-----------------------------------------------------------------

Almost but not quite. You need to also check whether the element itself got an invalidation. The FFI function you added could return a boolean that basically does that, or call `Gecko_NoteDirtyElement` from there.

But yeah, let's move this to another bug.
Attachment #8901721 - Flags: feedback?(emilio)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: