Move most of FrameLoader::CheckCompleted() to Document

Move most of FrameLoader::CheckCompleted() to Document

Split out the the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

BUG=

Review-Url: https://codereview.chromium.org/2809733003
Cr-Commit-Position: refs/heads/master@{#466470}
Committed: https://chromium.googlesource.com/chromium/src/+/37861ef99134d28395b1891a25edb0b750a7011e

[Nate Chapin, 2017-04-10 22:33:06]

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

[commit-bot: I haz the power, 2017-04-10 22:34:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-04-10 23:58:20]

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

[commit-bot: I haz the power, 2017-04-10 23:58:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 01:13:06]

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

[commit-bot: I haz the power, 2017-04-11 01:13:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-04-11 20:52:29]

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

[commit-bot: I haz the power, 2017-04-11 20:53:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 23:13:45]

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

[commit-bot: I haz the power, 2017-04-11 23:13:46]

Dry run: 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_androi...)

[Nate Chapin, 2017-04-11 23:22:47]

Description was changed from

==========
processingLoadEvent() as part of shouldComplete(), cleanup shouldComplete()


Move most of checkCompleted() to Document


Simplify checkCompleted

BUG=
==========

to

==========
Move most of FrameLoader::CheckCompleted() to Document

Split out the the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

BUG=
==========

[Nate Chapin, 2017-04-11 23:25:49]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org, yhirano@chromium.org

[Nate Chapin, 2017-04-11 23:25:50]

It's everyone's favorite thing: yet another crazy core/loader/ refactor!

I tried to provide a thorough description of the rationale for all changes, but
I'm sure I missed a bunch.

I'm still stumped as to *why* the LayoutTest output changed, but I think it's
unrelated to what the test is actually covering, so...meh.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2790: SuppressLoadEvent();
Instead of the early-exit for a nullptr parser_ in Document::ImplicitClose().

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2795: RemoveChildren();
This can trigger events, which can in turn call document.open()/document.close()
and cause reentrant parser state confusion. Just DetachParser() after.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2957: SetReadyState(kComplete);
There are cases where we want to SetReadyState(kComplete) while
ProcessingLoadEvent() (wtf?)

Instead of making parts of CheckCompleted() re-entrant so that we can enable
this insanity, just call SetReadyState(kComplete) here, unless of course,
parser_->Finish() triggered an event that re-open()ed the Document.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3044:
View()->HandleLoadCompleted();
From FrameLoader::CheckCompleted(). It just needs to happen sometime around the
load event, not very timing-sensitive.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3048: if (!frame)
Vacuously true! You can't have any descendants if you're already nullptr!

This cases needs to be handled in ShouldComplete() so that detached Documents
can fire onload, and doing it here means ShouldComplete() is all && clauses, no
paren-nesting required.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:642:
document->open();
This is needed because Document::CancelParsing() now calls SuppressLoadEvent(),
which causes Document::LoadEventFinished() to return true.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:399:
frame_->GetDocument()->CheckCompleted();
FrameLoader::FinishedParsing is only called from Document::FinishedParsing, so
ideally this clause would be in Document. However, it does need to run before
ProcessFragment, which is FrameLoader-private and would ideally stay that way.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:425: if
(!document_loader_->SentDidFinishLoad() || HasProvisionalNavigation())
So now:
* Failing a navigation, whether before or after it commits, happens via
DocumentLoader::LoadFailed()
* A successful navigation always goes through Document::CheckCompleted()

