Bug 31838 - Nested start/stop events in dispatchDidChangeLocationWithinPage for pages that do fragment redirect in window.onload
Summary: Nested start/stop events in dispatchDidChangeLocationWithinPage for pages tha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 420+
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-24 10:11 PST by xiyuan
Modified: 2009-12-01 10:44 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch. (1.41 KB, patch)
2009-11-24 10:13 PST, xiyuan
fishd: review-
Details | Formatted Diff | Diff
updated per Darin's comments (2.68 KB, patch)
2009-11-24 12:43 PST, xiyuan
no flags Details | Formatted Diff | Diff
remove scm.py change (2.06 KB, patch)
2009-11-24 12:50 PST, xiyuan
fishd: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
update patch (2.08 KB, patch)
2009-11-30 11:19 PST, xiyuan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description xiyuan 2009-11-24 10:11:08 PST
Start/stop events are generated in dispatchDidChangeLocationWithinPage for WebView delegate. Currently they are generated regardless whether a frame is is loading or not. This causes nested start/stop events being fired when dispatchDidChangeLocationWithinPage happens from a fragment redirect in window.onloader handler.

Attached patch makes the start/stop events only being fired when the page's main frame has stopped loading to avoid such situation.
Comment 1 xiyuan 2009-11-24 10:13:32 PST
Created attachment 43780 [details]
Proposed patch.
Comment 2 Darin Fisher (:fishd, Google) 2009-11-24 12:08:50 PST
Comment on attachment 43780 [details]
Proposed patch.

> Index: third_party/WebKit/WebKit/chromium/src/FrameLoaderClientImpl.cpp
...
> +    // Flag of whether frame loader is completed. Generate didStartLoading and
> +    // didStopLoading only when loader is completed so that we don't fire
> +    // them for fragment redirection that happens in window.onload handler.
> +    // See http://crbug.com/15782

nit: please link to the bugs.webkit.org bug instead.  that bug can have a reference
to the chromium bug if you think it would be helpful.

also, you need to include a modification to WebCore/ChangeLog.  please see the
instructions here: http://webkit.org/coding/contributing.html
Comment 3 xiyuan 2009-11-24 12:43:38 PST
Created attachment 43800 [details]
updated per Darin's comments

- Added ChangeLog using prepare-ChangeLog;
- Update bug link per suggestion;
Comment 4 xiyuan 2009-11-24 12:50:30 PST
Created attachment 43801 [details]
remove scm.py change

Remove my local scm.py change that accidentally gets into the previous diff.
Comment 5 Darin Fisher (:fishd, Google) 2009-11-24 16:48:31 PST
Comment on attachment 43801 [details]
remove scm.py change

R=me, but please remember to set the '?' on the review flag in the future.
Otherwise, reviewers may never notice your patch!
Comment 6 WebKit Commit Bot 2009-11-24 17:53:52 PST
Comment on attachment 43801 [details]
remove scm.py change

Rejecting patch 43801 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 7 xiyuan 2009-11-24 22:02:42 PST
Comment on attachment 43801 [details]
remove scm.py change

Is it because my diff is from git?
Comment 8 Eric Seidel (no email) 2009-11-25 22:43:20 PST
Comment on attachment 43801 [details]
remove scm.py change

I have no idea why the cq failed here.  Let's spin again and see if it fails a second time.  If it does, then I'll try to debug why.
Comment 9 WebKit Commit Bot 2009-11-25 22:54:35 PST
Comment on attachment 43801 [details]
remove scm.py change

Rejecting patch 43801 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 10 xiyuan 2009-11-30 11:19:36 PST
Created attachment 44030 [details]
update patch

Previous patch is rejected by commit-queue bot because my base ChangeLog is too old and the bot failed to patc. I have updated the patch and hopefully it works this time.

Thanks.
Comment 11 Adam Barth 2009-11-30 12:52:24 PST
style-queue ran check-webkit-style on attachment 44030 [details] without any errors.
Comment 12 WebKit Commit Bot 2009-12-01 10:44:04 PST
Comment on attachment 44030 [details]
update patch

Clearing flags on attachment: 44030

Committed r51548: <http://trac.webkit.org/changeset/51548>
Comment 13 WebKit Commit Bot 2009-12-01 10:44:12 PST
All reviewed patches have been landed.  Closing bug.