103889 2012-12-03 06:51:37 -0800 [Qt][WK2] REGRESSION(r135399): It made qmltests::DoubleTapToZoom::test_double_zoomInAndBack() API test fail 2012-12-05 08:29:43 -0800 1 1 1 Unclassified WebKit New Bugs 420+ All All RESOLVED FIXED Qt, QtTriaged P2 Normal --- 70236 102801 1 ossy abecsi abecsi allan.jensen jturcotte kenneth ossy oldest_to_newest 781410 0 ossy 2012-12-03 06:51:37 -0800 FAIL! : qmltests::DoubleTapToZoom::test_double_zoomInAndBack() 'wait for signal contentsScaleCommitted' returned FALSE. () Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(80)] 782573 1 177492 abecsi 2012-12-04 08:35:00 -0800 Created attachment 177492 Patch 783567 2 177492 jturcotte 2012-12-05 02:30:33 -0800 Comment on attachment 177492 Patch I had a chat with Allan and maybe it is wrong that we call clearRelativeZoomState() in PageViewportControllerClientQt::didChangeViewportAttributes. I think that the initial intention was to catch when the controller adjust the content to fit because of it being zoomed out too much. The fact that we are mixing those things together is leading to this kind of bug, so maybe we should put that line somewhere else. 783644 3 abecsi 2012-12-05 04:28:00 -0800 (In reply to comment #2) > (From update of attachment 177492 [details]) > I had a chat with Allan and maybe it is wrong that we call clearRelativeZoomState() in PageViewportControllerClientQt::didChangeViewportAttributes. I think that the initial intention was to catch when the controller adjust the content to fit because of it being zoomed out too much. > The fact that we are mixing those things together is leading to this kind of bug, so maybe we should put that line somewhere else. In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then. We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes. 783647 4 177492 abecsi 2012-12-05 04:32:53 -0800 Comment on attachment 177492 Patch Clearing flags on attachment: 177492 Committed r136668: <http://trac.webkit.org/changeset/136668> 783648 5 abecsi 2012-12-05 04:32:57 -0800 All reviewed patches have been landed. Closing bug. 783693 6 jturcotte 2012-12-05 06:09:45 -0800 (In reply to comment #3) > In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then. > We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes. loadCommitted might be more appropriate then. PageViewportControllerClientQt::setContentsScale also resets the stack for the initial scale, which might be the case you are talking about. 783698 7 abecsi 2012-12-05 06:26:25 -0800 (In reply to comment #6) > (In reply to comment #3) > > In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then. > > We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes. > > loadCommitted might be more appropriate then. > PageViewportControllerClientQt::setContentsScale also resets the stack for the initial scale, which might be the case you are talking about. (In reply to comment #6) > (In reply to comment #3) > > In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then. > > We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes. > > loadCommitted might be more appropriate then. > PageViewportControllerClientQt::setContentsScale also resets the stack for the initial scale, which might be the case you are talking about. Yes, but since we defer setting the scale to didRenderFrame we do not actually use the isInitial flag in setContentsScale, so this information is lost. The problem with load committed is that on complex pages the user might still double-tap _after_ loadCommitted but _before_ pageTransitionViewportReady and the stack would not be reset. So I guess the right spot for explicitly clearing the scale stack would be in pageTransitionViewportReady, or to be more precise: we should reset if we apply the initial scale. I will try to clean this up after the release. 783720 8 jturcotte 2012-12-05 07:23:09 -0800 (In reply to comment #7) Ok I see, that makes sense. pageTransitionViewportReady is still something that has no direct effect on the client, the user is still seeing the old page at this point. We could also try to carry the flag through applyScaleAfterRenderingContents. 783780 9 abecsi 2012-12-05 08:29:43 -0800 (In reply to comment #8) > (In reply to comment #7) > Ok I see, that makes sense. > > pageTransitionViewportReady is still something that has no direct effect on the client, the user is still seeing the old page at this point. We could also try to carry the flag through applyScaleAfterRenderingContents. Yepp, that's what I actually meant by "we should reset if we apply the initial scale." 177492 2012-12-04 08:35:00 -0800 2012-12-05 04:32:53 -0800 Patch bug-103889-20121204173153.patch text/plain 3511 abecsi U3VidmVyc2lvbiBSZXZpc2lvbjogMTM2NTEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggYmZkY2ZiOGE4MGRlZjA3 ZDIwMmVhNjg2YzcyNmJlMTk4OGVlZTk5ZC4uOGE2ZDU0OGU3MGM3ZTM5MDk1OWQwM2ZiNDdkYzUx YzdiYWRlZGE2NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTEyLTA0ICBBbmRy YXMgQmVjc2kgIDxhbmRyYXMuYmVjc2lAZGlnaWEuY29tPgorCisgICAgICAgIFtRdF1bV0syXSBS RUdSRVNTSU9OKHIxMzUzOTkpOiBJdCBtYWRlIHFtbHRlc3RzOjpEb3VibGVUYXBUb1pvb206OnRl c3RfZG91YmxlX3pvb21JbkFuZEJhY2soKSBBUEkgdGVzdCBmYWlsCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDM4ODkKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgY2xpZW50IHNob3VsZCBhbHdheXMg YmUgbm90aWZpZWQgaW4gUGFnZVZpZXdwb3J0Q29udHJvbGxlcjo6ZGlkQ2hhbmdlVmlld3BvcnRB dHRyaWJ1dGVzCisgICAgICAgIGFib3V0IHRoZSBjaGFuZ2VkIGF0dHJpYnV0ZXMgbm90IG9ubHkg aWYgdGhlIG1pbmltdW0gc2NhbGUgY2hhbmdlZC4gVGhpcyBlbnN1cmVzIHRoYXQgdGhlc2UKKyAg ICAgICAgY2hhbmdlcyBhcmUgcHJvcGFnYXRlZCB0byBRV2ViS2l0VGVzdCBhbmQgdGhlIHpvb20g c3RhY2sgb2YgZG91YmxlLXRhcC10by16b29tIGlzIHJlc2V0IGNvcnJlY3RseS4KKyAgICAgICAg QWxzbyBpbmNyZWFzZSBwcmVjaXNpb24gb2Ygc2NhbGUgY29tcGFyaXNvbnMgc2luY2UgdGhlIGN1 cnJlbnQgdmFsdWUgcmVzdWx0ZWQgaW4gZmxha3luZXNzIGluCisgICAgICAgIHNjYWxlIHJlbGF0 ZWQgQVBJIHRlc3RzLgorCisgICAgICAgICogVUlQcm9jZXNzL1BhZ2VWaWV3cG9ydENvbnRyb2xs ZXIuY3BwOgorICAgICAgICAoV2ViS2l0OjpQYWdlVmlld3BvcnRDb250cm9sbGVyOjpkaWRDaGFu Z2VWaWV3cG9ydEF0dHJpYnV0ZXMpOgorICAgICAgICAoV2ViS2l0OjpQYWdlVmlld3BvcnRDb250 cm9sbGVyOjp1cGRhdGVNaW5pbXVtU2NhbGVUb0ZpdCk6CisKIDIwMTItMTItMDQgIFl1bmkgSmVv bmcgIDx5aG5ldC5qdW5nQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtFRkxdW1dLMl0gQWRkIEFQ SXMgdG8gdG9nZ2xlIHBsdWctaW5zIHN1cHBvcnQuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0 Mi9VSVByb2Nlc3MvUGFnZVZpZXdwb3J0Q29udHJvbGxlci5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9V SVByb2Nlc3MvUGFnZVZpZXdwb3J0Q29udHJvbGxlci5jcHAKaW5kZXggZTU2ODY5ZjIzZjRhNWUw ZDljYTU1NzU1YjI1YWFjNzc4ZmNlYmIwYy4uYjI5ZDUzODBiMDVjNzE0ODA0MTkxYjMzNGU4ZTI2 ZmU2MDg3MGYzZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL1BhZ2VWaWV3 cG9ydENvbnRyb2xsZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9QYWdlVmll d3BvcnRDb250cm9sbGVyLmNwcApAQCAtMjMyLDggKzIzMiw4IEBAIHZvaWQgUGFnZVZpZXdwb3J0 Q29udHJvbGxlcjo6ZGlkQ2hhbmdlVmlld3BvcnRBdHRyaWJ1dGVzKGNvbnN0IFdlYkNvcmU6OlZp ZXdwb3J0CiAgICAgaWYgKCFtX2luaXRpYWxseUZpdFRvVmlld3BvcnQpCiAgICAgICAgIFdlYkNv cmU6OnJlc3RyaWN0U2NhbGVGYWN0b3JUb0luaXRpYWxTY2FsZUlmTm90VXNlclNjYWxhYmxlKG1f cmF3QXR0cmlidXRlcyk7CiAKLSAgICBpZiAodXBkYXRlTWluaW11bVNjYWxlVG9GaXQodHJ1ZSkp Ci0gICAgICAgIG1fY2xpZW50LT5kaWRDaGFuZ2VWaWV3cG9ydEF0dHJpYnV0ZXMoKTsKKyAgICB1 cGRhdGVNaW5pbXVtU2NhbGVUb0ZpdCh0cnVlKTsKKyAgICBtX2NsaWVudC0+ZGlkQ2hhbmdlVmll d3BvcnRBdHRyaWJ1dGVzKCk7CiB9CiAKIFdlYkNvcmU6OkZsb2F0U2l6ZSBQYWdlVmlld3BvcnRD b250cm9sbGVyOjp2aWV3cG9ydFNpemVJbkNvbnRlbnRzQ29vcmRpbmF0ZXMoKSBjb25zdApAQCAt MjgwLDE0ICsyODAsMTQgQEAgYm9vbCBQYWdlVmlld3BvcnRDb250cm9sbGVyOjp1cGRhdGVNaW5p bXVtU2NhbGVUb0ZpdChib29sIHVzZXJJbml0aWF0ZWRVcGRhdGUpCiAgICAgaWYgKG1fdmlld3Bv cnRTaXplLmlzRW1wdHkoKSB8fCBtX2NvbnRlbnRzU2l6ZS5pc0VtcHR5KCkpCiAgICAgICAgIHJl dHVybiBmYWxzZTsKIAotICAgIGJvb2wgY3VycmVudGx5U2NhbGVkVG9GaXQgPSBmdXp6eUNvbXBh cmUobV9lZmZlY3RpdmVTY2FsZSwgdG9WaWV3cG9ydFNjYWxlKG1fbWluaW11bVNjYWxlVG9GaXQp LCAwLjAwMSk7CisgICAgYm9vbCBjdXJyZW50bHlTY2FsZWRUb0ZpdCA9IGZ1enp5Q29tcGFyZSht X2VmZmVjdGl2ZVNjYWxlLCB0b1ZpZXdwb3J0U2NhbGUobV9taW5pbXVtU2NhbGVUb0ZpdCksIDAu MDAwMSk7CiAKICAgICBmbG9hdCBtaW5pbXVtU2NhbGUgPSBXZWJDb3JlOjpjb21wdXRlTWluaW11 bVNjYWxlRmFjdG9yRm9yQ29udGVudENvbnRhaW5lZChtX3Jhd0F0dHJpYnV0ZXMsIFdlYkNvcmU6 OnJvdW5kZWRJbnRTaXplKG1fdmlld3BvcnRTaXplKSwgV2ViQ29yZTo6cm91bmRlZEludFNpemUo bV9jb250ZW50c1NpemUpLCBkZXZpY2VQaXhlbFJhdGlvKCkpOwogCiAgICAgaWYgKG1pbmltdW1T Y2FsZSA8PSAwKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKLSAgICBpZiAoIWZ1enp5Q29tcGFy ZShtaW5pbXVtU2NhbGUsIG1fbWluaW11bVNjYWxlVG9GaXQsIDAuMDAxKSkgeworICAgIGlmICgh ZnV6enlDb21wYXJlKG1pbmltdW1TY2FsZSwgbV9taW5pbXVtU2NhbGVUb0ZpdCwgMC4wMDAxKSkg ewogICAgICAgICBtX21pbmltdW1TY2FsZVRvRml0ID0gbWluaW11bVNjYWxlOwogCiAgICAgICAg IGlmICghaGFzU3VzcGVuZGVkQ29udGVudCgpKSB7CkBAIC0yOTYsNyArMjk2LDcgQEAgYm9vbCBQ YWdlVmlld3BvcnRDb250cm9sbGVyOjp1cGRhdGVNaW5pbXVtU2NhbGVUb0ZpdChib29sIHVzZXJJ bml0aWF0ZWRVcGRhdGUpCiAgICAgICAgICAgICBlbHNlIHsKICAgICAgICAgICAgICAgICAvLyBF bnN1cmUgdGhlIGVmZmVjdGl2ZSBzY2FsZSBzdGF5cyB3aXRoaW4gYm91bmRzLgogICAgICAgICAg ICAgICAgIGZsb2F0IGJvdW5kZWRTY2FsZSA9IGlubmVyQm91bmRlZFZpZXdwb3J0U2NhbGUobV9l ZmZlY3RpdmVTY2FsZSk7Ci0gICAgICAgICAgICAgICAgaWYgKCFmdXp6eUNvbXBhcmUoYm91bmRl ZFNjYWxlLCBtX2VmZmVjdGl2ZVNjYWxlLCAwLjAwMSkpCisgICAgICAgICAgICAgICAgaWYgKCFm dXp6eUNvbXBhcmUoYm91bmRlZFNjYWxlLCBtX2VmZmVjdGl2ZVNjYWxlLCAwLjAwMDEpKQogICAg ICAgICAgICAgICAgICAgICBhcHBseVNjYWxlQWZ0ZXJSZW5kZXJpbmdDb250ZW50cyhib3VuZGVk U2NhbGUpOwogICAgICAgICAgICAgfQogICAgICAgICB9Cg==