Those are the only two callers of DidFinishNavigation. This function just checks
whether both provisional and commiteed are done, and fires "I'm completely done"
messages as needed.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:428: if
(frame_->IsLoading()) {
It's a shame we need to handle the case where DidFinishNavigation() is called
when the Frame is already marked as not loading, but enough tests broke when I
tried to DCHECK(frame_->IsLoading()) that I figure that's a potential followup.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:511:
frame_->GetDocument()->SuppressLoadEvent();
Instead of the early-exit in Document::ImplicitClose().

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:507: if
(!GetDocument().Parsing() && !GetDocument().ProcessingLoadEvent() &&
If LoadEventFinished() is true, ProcessingLoadEvent() must be false, since they
are driven by the same enum. I think this logic predates the enum.

[yhirano, 2017-04-12 12:16:16]

Sorry, I didn't have enough time today, and I'll be OOO tomorrow.

[Nate Chapin, 2017-04-12 16:59:23]

japhet@chromium.org changed reviewers:
+ hiroshige@chromium.org

[yhirano, 2017-04-14 10:33:36]

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3044:
View()->HandleLoadCompleted();
On 2017/04/11 23:25:50, Nate Chapin wrote:
> From FrameLoader::CheckCompleted(). It just needs to happen sometime around
the
> load event, not very timing-sensitive.

Hmm, there are cases where this line is not executed.
 - LoadEventStillNeeded() may return false in CheckCompleted()
 - There are two early returns.

Could you give me some explanation why it's OK?

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6023: if
(!load_event_delay_count_ && GetFrame())
Is checking GetFrame() needed here?

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:434:
loader.DidFinishNavigation();
Can you tell me why FrameLoader::DidFinishNavigation is called instead of
Document::CheckCompleted here?

[Nate Chapin, 2017-04-18 21:55:25]

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

[commit-bot: I haz the power, 2017-04-18 21:55:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-04-18 21:56:39]

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3044:
View()->HandleLoadCompleted();
On 2017/04/14 10:33:35, yhirano (slow) wrote:
> On 2017/04/11 23:25:50, Nate Chapin wrote:
> > From FrameLoader::CheckCompleted(). It just needs to happen sometime around
> the
> > load event, not very timing-sensitive.
> 
> Hmm, there are cases where this line is not executed.
>  - LoadEventStillNeeded() may return false in CheckCompleted()
>  - There are two early returns.
> 
> Could you give me some explanation why it's OK?

I'm not absolutely sure it's OK (though all the tests did pass), and it harms
nothing to return it to CheckCompleted(), so I'll do that.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6023: if
(!load_event_delay_count_ && GetFrame())
On 2017/04/14 10:33:35, yhirano (slow) wrote:
> Is checking GetFrame() needed here?

This is needed to ensure the DCHECK(parser_) isn't triggered in CheckCompleted()
in the case where a Document *never* has a parser (cloneNode seems to be the
main way this can happen).

Instead of checking GetFrame() here, this patch now sets load_event_progress_ to
kLoadEventCompleted in the constructor, and moves resetting the progress to
kLoadEventNotRun to implicitOpen() instead of open(), so that it runs for
document.open() as well as parser creation during DocumentWriter initialization.

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2809733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:434:
loader.DidFinishNavigation();
On 2017/04/14 10:33:35, yhirano wrote:
> Can you tell me why FrameLoader::DidFinishNavigation is called instead of
> Document::CheckCompleted here?

Because all the tests pass? :D

For provisional navigations, we should just call DidFinishNavigation(), becuase
a navigation definitely finished, and that's independent from whether the
committed document is still loading.

For committed navigations, it's a bit trickier. If this is a cancellation (by
far the most common reason for LoadFailed() after commit), then we should be
more or less guaranteed that the document was made done by some other code path
(e.g., parsing completing), but it's not 100% certain. And if the load somehow
failed *during* parsing, after some subresource loads had started...yeah. There
are a lot of cases, and our test coverage problem isn't complete, so I should
err on the side of caution.

Changed to:
* Use a switch
* For the provisional case, have FrameLoder::DetachProvisionalDocumentLoader()
handle calling DidFinishNavigation()
* For the committed case, call checkCompleted(). This still messes with the
ordering of kSentDidFinishLoad getting set relative to onload, but that's a much
larger change.
* For the case where this is called and we're already in the
kSentDidFinishLoad...call DidFinishNavigation() again. I don't know why it's
needed, but tests fail without it. Added a TODO about that.

[commit-bot: I haz the power, 2017-04-18 21:59:57]

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

[commit-bot: I haz the power, 2017-04-18 21:59:58]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nate Chapin, 2017-04-18 22:10:48]

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

