1676760 - ParentProcessVsyncNotifier and NotifyParentProcessVsync is confusing, should be renamed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Apparently some of the refresh driver vsync code that has "ParentProcess" in its name is now used in the content process, as of bug 1641033.

See bug 1641033 comment 11:

(In reply to Olli Pettay [:smaug] from bug 1641033 comment #11)

And don't now also child processes use code which is named like "ParentProcessVsyncNotifier", or am I missing something obvious?

Furthermore, bug 1661881 added this particularly strange hunk of code:

      if (XRE_IsContentProcess()) {
        NotifyParentProcessVsync();
        return true;
      }

This should be cleaned up and the naming should be fixed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:boris, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Replace VsyncRefreshDriverTimer constructors with named creation methods. r=smaug

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Fix comments on VsyncRefreshDriverTimer member variables. r=smaug

Depends on D139762

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Rename NotifyParentProcessVsync to NotifyVsyncOnMainThread and add some comments. r=smaug

Depends on D139763

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Always use vsync priority for the ParentProcessVsyncNotifier runnable. r=smaug

I think this is ok now? Or are there any remaining non-e10s scenarios that we need to keep working (and would they even break?)?

Depends on D139764

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Remove ParentProcessVsyncNotifier. r=smaug

Seems excessive to have a separate class for this, it really just calls a method on a different thread.

Depends on D139765

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Use DataMutex for pending vsync tracking and rename variables so that they don't refer to "parent process" even though they're also used in the content process. r=smaug

Depends on D139766

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Remove unused RefreshTimerVsyncDispatcher::SetParentRefreshTimer. r=smaug

Even in the parent process, VsyncRefreshDriverTimer uses AddChildRefreshTimer.

Depends on D139767

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1676760 - Rename Add/RemoveChildRefreshTimer. r=smaug

It is used by far more than just content process refresh driver timers.

Depends on D139768

Comment 10

[Markus Stange [:mstange]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97bde4b52343
Replace VsyncRefreshDriverTimer constructors with named creation methods. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2ded755f08d0
Fix comments on VsyncRefreshDriverTimer member variables. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3f523ecfbd28
Rename NotifyParentProcessVsync to NotifyVsyncOnMainThread and add some comments. r=smaug
https://hg.mozilla.org/integration/autoland/rev/45f91f30f8b4
Always use vsync priority for the ParentProcessVsyncNotifier runnable. r=smaug
https://hg.mozilla.org/integration/autoland/rev/dc08f59ac67a
Remove ParentProcessVsyncNotifier. r=smaug
https://hg.mozilla.org/integration/autoland/rev/cfa85021bc19
Use DataMutex for pending vsync tracking and rename variables so that they don't refer to "parent process" even though they're also used in the content process. r=smaug
https://hg.mozilla.org/integration/autoland/rev/50b8f8c958d3
Remove unused RefreshTimerVsyncDispatcher::SetParentRefreshTimer. r=smaug
https://hg.mozilla.org/integration/autoland/rev/aa17afd2b411
Rename Add/RemoveChildRefreshTimer. r=smaug

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/97bde4b52343
https://hg.mozilla.org/mozilla-central/rev/2ded755f08d0
https://hg.mozilla.org/mozilla-central/rev/3f523ecfbd28
https://hg.mozilla.org/mozilla-central/rev/45f91f30f8b4
https://hg.mozilla.org/mozilla-central/rev/dc08f59ac67a
https://hg.mozilla.org/mozilla-central/rev/cfa85021bc19
https://hg.mozilla.org/mozilla-central/rev/50b8f8c958d3
https://hg.mozilla.org/mozilla-central/rev/aa17afd2b411