[chromium] Apply viewport tag initial-scale only once

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] Apply viewport tag initial-scale only once

[Alexandre Elias]

Created attachment 135194 [details]
Patch

[Fady Samuel]

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?

[Alexandre Elias]

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.

[Darin Fisher (:fishd, Google)]

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?

[Alexandre Elias]

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.

[Alexandre Elias]

Created attachment 135234 [details]
Patch

[Alexandre Elias]

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.

[Alexandre Elias]

Created attachment 135238 [details]
Patch

[Alexandre Elias]

Fixed copy-paste mistake.

[Alexandre Elias]

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

[Adam Barth]

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.)

[Alexandre Elias]

(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.

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

(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 :-)

[Tom Zakrajsek]

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?

[Kenneth Rohde Christiansen]

Comment on attachment 135238 [details]
Patch

Fine with me

[WebKit Review Bot]

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

[Adam Barth]

@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.

[Alexandre Elias]

Created attachment 142438 [details]
Patch

[Alexandre Elias]

Resolved conflict and added new viewportEnabled setting to unit test.

[WebKit Review Bot]

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

[Adam Barth]

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

[Adam Barth]

Created attachment 143427 [details]
Patch for landing

[Alexandre Elias]

Looks good, thanks!

[WebKit Review Bot]

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.

[Adam Barth]

Created attachment 143433 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143433 [details]
Patch for landing

Clearing flags on attachment: 143433

Committed r118121: <http://trac.webkit.org/changeset/118121>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.