[commit-bot: I haz the power, 2017-04-18 22:11:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 23:12:44]

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

[commit-bot: I haz the power, 2017-04-18 23:12:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yhirano, 2017-04-19 16:13:49]

It looks DocumentTest.ValidationMessageCleanup is faling.

[Nate Chapin, 2017-04-19 17:43:14]

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

[Nate Chapin, 2017-04-19 17:43:21]

On 2017/04/19 16:13:49, yhirano wrote:
> It looks DocumentTest.ValidationMessageCleanup is faling.

Fixed.

[commit-bot: I haz the power, 2017-04-19 17:43:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 18:53:05]

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

[commit-bot: I haz the power, 2017-04-19 18:53:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2017-04-19 19:38:50]

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

[commit-bot: I haz the power, 2017-04-19 19:39:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 21:35:03]

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

[commit-bot: I haz the power, 2017-04-19 21:35:04]

Dry run: 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_androi...)

[yhirano, 2017-04-20 12:30:45]

lgtm

[Nate Chapin, 2017-04-21 19:23:58]

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

[commit-bot: I haz the power, 2017-04-21 19:24:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-04-21 20:33:34]

japhet@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Nate Chapin, 2017-04-21 20:33:35]

Hi dfalcantara, would you mind reviewing the 1-line android test change in this
CL?

This is a blink-side change that, among other things, subtly changes load
finish/fail messages. Specifically, we report loads as finishing successfully
where we would have failed (specifically, cancelled) them before.

For the test in question, it's the replacement of an empty page with an error
page. Previously, this was modeled as a failure-due-to-cancellation followed by
a successful load of the error page. With this change, it's 2 successful loads.
This causes SearchGeolocationDisclosureInfoBarTest#testInfoBarAppears to fail.
It is (unnecessarily, I think) loading a page that triggers the error page
replacement, and expects exactly one finish per requested load. It got away with
this because it was ignoring the failure for the requested page, then continuing
when it received the successful finish notification for the error page. I've
resolved this by having it request a non-empty page. Does that seem reasonable?

[gone, 2017-04-21 20:35:20]

lgtm

seems reasonable, yeah.

[Nate Chapin, 2017-04-21 20:39:04]

japhet@chromium.org changed reviewers:
+ boliu@chromium.org

[Nate Chapin, 2017-04-21 20:39:05]

Hi boliu,

I just wanted a quick sanity check that this is unlikely to break webview compat
by subtly changing when we send fail vs. success load completion messages. See
https://codereview.chromium.org/2809733003/#msg41 for the full explanation.

Does this seem OK to you?

[boliu, 2017-04-21 21:41:40]

On 2017/04/21 20:39:05, Nate Chapin wrote:
> Hi boliu,
> 
> I just wanted a quick sanity check that this is unlikely to break webview
compat
> by subtly changing when we send fail vs. success load completion messages. See
> https://codereview.chromium.org/2809733003/#msg41 for the full explanation.
> 
> Does this seem OK to you?

This is specifically when user clicks stop, right? I just tried the equivalent
in webview, and stopping a load without this change already triggers webview's
onPageFinished callback (which just indicates loads is done, not necessarily
that it succeeded). So I think this should be ok?

[Nate Chapin, 2017-04-21 21:50:30]

