Closed Bug 1297899 Opened 7 years ago Closed 6 years ago

stylo: Share append/insert/remove restyle logic with Gecko code

Categories

(Core :: CSS Parsing and Computation, defect, P2)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: heycam)

References

Details

Attachments

(10 files)

Bug 1297899 - Part 1: Rename RestyleManager.{h,cpp} to GeckoRestyleManager.{h,cpp}.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 2: Rename RestyleManagerBase.{h,cpp} to RestyleManager.{h,cpp}.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 3: Rename RestyleManager to GeckoRestyleManager and RestyleManagerBase to RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 4: Store concrete restyle manager type on RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 5: Move refcounting from concrete restyle manager classes up to RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 6: Move RestyleManagerHandle functionality into RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 7: Move PostRestyleEventForLazyConstruction up to RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 8: Move Content{Inserted,Appended} up to RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 9: Move RestyleFor{InsertOrChange,Append,EmptyChange} and ContentRemoved up to RestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1297899 - Part 10: Test expectation adjustment.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
There's a bunch of stuff that happens in RestyleManager::RestyleFor{Append,Insert,Remove} in order to handle changes to :empty selectors and so forth. This should be straightforward to share, but requires a bit of refactoring that I'm going to skip for now.
I'm looking into this. I'll see how can I address the atomic setting of the style flags, which is somewhat unfortunate.
Assignee: nobody → ecoal95
Thanks Emilio!

One thing I'd eventually like to do is to get rid of the handles and just have everything use base class pointers. We'd then use macros to have BaseClass::Foo do a type-check and then delegate to either GeckoClass::Foo or ServoClass::Foo. Shared could would live in BaseClass::FooInternal.

No need to do that in this patch, but just be aware of the eventual model when doing refactoring that involves the superclass.
Priority: -- → P2
Blocks: 1330885
Cameron was going to look at this.
Flags: needinfo?(cam)
Assignee: emilio+bugs → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Now that we support the node bits to indicate slow selectors etc., we can basically hoist the restyle logic for content insertion/removal up to the base class without change.
Although we do have some support for the node selector bits, we don't always set them correctly, so dynamic updates involving sibling selectors still don't work properly.  But we can fix those after this refactoring.
(In reply to Cameron McCormack (:heycam) from comment #5)
> Although we do have some support for the node selector bits, we don't always
> set them correctly, so dynamic updates involving sibling selectors still
> don't work properly.  But we can fix those after this refactoring.

Please file any such issues as blockers against bug 1337075.
Comment on attachment 8836550 [details]
Bug 1297899 - Part 1: Rename RestyleManager.{h,cpp} to GeckoRestyleManager.{h,cpp}.

https://reviewboard.mozilla.org/r/111948/#review113486
Attachment #8836550 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836551 [details]
Bug 1297899 - Part 2: Rename RestyleManagerBase.{h,cpp} to RestyleManager.{h,cpp}.

https://reviewboard.mozilla.org/r/111950/#review113488
Attachment #8836551 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836552 [details]
Bug 1297899 - Part 3: Rename RestyleManager to GeckoRestyleManager and RestyleManagerBase to RestyleManager.

https://reviewboard.mozilla.org/r/111952/#review113494

rs=me
Attachment #8836552 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836553 [details]
Bug 1297899 - Part 4: Store concrete restyle manager type on RestyleManager.

https://reviewboard.mozilla.org/r/111954/#review113496
Attachment #8836553 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836554 [details]
Bug 1297899 - Part 5: Move refcounting from concrete restyle manager classes up to RestyleManager.

https://reviewboard.mozilla.org/r/111956/#review113498
Attachment #8836554 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836555 [details]
Bug 1297899 - Part 6: Move RestyleManagerHandle functionality into RestyleManager.

https://reviewboard.mozilla.org/r/111958/#review113500

Thank you for doing this cleanup!
Attachment #8836555 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836556 [details]
Bug 1297899 - Part 7: Move PostRestyleEventForLazyConstruction up to RestyleManager.

https://reviewboard.mozilla.org/r/111960/#review113502
Attachment #8836556 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836557 [details]
Bug 1297899 - Part 8: Move Content{Inserted,Appended} up to RestyleManager.

https://reviewboard.mozilla.org/r/111962/#review113504
Attachment #8836557 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836559 [details]
Bug 1297899 - Part 10: Test expectation adjustment.

https://reviewboard.mozilla.org/r/111966/#review113514
Attachment #8836559 - Flags: review?(bobbyholley) → review+
Comment on attachment 8836558 [details]
Bug 1297899 - Part 9: Move RestyleFor{InsertOrChange,Append,EmptyChange} and ContentRemoved up to RestyleManager.

https://reviewboard.mozilla.org/r/111964/#review113512

r=me assuming this is just a simple move.
Attachment #8836558 - Flags: review?(bobbyholley) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f24f346ce8cd
Part 1: Rename RestyleManager.{h,cpp} to GeckoRestyleManager.{h,cpp}. r=bholley
https://hg.mozilla.org/integration/autoland/rev/09d3a31f1160
Part 2: Rename RestyleManagerBase.{h,cpp} to RestyleManager.{h,cpp}. r=bholley
https://hg.mozilla.org/integration/autoland/rev/27ebaadd9a79
Part 3: Rename RestyleManager to GeckoRestyleManager and RestyleManagerBase to RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/16eb31796cc6
Part 4: Store concrete restyle manager type on RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/92187fa35689
Part 5: Move refcounting from concrete restyle manager classes up to RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/5b3ce8ae4965
Part 6: Move RestyleManagerHandle functionality into RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/fd140aa9a1f6
Part 7: Move PostRestyleEventForLazyConstruction up to RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/767da488b5dd
Part 8: Move Content{Inserted,Appended} up to RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/41cd73463ef9
Part 9: Move RestyleFor{InsertOrChange,Append,EmptyChange} and ContentRemoved up to RestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/6504c3bc8435
Part 10: Test expectation adjustment. r=bholley
You need to log in before you can comment on or make changes to this bug.