Bug 33884 – [Chromium] spurious WebViewClient::didStopLoading if changing location.hash while a subframe is still loading

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Fisher (:fishd, Google)]

[Chromium] spurious WebViewClient::didStopLoading if changing location.hash while a subframe is still loading

The code in dispatchDidChangeLocationWithinPage needs to check more than just the isLoading state of the main frame.  It needs to also check if any subframes are loading.

[Darin Fisher (:fishd, Google)]

Created attachment 47152 [details]
v1 patch

There isn't a mechanism to verify WebViewClient::did{Start,Stop}Loading notifications using DRT.  I plan to write a test for this in the Chromium repository, leveraging the TestShellTests framework.

[WebKit Review Bot]

Attachment 47152 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1681:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Created attachment 47161 [details]
v2 patch (fixes style issue)

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/53705

[Darin Fisher (:fishd, Google)]

This change was reverted (http://trac.webkit.org/changeset/53747) due to Chromium test failures.

[Darin Fisher (:fishd, Google)]

It turns out that we sometimes get a didStartLoading before didStopLoading.  This happens when loading error pages.  It may also be possible for script to trigger this case, but I haven't studied this fully.

At any rate, I think this means that I need a less aggressive solution.

[Eric Seidel (no email)]

Comment on attachment 47161 [details]
v2 patch (fixes style issue)

Marking this patch r- since it was reverted.

[Darin Fisher (:fishd, Google)]

Created attachment 50118 [details]
v1 patch

This is a less aggressive solution.  Just check to see if any subframes are
also loading.  isLoadingInAPISense checks this frame as well as any of its
subframes, which is exactly what we want.

[WebKit Commit Bot]

Comment on attachment 50118 [details]
v1 patch

Clearing flags on attachment: 50118

Committed r55625: <http://trac.webkit.org/changeset/55625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.