On 2017/04/21 21:41:40, boliu wrote:
> On 2017/04/21 20:39:05, Nate Chapin wrote:
> > Hi boliu,
> > 
> > I just wanted a quick sanity check that this is unlikely to break webview
> compat
> > by subtly changing when we send fail vs. success load completion messages.
See
> > https://codereview.chromium.org/2809733003/#msg41 for the full explanation.
> > 
> > Does this seem OK to you?
> 
> This is specifically when user clicks stop, right? I just tried the equivalent
> in webview, and stopping a load without this change already triggers webview's
> onPageFinished callback (which just indicates loads is done, not necessarily
> that it succeeded). So I think this should be ok?

This has to do with onPageLoadFinished/onPageLoadFailed (which map to
didFinishLoad/didFailLoad on WebContentsObserver in content/public/), which are
subtly different than onPageFinished (which maps to didStopLoading on
WebContentsObserver in content/public/). And the only case that should change
with this CL is when a navigation "fails" due to an empty document getting
replaced with an error page (e.g., an HTTP 404 with an empty body getting
replaced with a generic chrome-provided error page).

[boliu, 2017-04-21 21:59:39]

On 2017/04/21 21:50:30, Nate Chapin wrote:
> On 2017/04/21 21:41:40, boliu wrote:
> > On 2017/04/21 20:39:05, Nate Chapin wrote:
> > > Hi boliu,
> > > 
> > > I just wanted a quick sanity check that this is unlikely to break webview
> > compat
> > > by subtly changing when we send fail vs. success load completion messages.
> See
> > > https://codereview.chromium.org/2809733003/#msg41 for the full
explanation.
> > > 
> > > Does this seem OK to you?
> > 
> > This is specifically when user clicks stop, right? I just tried the
equivalent
> > in webview, and stopping a load without this change already triggers
webview's
> > onPageFinished callback (which just indicates loads is done, not necessarily
> > that it succeeded). So I think this should be ok?
> 
> This has to do with onPageLoadFinished/onPageLoadFailed (which map to
> didFinishLoad/didFailLoad on WebContentsObserver in content/public/), which
are
> subtly different than onPageFinished (which maps to didStopLoading on
> WebContentsObserver in content/public/). And the only case that should change
> with this CL is when a navigation "fails" due to an empty document getting
> replaced with an error page (e.g., an HTTP 404 with an empty body getting
> replaced with a generic chrome-provided error page).

AwWebContentsObserver also maps didFailLoad with ERR_ABORTED to onPageFinished
as well. So sounds like blink is just doing the same thing, and possibly that
block in AwWebContentsObserver can be removed.

[commit-bot: I haz the power, 2017-04-21 22:00:29]

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

[commit-bot: I haz the power, 2017-04-21 22:00:30]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-04-21 22:13:04]

On 2017/04/21 21:59:39, boliu wrote:
> On 2017/04/21 21:50:30, Nate Chapin wrote:
> > On 2017/04/21 21:41:40, boliu wrote:
> > > On 2017/04/21 20:39:05, Nate Chapin wrote:
> > > > Hi boliu,
> > > > 
> > > > I just wanted a quick sanity check that this is unlikely to break
webview
> > > compat
> > > > by subtly changing when we send fail vs. success load completion
messages.
> > See
> > > > https://codereview.chromium.org/2809733003/#msg41 for the full
> explanation.
> > > > 
> > > > Does this seem OK to you?
> > > 
> > > This is specifically when user clicks stop, right? I just tried the
> equivalent
> > > in webview, and stopping a load without this change already triggers
> webview's
> > > onPageFinished callback (which just indicates loads is done, not
necessarily
> > > that it succeeded). So I think this should be ok?
> > 
> > This has to do with onPageLoadFinished/onPageLoadFailed (which map to
> > didFinishLoad/didFailLoad on WebContentsObserver in content/public/), which
> are
> > subtly different than onPageFinished (which maps to didStopLoading on
> > WebContentsObserver in content/public/). And the only case that should
change
> > with this CL is when a navigation "fails" due to an empty document getting
> > replaced with an error page (e.g., an HTTP 404 with an empty body getting
> > replaced with a generic chrome-provided error page).
> 
> AwWebContentsObserver also maps didFailLoad with ERR_ABORTED to onPageFinished
> as well. So sounds like blink is just doing the same thing, and possibly that
> block in AwWebContentsObserver can be removed.

