[PageLifecyle] Match the initial background state of the Renderer Scheduler to the Host [chromium/src : master]
Fadi Meawad (Gerrit)
Fadi Meawad uploaded patch set #5 to this change.
[PageLifecyle] Match the initial background state of the Renderer Scheduler to the Host
This CL initializes the background state on Android to true to match what
the RenderProcessHostImpl initial priority think it is.
This CL fixes an issue where the scheduler assumes that it is foregrounded
while it is actually backgrounded during the initial phase. Once the state
changes, the state was correctly maintained.
Bug:chromium:780191
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
---
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
1 file changed, 15 insertions(+), 6 deletions(-)
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Fadi Meawad (Gerrit)
Fadi Meawad would like Alexander Timin to review this change.
[PageLifecyle] Match the initial background state of the Renderer Scheduler to the Host
This CL initializes the background state on Android to true to match what
the RenderProcessHostImpl initial priority think it is.
This CL fixes an issue where the scheduler assumes that it is foregrounded
while it is actually backgrounded during the initial phase. Once the state
changes, the state was correctly maintained.
Bug:chromium:780191
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
---
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
1 file changed, 15 insertions(+), 6 deletions(-)
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Fadi Meawad (Gerrit)
I was fix this issue incorrectly, my first approach was to send the first correct state back regardless from the host to the scheduler.
My second approach was to sync the initial state (not the correct first state).
While both approach do work correctly, they are not the best solution.
The correct solution is to match the initial state on both side, hence I move the fix to the scheduler.
altimin@, can you PTAL.
Fadi Meawad (Gerrit)
Fadi Meawad removed Bo from this change.
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Fadi Meawad (Gerrit)
Fadi Meawad uploaded patch set #6 to this change.
[PageLifecyle] Match the initial background state of the Renderer Scheduler to the Host
This CL initializes the background state on Android to match what the
RenderProcessHostImpl initial priority. Initial state in RenderProcessHost
is backgrounded, while initial state in the scheduler was foregrounded.
This CL fixes an issue where the scheduler assumes that it is foregrounded
while it is actually backgrounded during the initial phase. Once the state
changes, the state was correctly maintained.
Bug:chromium:780191
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
---
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
1 file changed, 15 insertions(+), 6 deletions(-)
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Bo (Gerrit)
1 comment:
File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:
Patch Set #6, Line 245:
renderer_backgrounded(true,it would be nice if this can be taken from the same source as RPHI, so they never diverge:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_process_host_impl.cc?rcl=a90bc9d771b96aa1f7216be715ce5cc9af3fd5e3&l=280failing that, should have comments that point to each other, ie should have a comment in RPHI as well
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Fadi Meawad (Gerrit)
Address comment. PTAL.
1 comment:
Patch Set #6, Line 245:
renderer_backgrounded(true,
it would be nice if this can be taken from the same source as RPHI, so they never diverge: […]
Added a comment to RPHI
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Bo (Gerrit)
Patch set 7:Code-Review +1
Alexander Timin (Gerrit)
Thanks for doing this!
1 comment:
File content/browser/renderer_host/render_process_host_impl.cc:
Patch Set #7, Line 282:
kLaunchingProcessIsBackgroundedI wonder if we could move this declaration to RendererScheduler in public/platform/scheduler —- this will make this definition available both to blink scheduler and content layer.
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Bo (Gerrit)
1 comment:
Patch Set #7, Line 282:
kLaunchingProcessIsBackgroundedI wonder if we could move this declaration to RendererScheduler in public/platform/scheduler —- this will make this definition available both to blink scheduler and content layer.
browser code shouldn't (and probably can't) include renderer code.
is there an equivalent of content/common in blink that makes sense?
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Timin (Gerrit)
+haraken@ for content/browser and platform/scheduler layering (tl;dr: we want to make the same value available both in content/browser and platform/scheduler/renderer).
1 comment:
Patch Set #7, Line 282:
kLaunchingProcessIsBackgrounded
> I wonder if we could move this declaration to RendererScheduler in public/platform/scheduler —- th […]
+haraken@
Hm, completely missed that it is the browser process. :(
However, it is possible to whitelist certain files in blink and include them from content/browser: https://cs.chromium.org/chromium/src/content/browser/DEPS?type=cs&q=f:content/browser+blink+package:%5Echromium$&l=127
Kentaro-san, do you think it is reasonable to add public/platform/scheduler/common/initial_process_visibility.h? Or should it go somewhere else?
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #7, Line 282:
kLaunchingProcessIsBackgrounded
+haraken@ […]
For code that is shared between browser and renderer, you can use WebKit/common/.
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Fadi Meawad (Gerrit)
Fadi Meawad uploaded patch set #10 to this change.
[PageLifecyle] Move the launch Renderer background state to Webkit/common
This CL introduces Webkit/common/renderer/initial_state.h that includes
the launch background and launch boosted state.
The reason this file is needed, is because the state is different per OS
but needs to be consistent between the RenderProcessHostImpl and the
RendererSchedulerImpl. To prevent future changes in one not reflecting
in the other, the code is moved to a common place and shared.
This CL fixes an issue where the scheduler assumes that it is
foregrounded while it is actually backgrounded during the initial phase
on Android.
Bug:chromium:780191
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
---
M content/browser/renderer_host/render_process_host_impl.cc
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
M third_party/WebKit/common/BUILD.gn
A third_party/WebKit/common/renderer/initial_state.h
4 files changed, 34 insertions(+), 15 deletions(-)
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Bo (Gerrit)
2 comments:
File content/browser/renderer_host/render_process_host_impl.cc:
missing some include and code updates here
File third_party/WebKit/common/renderer/initial_state.h:
Patch Set #10, Line 5:
RENDERERbrowser still shouldn't be including renderer things, should just move this up one level?
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
WebKit/common/ is a place to put code shared by browser and renderer. So it's not right to put code used only by renderer.
You should put the code to WebKit/public/.
Fadi Meawad (Gerrit)
Patch Set 11:
WebKit/common/ is a place to put code shared by browser and renderer. So it's not right to put code used only by renderer.
You should put the code to WebKit/public/.
But it is used by the RenderProcessHostImpl, isn't that considered in the browser?
How about Bo's suggestion to move it to one level up, i.e. Webkit/common/ rather than Webkit/common/renderer/ and name it:
process_launch_initial_state.h?
I will address the rest of the comments on the CL in details on Monday.
1 comment:
File content/browser/renderer_host/render_process_host_impl.cc:
Patch Set #10, Line 277:
const char kSiteProcessMapKeyName[] = "content_site_process_map";
missing some include and code updates here
Yes, I accidentally stashed then uploaded last changes. Fixed now.
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Patch Set 11:
(1 comment)
Patch Set 11:
WebKit/common/ is a place to put code shared by browser and renderer. So it's not right to put code used only by renderer.
You should put the code to WebKit/public/.
But it is used by the RenderProcessHostImpl, isn't that considered in the browser?
How about Bo's suggestion to move it to one level up, i.e. Webkit/common/ rather than Webkit/common/renderer/ and name it:
process_launch_initial_state.h?
I will address the rest of the comments on the CL in details on Monday.
Ah, got it. You're right.
Maybe we can use WebKit/common/page/? We should avoid putting files at the top level. On the other hand, WebKit/common/renderer/ sounds too generic and confusing (because the code is shared between browser and renderer)...
Kentaro Hara (Gerrit)
With that change, LGTM.
Patch set 11:Code-Review +1
Fadi Meawad (Gerrit)
Fadi Meawad uploaded patch set #13 to this change.
[PageLifecyle] Move the launch process state to Webkit/common
This CL introduces Webkit/common/page/launching_process_state.h that
includes the launch background and launch boosted state.
The reason this file is needed, is because the state is different per
OS but needs to be consistent between the RenderProcessHostImpl and
the RendererSchedulerImpl. To prevent future changes in one not
reflecting in the other, the code is moved to a common place and
shared.
This CL fixes an issue where the scheduler assumes that it is
foregrounded while it is actually backgrounded during the initial
phase on Android.
Bug:chromium:780191
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
---
M content/browser/renderer_host/render_process_host_impl.cc
M third_party/WebKit/Source/platform/scheduler/BUILD.gn
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
M third_party/WebKit/common/BUILD.gn
A third_party/WebKit/common/page/launching_process_state.h
5 files changed, 46 insertions(+), 21 deletions(-)
To view, visit change 815678. To unsubscribe, or for help writing mail filters, visit settings.
Fadi Meawad (Gerrit)
haraken@, I have made the proposed changes. Can you take another look.
Thanks.
Kentaro Hara (Gerrit)
LGTM
Fadi Meawad (Gerrit)
Patch set 14:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix windows build" https://chromium-review.googlesource.com/c/815678/14
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/815678/14
Bot data: {"action": "start", "triggered_at": "2017-12-19T03:39:18.0Z", "cq_cfg_revision": "449ceedce2207329148e0a132f0c79a46cde3381", "revision": "58ee51ef250e813c2a53706bbc2e8a3cf2714455"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/455511)
Alexander Timin (Gerrit)
scheduler/ lgtm! Thanks for futureproofing this!
Patch set 14:Code-Review +1
Fadi Meawad (Gerrit)
Patch set 15:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase after conflicting https://chromium-review.googlesource.com/c/chromium/src/+/833217" https://chromium-review.googlesource.com/c/815678/15
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/815678/15
Bot data: {"action": "start", "triggered_at": "2017-12-19T18:42:23.0Z", "cq_cfg_revision": "449ceedce2207329148e0a132f0c79a46cde3381", "revision": "0f1a2947597869471037b661e8d6ae3de8ab54fb"}
Fadi Meawad (Gerrit)
Patch set 15:-Commit-Queue
Fadi Meawad (Gerrit)
It turns out that there is a few tests (including one I added recently) assume that the initial state is always foregrounded. I am fixing those tests now.
(You can see those failing on the android bots, since those are the ones with the inverted initial state.)
Fadi Meawad (Gerrit)
Patch set 17:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Minor" https://chromium-review.googlesource.com/c/815678/17
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/815678/17
Bot data: {"action": "start", "triggered_at": "2017-12-21T01:59:31.0Z", "cq_cfg_revision": "449ceedce2207329148e0a132f0c79a46cde3381", "revision": "3b4f9803e3c92de4785983aa148e8d0a0ecd3d6e"}
Commit Bot (Gerrit)
Commit Bot merged this change.
[PageLifecyle] Move the launch process state to Webkit/common
This CL introduces Webkit/common/page/launching_process_state.h that
includes the launch background and launch boosted state.
The reason this file is needed, is because the state is different per
OS but needs to be consistent between the RenderProcessHostImpl and
the RendererSchedulerImpl. To prevent future changes in one not
reflecting in the other, the code is moved to a common place and
shared.
This CL fixes an issue where the scheduler assumes that it is
foregrounded while it is actually backgrounded during the initial
phase on Android.
Bug: chromium:780191
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
Reviewed-on: https://chromium-review.googlesource.com/815678
Commit-Queue: Fadi Meawad <fme...@chromium.org>
Reviewed-by: Alexander Timin <alt...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Bo <bo...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525563}
---
M content/browser/renderer_host/render_process_host_impl.cc
M third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator.h
M third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator_unittest.cc
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_metrics_helper_unittest.cc
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
M third_party/WebKit/common/BUILD.gn
A third_party/WebKit/common/page/launching_process_state.h
8 files changed, 149 insertions(+), 87 deletions(-)
Markus Heintz (Gerrit)
Could this have broken: https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Android%20(Nexus4)/72532 ?