Bug 33884 - [Chromium] spurious WebViewClient::didStopLoading if changing location.hash while a subframe is still loading
Summary: [Chromium] spurious WebViewClient::didStopLoading if changing location.hash w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-19 23:16 PST by Darin Fisher (:fishd, Google)
Modified: 2010-03-06 07:14 PST (History)
2 users (show)

See Also:


Attachments
v1 patch (5.91 KB, patch)
2010-01-21 15:28 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
v2 patch (fixes style issue) (5.90 KB, patch)
2010-01-21 17:11 PST, Darin Fisher (:fishd, Google)
eric: review-
Details | Formatted Diff | Diff
v1 patch (1.48 KB, patch)
2010-03-05 14:07 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2010-01-19 23:16:04 PST
[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.
Comment 1 Darin Fisher (:fishd, Google) 2010-01-21 15:28:49 PST
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.
Comment 2 WebKit Review Bot 2010-01-21 15:31:58 PST
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.
Comment 3 Darin Fisher (:fishd, Google) 2010-01-21 17:11:06 PST
Created attachment 47161 [details]
v2 patch (fixes style issue)
Comment 4 Darin Fisher (:fishd, Google) 2010-01-22 10:50:24 PST
Landed as http://trac.webkit.org/changeset/53705
Comment 5 Darin Fisher (:fishd, Google) 2010-01-26 12:50:31 PST
This change was reverted (http://trac.webkit.org/changeset/53747) due to Chromium test failures.
Comment 6 Darin Fisher (:fishd, Google) 2010-01-26 12:51:46 PST
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.
Comment 7 Eric Seidel (no email) 2010-02-02 14:13:08 PST
Comment on attachment 47161 [details]
v2 patch (fixes style issue)

Marking this patch r- since it was reverted.
Comment 8 Darin Fisher (:fishd, Google) 2010-03-05 14:07:05 PST
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.
Comment 9 WebKit Commit Bot 2010-03-06 07:14:04 PST
Comment on attachment 50118 [details]
v1 patch

Clearing flags on attachment: 50118

Committed r55625: <http://trac.webkit.org/changeset/55625>
Comment 10 WebKit Commit Bot 2010-03-06 07:14:09 PST
All reviewed patches have been landed.  Closing bug.