Did you mean onPageFinished here? This patch has no direct impact on
onPageFinished (or its equivalent in blink), it only changes onPageLoadFinished
and onPageLoadFailed behavior.

[Nate Chapin, 2017-04-21 22:19:32]

On 2017/04/21 22:13:04, Nate Chapin wrote:
> On 2017/04/21 21:59:39, boliu wrote:
> > On 2017/04/21 21:50:30, Nate Chapin wrote:
> > > On 2017/04/21 21:41:40, boliu wrote:
> > > > On 2017/04/21 20:39:05, Nate Chapin wrote:
> > > > > Hi boliu,
> > > > > 
> > > > > I just wanted a quick sanity check that this is unlikely to break
> webview
> > > > compat
> > > > > by subtly changing when we send fail vs. success load completion
> messages.
> > > See
> > > > > https://codereview.chromium.org/2809733003/#msg41 for the full
> > explanation.
> > > > > 
> > > > > Does this seem OK to you?
> > > > 
> > > > This is specifically when user clicks stop, right? I just tried the
> > equivalent
> > > > in webview, and stopping a load without this change already triggers
> > webview's
> > > > onPageFinished callback (which just indicates loads is done, not
> necessarily
> > > > that it succeeded). So I think this should be ok?
> > > 
> > > This has to do with onPageLoadFinished/onPageLoadFailed (which map to
> > > didFinishLoad/didFailLoad on WebContentsObserver in content/public/),
which
> > are
> > > subtly different than onPageFinished (which maps to didStopLoading on
> > > WebContentsObserver in content/public/). And the only case that should
> change
> > > with this CL is when a navigation "fails" due to an empty document getting
> > > replaced with an error page (e.g., an HTTP 404 with an empty body getting
> > > replaced with a generic chrome-provided error page).
> > 
> > AwWebContentsObserver also maps didFailLoad with ERR_ABORTED to
onPageFinished
> > as well. So sounds like blink is just doing the same thing, and possibly
that
> > block in AwWebContentsObserver can be removed.
> 
> Did you mean onPageFinished here? This patch has no direct impact on
> onPageFinished (or its equivalent in blink), it only changes
onPageLoadFinished
> and onPageLoadFailed behavior.

