Bug 82949

Created attachment 135194 [details] Patch

This looks good to me. Do we know that isNewNavigation is only called on the first commit? Are there any other cases where it might get called?

Yes. The only time isNewNavigation will be set is when m_observedNewNavigation is true, and grepping shows that this is only set by BackForwardListChromium::addItem(). The comment there says: // If WebCore adds a new HistoryItem, it means this is a new navigation (ie, // not a reload or back/forward). m_webView->observeNewNavigation(); This matches the behavior we want.

Comment on attachment 135194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135194&action=review > Source/WebKit/chromium/ChangeLog:9 > + initial-scale.  We need to call dispatchViewportPropertiesDidChange() funny character before "We"? > Source/WebKit/chromium/ChangeLog:15 > + isPageScaleFactorIsSet in didCommitLoad.  We only want to clear it on again, funny character before "We" > Source/WebKit/chromium/ChangeLog:17 > + the user zooms in before the load is complete. i'm not sure I completely understand the bug you are fixing. what is your test case?

It's for when a user does a pinch or double-tap zoom while a slow-loading site hasn't finished loading. I'll try and write a test case in WebFrameTest, as it's too racey to be tested reliably in DRT.

Created attachment 135234 [details] Patch

OK, I wrote a test in WebFrameImpl and confirms it reliably fails if this patch is not applied. It's an extension of the test I originally wrote for http://webk.it/80554 so to avoid conflicts I moved the whole test into this patch.

Created attachment 135238 [details] Patch

Fixed copy-paste mistake.

Ping to review this CL and http://webk.it/80554 , as I believe they should be ready to submit now.

Comment on attachment 135238 [details] Patch I'm happy to R+ this given that Fady is happy and that you've added a test case. Thanks for the patch. (If I'm correct, we'll need to wait for Bug 80554 to land before we actually commit this change.)

(In reply to comment #11) > (From update of attachment 135238 [details]) > I'm happy to R+ this given that Fady is happy and that you've added a test case. Thanks for the patch. (If I'm correct, we'll need to wait for Bug 80554 to land before we actually commit this change.) Right, thanks for taking a look. Feel free to r+ cq+ Bug 80554 first, then we can commit this one.

I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case. Zalan it might make sense to write a autotest similar to the one apparently asserting for Chromium. That should be possible using my new UI side testing framework.

(In reply to comment #13) > I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case. > > Zalan it might make sense to write a autotest similar to the one apparently asserting for Chromium. That should be possible using my new UI side testing framework. Sorry, I meant to comment in bug 80554 :-)

Kenneth, I think this can be cq+ now since Bug 80554 is resolved, but wasn't sure because of your comment https://bugs.webkit.org/show_bug.cgi?id=80554#c30. What do you think?

Comment on attachment 135238 [details] Patch Fine with me

Comment on attachment 135238 [details] Patch Rejecting attachment 135238 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ded at 2973 (offset 21 lines). patching file Source/WebKit/chromium/tests/WebFrameTest.cpp Hunk #1 FAILED at 31. Hunk #2 succeeded at 236 with fuzz 2 (offset 85 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/WebFrameTest.cpp.rej patching file Source/WebKit/chromium/tests/data/fixed_layout.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12694899

@Alex: That message from the commit-queue usually means that there's a conflict applying your patch to top-of-tree. Would you be willing to upload a new version that resolves the conflict? Thanks.

Created attachment 142438 [details] Patch

Resolved conflict and added new viewportEnabled setting to unit test.

Comment on attachment 142438 [details] Patch Rejecting attachment 142438 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ded at 3102 (offset 129 lines). patching file Source/WebKit/chromium/tests/WebFrameTest.cpp Hunk #1 FAILED at 31. Hunk #2 succeeded at 243 with fuzz 2 (offset 9 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/WebFrameTest.cpp.rej patching file Source/WebKit/chromium/tests/data/fixed_layout.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12772033

Looks like the conflict was in WebFrameTest.cpp. I'll try to merge it.

Created attachment 143427 [details] Patch for landing

Looks good, thanks!

Attachment 143427 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/tests/WebFrameTest.cpp:50: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 143433 [details] Patch for landing

Comment on attachment 143433 [details] Patch for landing Clearing flags on attachment: 143433 Committed r118121: <http://trac.webkit.org/changeset/118121>

All reviewed patches have been landed. Closing bug.