1435022 - Clean up the CompositorVsyncScheduler code

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

This code makes me very sad anytime I get anywhere near it. Mostly because the functions all sound the same, it's not obvious from the names what calls what, and it's hard to hold in my head. It's causing me grief now with WR mochitest stuff so I'm going to clean it up. (I already have patches, but need to make sure they didn't break anything).

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

Looking good so far

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Sorry in advance for the giant patchset but each patch is pretty small and basically standalone so hopefully it should be easy to review.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Inline OnForceComposeToTarget and clarify the comment.

This function has only one call site and there's no need to make it
public.

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Make ScheduleTask private and drop a useless argument.

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Remove the ResumeComposition function from the vsync scheduler.

Having a resume function without a corresponding pause function seems
silly. And the implementation of the function is the same as doing a
force-compose, which is what the call site is intending to do. So we can
just remove this function and do a force-compose instead.

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Inline a one-line function with a confusing name.

This function is exposed as public but only ever called from two places,
both of which are inside the class. It's confusing to have it as a
separate function because it just makes it harder to trace through the
code.

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Make a couple of functions private.

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Minor cleanup on the Destroy function.

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Inline the SetNeedsComposite function.

SetNeedsComposite is only ever called from one place on the compositor
thread, but it has a bunch of generic boilerplate to handle being called
from any thread. If we inline it we don't need the extra boilerplate and
it's much simpler to follow the code.

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Make the Composite function private and document/clean it.

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

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Document some methods.

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

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Clean up GetLastComposeTime.

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

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Remove useless code (mExpectedComposeStartTime is never assigned).

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

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435022 - Remove more unused stuff.

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

Comment 15

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947622 [details]
Bug 1435022 - Inline OnForceComposeToTarget and clarify the comment.

https://reviewboard.mozilla.org/r/217318/#review223252

Comment 16

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947623 [details]
Bug 1435022 - Make ScheduleTask private and drop a useless argument.

https://reviewboard.mozilla.org/r/217320/#review223254

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947624 [details]
Bug 1435022 - Remove the ResumeComposition function from the vsync scheduler.

https://reviewboard.mozilla.org/r/217322/#review223256

Comment 18

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947625 [details]
Bug 1435022 - Inline a one-line function with a confusing name.

https://reviewboard.mozilla.org/r/217324/#review223258

Comment 19

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947626 [details]
Bug 1435022 - Make a couple of functions private.

https://reviewboard.mozilla.org/r/217326/#review223260

Comment 20

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947627 [details]
Bug 1435022 - Minor cleanup on the Destroy function.

https://reviewboard.mozilla.org/r/217328/#review223262

Comment 21

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947629 [details]
Bug 1435022 - Make the Composite function private and document/clean it.

https://reviewboard.mozilla.org/r/217332/#review223264

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947630 [details]
Bug 1435022 - Document some methods.

https://reviewboard.mozilla.org/r/217334/#review223266

Comment 23

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947631 [details]
Bug 1435022 - Clean up GetLastComposeTime.

https://reviewboard.mozilla.org/r/217336/#review223268

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947632 [details]
Bug 1435022 - Remove useless code (mExpectedComposeStartTime is never assigned).

https://reviewboard.mozilla.org/r/217338/#review223270

Comment 25

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947633 [details]
Bug 1435022 - Remove more unused stuff.

https://reviewboard.mozilla.org/r/217340/#review223272

Comment 26

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947628 [details]
Bug 1435022 - Inline the SetNeedsComposite function.

https://reviewboard.mozilla.org/r/217330/#review223276

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3e3d4a0fd58
Inline OnForceComposeToTarget and clarify the comment. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/20a81a819636
Make ScheduleTask private and drop a useless argument. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/f91eebe2f252
Remove the ResumeComposition function from the vsync scheduler. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/e7f0a181d2eb
Inline a one-line function with a confusing name. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/d5aa68597329
Make a couple of functions private. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/1010e506680a
Minor cleanup on the Destroy function. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/89844912178c
Inline the SetNeedsComposite function. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/5d98a33f8f3e
Make the Composite function private and document/clean it. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/5c9f4191c07b
Document some methods. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/956046e5d39f
Clean up GetLastComposeTime. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/18ed51506372
Remove useless code (mExpectedComposeStartTime is never assigned). r=sotaro
https://hg.mozilla.org/integration/autoland/rev/18f9c8f22beb
Remove more unused stuff. r=sotaro

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b3e3d4a0fd58
https://hg.mozilla.org/mozilla-central/rev/20a81a819636
https://hg.mozilla.org/mozilla-central/rev/f91eebe2f252
https://hg.mozilla.org/mozilla-central/rev/e7f0a181d2eb
https://hg.mozilla.org/mozilla-central/rev/d5aa68597329
https://hg.mozilla.org/mozilla-central/rev/1010e506680a
https://hg.mozilla.org/mozilla-central/rev/89844912178c
https://hg.mozilla.org/mozilla-central/rev/5d98a33f8f3e
https://hg.mozilla.org/mozilla-central/rev/5c9f4191c07b
https://hg.mozilla.org/mozilla-central/rev/956046e5d39f
https://hg.mozilla.org/mozilla-central/rev/18ed51506372
https://hg.mozilla.org/mozilla-central/rev/18f9c8f22beb