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

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I'm looking into this. I'll see how can I address the atomic setting of the style flags, which is somewhat unfortunate.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

Cameron was going to look at this.

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 1: Rename RestyleManager.{h,cpp} to GeckoRestyleManager.{h,cpp}.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 2: Rename RestyleManagerBase.{h,cpp} to RestyleManager.{h,cpp}.

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

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 3: Rename RestyleManager to GeckoRestyleManager and RestyleManagerBase to RestyleManager.

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

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 4: Store concrete restyle manager type on RestyleManager.

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

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 5: Move refcounting from concrete restyle manager classes up to RestyleManager.

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

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 6: Move RestyleManagerHandle functionality into RestyleManager.

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

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 7: Move PostRestyleEventForLazyConstruction up to RestyleManager.

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

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 8: Move Content{Inserted,Appended} up to RestyleManager.

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

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 9: Move RestyleFor{InsertOrChange,Append,EmptyChange} and ContentRemoved up to RestyleManager.

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

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Bug 1297899 - Part 10: Test expectation adjustment.

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

Comment 16

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=337b57f6aa826efa602b91073435c659b3685bc9

Comment 17

[Bobby Holley (:bholley)]

(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 18

[Bobby Holley (:bholley)]

Comment on attachment 8836550 [details]
Bug 1297899 - Part 1: Rename RestyleManager.{h,cpp} to GeckoRestyleManager.{h,cpp}.

https://reviewboard.mozilla.org/r/111948/#review113486

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8836551 [details]
Bug 1297899 - Part 2: Rename RestyleManagerBase.{h,cpp} to RestyleManager.{h,cpp}.

https://reviewboard.mozilla.org/r/111950/#review113488

Comment 20

[Bobby Holley (:bholley)]

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

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8836553 [details]
Bug 1297899 - Part 4: Store concrete restyle manager type on RestyleManager.

https://reviewboard.mozilla.org/r/111954/#review113496

Comment 22

[Bobby Holley (:bholley)]

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

Comment 23

[Bobby Holley (:bholley)]

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!

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8836556 [details]
Bug 1297899 - Part 7: Move PostRestyleEventForLazyConstruction up to RestyleManager.

https://reviewboard.mozilla.org/r/111960/#review113502

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8836557 [details]
Bug 1297899 - Part 8: Move Content{Inserted,Appended} up to RestyleManager.

https://reviewboard.mozilla.org/r/111962/#review113504

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8836559 [details]
Bug 1297899 - Part 10: Test expectation adjustment.

https://reviewboard.mozilla.org/r/111966/#review113514

Comment 27

[Bobby Holley (:bholley)]

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.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f24f346ce8cd
https://hg.mozilla.org/mozilla-central/rev/09d3a31f1160
https://hg.mozilla.org/mozilla-central/rev/27ebaadd9a79
https://hg.mozilla.org/mozilla-central/rev/16eb31796cc6
https://hg.mozilla.org/mozilla-central/rev/92187fa35689
https://hg.mozilla.org/mozilla-central/rev/5b3ce8ae4965
https://hg.mozilla.org/mozilla-central/rev/fd140aa9a1f6
https://hg.mozilla.org/mozilla-central/rev/767da488b5dd
https://hg.mozilla.org/mozilla-central/rev/41cd73463ef9
https://hg.mozilla.org/mozilla-central/rev/6504c3bc8435