I'm mixing chrome/android/ and android_webview/ terminology here, that's
probably confusing things. Sorry :(

Using the terminology from AwWebContentsObserver.java, this CL will cause
didFinishLoad() to be called instead of didFailLoad() in the unusual case I
described earlier. Before and after, it will be promptly followed by a
didFinishLoad() for the error page, then a didStopLoading().

[boliu, 2017-04-21 22:22:59]

On 2017/04/21 22:13:04, Nate Chapin wrote:
> On 2017/04/21 21:59:39, boliu wrote:
> > On 2017/04/21 21:50:30, Nate Chapin wrote:
> > > On 2017/04/21 21:41:40, boliu wrote:
> > > > On 2017/04/21 20:39:05, Nate Chapin wrote:
> > > > > Hi boliu,
> > > > > 
> > > > > I just wanted a quick sanity check that this is unlikely to break
> webview
> > > > compat
> > > > > by subtly changing when we send fail vs. success load completion
> messages.
> > > See
> > > > > https://codereview.chromium.org/2809733003/#msg41 for the full
> > explanation.
> > > > > 
> > > > > Does this seem OK to you?
> > > > 
> > > > This is specifically when user clicks stop, right? I just tried the
> > equivalent
> > > > in webview, and stopping a load without this change already triggers
> > webview's
> > > > onPageFinished callback (which just indicates loads is done, not
> necessarily
> > > > that it succeeded). So I think this should be ok?
> > > 
> > > This has to do with onPageLoadFinished/onPageLoadFailed (which map to
> > > didFinishLoad/didFailLoad on WebContentsObserver in content/public/),
which
> > are
> > > subtly different than onPageFinished (which maps to didStopLoading on
> > > WebContentsObserver in content/public/). And the only case that should
> change
> > > with this CL is when a navigation "fails" due to an empty document getting
> > > replaced with an error page (e.g., an HTTP 404 with an empty body getting
> > > replaced with a generic chrome-provided error page).
> > 
> > AwWebContentsObserver also maps didFailLoad with ERR_ABORTED to
onPageFinished
> > as well. So sounds like blink is just doing the same thing, and possibly
that
> > block in AwWebContentsObserver can be removed.
> 
> Did you mean onPageFinished here? This patch has no direct impact on
> onPageFinished (or its equivalent in blink), it only changes
onPageLoadFinished
> and onPageLoadFailed behavior.

Ok, qualify everything with blink, WebContentsObserver, or android webview. Apps
can only observer android webview events.

Before this CL, cancel load -> blink onPageLoadFailed -> WebContentsObserver
didFailLoad with ERR_ABORT -> android webview onPageFinished
After this CL, cancel load -> blink onPageLoadFinished -> WebContentsObserver
didFinishLoad. Then afterward WebContentsObserver didStopLoading -> android
webview onPageFinished.

Basically afaict this change should not affect what apps are able to observe, so
should not affect compatibility.

[Nate Chapin, 2017-04-21 22:27:06]

On 2017/04/21 22:22:59, boliu wrote:
> On 2017/04/21 22:13:04, Nate Chapin wrote:
> > On 2017/04/21 21:59:39, boliu wrote:
> > > On 2017/04/21 21:50:30, Nate Chapin wrote:
> > > > On 2017/04/21 21:41:40, boliu wrote:
> > > > > On 2017/04/21 20:39:05, Nate Chapin wrote:
> > > > > > Hi boliu,
> > > > > > 
> > > > > > I just wanted a quick sanity check that this is unlikely to break
> > webview
> > > > > compat
> > > > > > by subtly changing when we send fail vs. success load completion
> > messages.
> > > > See
> > > > > > https://codereview.chromium.org/2809733003/#msg41 for the full
> > > explanation.
> > > > > > 
> > > > > > Does this seem OK to you?
> > > > > 
> > > > > This is specifically when user clicks stop, right? I just tried the
> > > equivalent
> > > > > in webview, and stopping a load without this change already triggers
> > > webview's
> > > > > onPageFinished callback (which just indicates loads is done, not
> > necessarily
> > > > > that it succeeded). So I think this should be ok?
> > > > 
> > > > This has to do with onPageLoadFinished/onPageLoadFailed (which map to
> > > > didFinishLoad/didFailLoad on WebContentsObserver in content/public/),
> which
> > > are
> > > > subtly different than onPageFinished (which maps to didStopLoading on
> > > > WebContentsObserver in content/public/). And the only case that should
> > change
> > > > with this CL is when a navigation "fails" due to an empty document
getting
> > > > replaced with an error page (e.g., an HTTP 404 with an empty body
getting
> > > > replaced with a generic chrome-provided error page).
> > > 
> > > AwWebContentsObserver also maps didFailLoad with ERR_ABORTED to
> onPageFinished
> > > as well. So sounds like blink is just doing the same thing, and possibly
> that
> > > block in AwWebContentsObserver can be removed.
> > 
> > Did you mean onPageFinished here? This patch has no direct impact on
> > onPageFinished (or its equivalent in blink), it only changes
> onPageLoadFinished
> > and onPageLoadFailed behavior.
> 
> Ok, qualify everything with blink, WebContentsObserver, or android webview.
Apps
> can only observer android webview events.
> 
> Before this CL, cancel load -> blink onPageLoadFailed -> WebContentsObserver
> didFailLoad with ERR_ABORT -> android webview onPageFinished
> After this CL, cancel load -> blink onPageLoadFinished -> WebContentsObserver
> didFinishLoad. Then afterward WebContentsObserver didStopLoading -> android
> webview onPageFinished.
> 
> Basically afaict this change should not affect what apps are able to observe,
so
> should not affect compatibility.

Alright, yeah, that makes sense. Thanks!

[Nate Chapin, 2017-04-21 22:27:12]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-04-21 22:27:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2809733003/#ps140001
(title: "Fix failing android test")

[commit-bot: I haz the power, 2017-04-21 22:28:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 22:35:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492813632741370,
"parent_rev": "5ab7c2d09320aa6f4fc6ba0118908ac7e378e77b", "commit_rev":
"37861ef99134d28395b1891a25edb0b750a7011e"}

[commit-bot: I haz the power, 2017-04-21 22:35:35]

Description was changed from

==========
Move most of FrameLoader::CheckCompleted() to Document

Split out the the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

BUG=
==========

to

==========
Move most of FrameLoader::CheckCompleted() to Document

Split out the the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

BUG=

Review-Url: https://codereview.chromium.org/2809733003
Cr-Commit-Position: refs/heads/master@{#466470}
Committed:
https://chromium.googlesource.com/chromium/src/+/37861ef99134d28395b1891a25ed...
==========

[commit-bot: I haz the power, 2017-04-21 22:35:38]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/37861ef99134d28395b1891a25ed...

[nasko, 2017-04-24 18:47:08]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2836103003/ by nasko@chromium.org.

The reason for reverting is: Causes DCHECKs in webkit_tests when running with
--site-per-process, which blocks CQ jobs requiring the linux_site_isolation bot.

Example failure:
https://storage.googleapis.com/chromium-layout-test-archives/Site_Isolation_W...

STDERR: [4584:5888:0421/170621.194:3990474:FATAL:document.cpp(4648)] Check
failed: GetFrame() && GetFrame()->Owner(). 
STDERR: Backtrace:
STDERR: 	base::debug::StackTrace::StackTrace [0x01E9EC87+55]
STDERR: 	base::debug::StackTrace::StackTrace [0x01EC8D5A+10]
STDERR: 	blink::Document::WillChangeFrameOwnerProperties [0x02DB7391+90]
STDERR: 	blink::HTMLFrameElementBase::SetScrollingMode [0x02F4655F+57]
STDERR: 	blink::HTMLFrameElementBase::ParseAttribute [0x02F4638A+490]
STDERR: 	blink::HTMLIFrameElement::ParseAttribute [0x02F3C9AC+604]
STDERR: 	blink::Element::AttributeChanged [0x02DBC38F+111]
STDERR: 	blink::HTMLElement::AttributeChanged [0x02F26590+16]
STDERR: 	blink::Element::DidModifyAttribute [0x02DBDD8C+67]
STDERR: 	blink::Element::setAttribute [0x02DC7B6E+222]
STDERR: 	blink::V8HTMLFrameElement::scrollingAttributeSetterCallback
[0x02BA8131+129]
STDERR: 	v8::internal::FunctionCallbackArguments::Call [0x01698E9D+157]
STDERR: 	v8::internal::Isolate::typed_array_prototype [0x0173761C+1788]
STDERR: 	v8::internal::Builtins::InvokeApiFunction [0x01738931+849]
STDERR: 	v8::internal::Object::SetPropertyWithAccessor [0x01AFA1E8+808]
STDERR: 	v8::internal::Object::SetPropertyInternal [0x01AF9E2C+588]
STDERR: 	v8::internal::Object::SetProperty [0x01AF9A7D+45]
STDERR: 	v8::internal::StoreIC::Store [0x01A4F1DB+491]
STDERR: 	v8::internal::BinaryOpICState::UseInlinedSmiCode [0x01A551EA+10074]
STDERR: 	v8::internal::Runtime_StoreIC_Miss [0x01A4D32C+220].