Move deferred commit logic from WebViewImpl to Document.

Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

This patch doesn't change any behavior and only moves the logic.

BUG=521692
TEST=DocumentLoadingRenderingTest.*

Committed: https://crrev.com/e10293b755e7e28e5f82adeebee7a2f1b630a6a6
Cr-Commit-Position: refs/heads/master@{#389829}

[esprehn, 2016-04-26 01:18:27]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 01:18:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922793002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922793002/1

[commit-bot: I haz the power, 2016-04-26 02:45:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 02:45:36]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-04-26 09:18:29]

Description was changed from

==========
Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

BUG=521692
==========

to

==========
Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

This patch doesn't change any behavior and
only moves the logic.

BUG=521692
==========

[esprehn, 2016-04-26 09:20:17]

esprehn@chromium.org changed reviewers:
+ bmcquade@chromium.org, dglazkov@chromium.org

[esprehn, 2016-04-26 09:20:18]

This is all covered by the DocumentLoadingRendering unit test this time! Yay. :)

[Bryan McQuade, 2016-04-26 14:28:34]

LGTM. Thanks for the cleanup!

Adding rdsmith as FYI since this change paves the way for removing some
additional code that's only currently needed by ResourceScheduler. Randy, can we
remove the willInsertBody code as part of your work to remove ResourceScheduler?

https://codereview.chromium.org/1922793002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1922793002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:966:
m_webFrame->client()->willInsertBody(m_webFrame);
it's great that we're reducing the callbacks from blink to the outside that have
to know about inserting the body of the document. seems like clients should not
have to know about an implementation detail like body being inserted, especially
given that not all types of documents have a body.

it looks like the only code at this point that cares about the body being
inserted is in ResourceScheduler::OnWillInsertBody. rdsmith is working on
removing ResourceScheduler, so when that goes, it seems like we should be able
to remove this method, as well as willInsertBody and the downstream plumbing.

will add rdsmith to this review as FYI so he can remove this as part of his
ResourceScheduler work.

[dglazkov, 2016-04-26 16:49:06]

LGTM!

[Randy Smith (Not in Mondays), 2016-04-26 17:29:15]

On 2016/04/26 14:28:34, Bryan McQuade wrote:
> LGTM. Thanks for the cleanup!
> 
> Adding rdsmith as FYI since this change paves the way for removing some
> additional code that's only currently needed by ResourceScheduler. Randy, can
we
> remove the willInsertBody code as part of your work to remove
ResourceScheduler?

I don't think so, but I don't know the blink code well enough to be sure.  Yes,
ResourceScheduler is the only current consumer of it, but the information (when
we hit the body tag) is still needed in the world where ResourceScheduler
doesn't exist, at least while we're keeping the basic prioritization and
ordering scheme we currently have with web page parsing.  My plan is to have
that information encoded in a different way; specifically, my plan is to have
all requests that would be throttled to "just one request active until all
renderer blocking requests dispatched before <body> tag complete" put in a new
priority bucket that's more restrictive than IDLE (THROTTLED) and have the
renderer re-prioritize all those requests when it passes the the somewhat
complicated point described in my quotes above :-}.  But that means that the
renderer still needs to know when the body tag is hit.  It may have other ways
to figure that out (depending on where I eventually put in the new renderer
code) but it may not.  I'll try to keep the possibility of removing
willInsertBody in mind as I put in the new code.

See
https://docs.google.com/document/d/1p-XGz-BFFehs5j2olbkDkjrXzxzT3n45PVcLSZzOV...
for more details (for folks other than Bryan; he's already seen that doc).

[esprehn, 2016-04-26 18:31:16]

On 2016/04/26 at 17:29:15, rdsmith wrote:
> On 2016/04/26 14:28:34, Bryan McQuade wrote:
> > LGTM. Thanks for the cleanup!
> > 
> > Adding rdsmith as FYI since this change paves the way for removing some
> > additional code that's only currently needed by ResourceScheduler. Randy,
can we
> > remove the willInsertBody code as part of your work to remove
ResourceScheduler?
> 
> I don't think so, but I don't know the blink code well enough to be sure. 
Yes, ResourceScheduler is the only current consumer of it, but the information
(when we hit the body tag) is still needed in the world where ResourceScheduler
doesn't exist, at least while we're keeping the basic prioritization and
ordering scheme we currently have with web page parsing.  My plan is to have
that information encoded in a different way; specifically, my plan is to have
all requests that would be throttled to "just one request active until all
renderer blocking requests dispatched before <body> tag complete" put in a new
priority bucket that's more restrictive than IDLE (THROTTLED) and have the
renderer re-prioritize all those requests when it passes the the somewhat
complicated point described in my quotes above :-}.  But that means that the
renderer still needs to know when the body tag is hit.  It may have other ways
to figure that out (depending on where I eventually put in the new renderer
code) but it may not.  I'll try to keep the possibility of removing
willInsertBody in mind as I put in the new code.
> 
> See
https://docs.google.com/document/d/1p-XGz-BFFehs5j2olbkDkjrXzxzT3n45PVcLSZzOV...
for more details (for folks other than Bryan; he's already seen that doc).

Note that WillInsertBody is going to go away, we could possibly keep something
very specific to the scheduler like ResourceLoadingShouldStartDoingX() or
something, but a signal about the body is bad.

[esprehn, 2016-04-26 18:31:19]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2016-04-26 18:31:52]

Description was changed from

==========
Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

This patch doesn't change any behavior and
only moves the logic.

BUG=521692
==========

to

==========
Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

This patch doesn't change any behavior and only moves the logic.

BUG=521692
TEST=DocumentLoadingRenderingTest.*
==========

[commit-bot: I haz the power, 2016-04-26 18:32:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922793002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922793002/1

[commit-bot: I haz the power, 2016-04-26 18:57:12]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-26 18:59:30]

Description was changed from

==========
Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

This patch doesn't change any behavior and only moves the logic.

BUG=521692
TEST=DocumentLoadingRenderingTest.*
==========

to

==========
Move deferred commit logic from WebViewImpl to Document.

This is more natural here, and will make it easy to add a bit that
tracks if a document would be in "deferred commits" mode even if its
not the main document. Then we can use that bit to trigger pipeline
throttling for iframes when they're loading avoiding FOUC in a sane
way.

This patch doesn't change any behavior and only moves the logic.

BUG=521692
TEST=DocumentLoadingRenderingTest.*

Committed: https://crrev.com/e10293b755e7e28e5f82adeebee7a2f1b630a6a6
Cr-Commit-Position: refs/heads/master@{#389829}
==========

[commit-bot: I haz the power, 2016-04-26 18:59:32]

Patchset 1 (id:??) landed as
https://crrev.com/e10293b755e7e28e5f82adeebee7a2f1b630a6a6
Cr-Commit-Position: refs/heads/master@{#389829}