108252 2013-01-29 16:40:12 -0800 [BlackBerry] Zooming in during page load of non-scalable webpage results in fixed magnification 2013-02-01 12:24:49 -0800 1 1 1 Unclassified WebKit WebKit BlackBerry 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jkjiang jkjiang anilsson jpetsovits manyoso mifenton rwlbuis staikos tonikitoo webkit.review.bot yong.li.webkit oldest_to_newest 819482 0 jkjiang 2013-01-29 16:40:12 -0800 PR: 284828 820446 1 185541 jkjiang 2013-01-30 12:57:44 -0800 Created attachment 185541 Patch 820476 2 185541 yong.li.webkit 2013-01-30 13:23:44 -0800 Comment on attachment 185541 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185541&action=review I don't like this confusing function, and your patch makes it even more confusing. > Source/WebKit/blackberry/Api/WebPage.cpp:1772 > + // We can get float layoutSize(342.284122, 521.448467) and m_maximumScale(2.243750) > + // after computing viewport meta and lay out the contents at IntSize(342, 521). Can we adjust m_maximumScale to fit the integer layout size? > Source/WebKit/blackberry/Api/WebPage.cpp:1774 > + // Therefore, zoomToFitScale(2.245681) could be a bit larger than m_maximumScale > + // based on that contents size and results in maximumScale()!=minimumScale(), which can we make all these floating comparison with something like (fabs(f1-f2) < 0.01)? 820507 3 185541 yong.li.webkit 2013-01-30 13:50:29 -0800 Comment on attachment 185541 Patch how about: 1) isPageScalable = minScale < maxScale (rather than !=) or 2) maxScale() returns std::max(maxScale, zoomToFitScale) 820652 4 185541 jkjiang 2013-01-30 15:04:07 -0800 Comment on attachment 185541 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185541&action=review >> Source/WebKit/blackberry/Api/WebPage.cpp:1772 >> + // after computing viewport meta and lay out the contents at IntSize(342, 521). > > Can we adjust m_maximumScale to fit the integer layout size? That might be best to be resolved right after viewport computing which was the first way I considered. However, looks not good. We have to change result.devicePixelRatio to zoomToFitScale which can not be calculated at that point and also it is not necessary to be std::max(deviceWidth/342, deviceHeight/521). if (result.initialScale > 0) setInitialScale(result.initialScale * result.devicePixelRatio); if (result.minimumScale > 0) setMinimumScale(result.minimumScale * result.devicePixelRatio); if (result.maximumScale > 0) setMaximumScale(result.maximumScale * result.devicePixelRatio); >> Source/WebKit/blackberry/Api/WebPage.cpp:1774 >> + // based on that contents size and results in maximumScale()!=minimumScale(), which > > can we make all these floating comparison with something like (fabs(f1-f2) < 0.01)? This is doable to make things better, but can not fix the issue. 820676 5 jkjiang 2013-01-30 15:23:28 -0800 (In reply to comment #3) > (From update of attachment 185541 [details]) > how about: > 1) isPageScalable = minScale < maxScale (rather than !=) May be it is OK. What if minScale() > maxScale()? > or 2) maxScale() returns std::max(maxScale, zoomToFitScale) Suppose there is another page that m_maxScale:2.4 and zoomToFitScale: 2.5, previously we would return 4 as maximumScaleScale(). If we return std::max(maxScale, zoomToFitScale), we will then return 2.5 as the maximumScaleScale() and the page will be non-zoomable even if the page was supposed to be zoomable, which seems not good. Adding Arvid to the discussion, any opinions on the bug? 820680 6 jkjiang 2013-01-30 15:25:06 -0800 (In reply to comment #5) > (In reply to comment #3) > Adding Arvid to the discussion, any opinions on the bug? Forgot CC. 822068 7 jkjiang 2013-01-31 16:03:50 -0800 (In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 185541 [details] [details]) > > how about: > > 1) isPageScalable = minScale < maxScale (rather than !=) > May be it is OK. What if minScale() > maxScale()? This doesn't fix the problem by testing, and also there are some cases minScale() >= maxScale() although m_minimumScale < m_maximumScale which expects the page to be scalable. 823080 8 186095 jkjiang 2013-02-01 11:55:58 -0800 Created attachment 186095 Patch Updated the patch by taking Yong's suggestion. 823088 9 186095 yong.li.webkit 2013-02-01 11:59:55 -0800 Comment on attachment 186095 Patch thanks. much simpler 823125 10 jkjiang 2013-02-01 12:24:49 -0800 Committed r141626: <http://trac.webkit.org/changeset/141626> 185541 2013-01-30 12:57:44 -0800 2013-02-01 11:55:54 -0800 Patch bug-108252-20130130155429.patch text/plain 2955 jkjiang U3VidmVyc2lvbiBSZXZpc2lvbjogMTQxMjg3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Js YWNrYmVycnkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpp bmRleCA3ZjRiODVjNGEwNDEyNTQyYjMyZmRlMWY5MTkzZTAxMGJhMmFkM2ViLi4wZjA1ZTYzYTY3 MjdjN2ZmODk5ZmIwM2IwZThkMGNiOWExMTNiODRhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2JsYWNrYmVycnkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cKQEAgLTEsMyArMSwyNSBAQAorMjAxMy0wMS0zMCAgSmFja3kgSmlhbmcgIDx6aGFqaWFu Z0ByaW0uY29tPgorCisgICAgICAgIFtCbGFja0JlcnJ5XSBab29taW5nIGluIGR1cmluZyBwYWdl IGxvYWQgb2Ygbm9uLXNjYWxhYmxlIHdlYnBhZ2UgcmVzdWx0cyBpbiBmaXhlZCBtYWduaWZpY2F0 aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDgy NTIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgSW50ZXJu YWxseSByZXZpZXdlZCBieSBKYWtvYiBQZXRzb3ZpdHMuCisKKyAgICAgICAgUFI6IDI4NDgyOAor ICAgICAgICBXZSBnb3QgZmxvYXQgbGF5b3V0U2l6ZSgzNDIuMjg0MTIyLCA1MjEuNDQ4NDY3KSBh bmQKKyAgICAgICAgbV9tYXhpbXVtU2NhbGUoMi4yNDM3NTApIGFmdGVyIGNvbXB1dGluZyB2aWV3 cG9ydCBtZXRhIGJhc2VkIG9uIHRoZQorICAgICAgICBkZXZpY2UgcGl4ZWwgcmF0aW8gYW5kIGxh aWQgb3V0IHRoZSBjb250ZW50cyBhdCBJbnRTaXplKDM0MiwgNTIxKS4KKyAgICAgICAgVGhlcmVm b3JlLCB6b29tVG9GaXRTY2FsZSgyLjI0NTY4MSkgd291bGQgYmUgYSBiaXQgbGFyZ2VyIHRoYW4K KyAgICAgICAgbV9tYXhpbXVtU2NhbGUgYmFzZWQgb24gdGhhdCBjb250ZW50cyBzaXplIGFuZCBy ZXN1bHRlZCBpbgorICAgICAgICBtYXhpbXVtU2NhbGUoKSE9bWluaW11bVNjYWxlKCksIHdoaWNo IG1hZGUgdGhlIG5vbi1zY2FsYWJsZSBwYWdlCisgICAgICAgIHNjYWxhYmxlLgorICAgICAgICBU aGUgcGF0Y2ggYmFzaWNhbGx5IGlnbm9yZXMgdGhlIHJvdW5kaW5nIGVycm9yIGFuZCByZXR1cm5z CisgICAgICAgIHpvb21Ub0ZpdFNjYWxlIGluc3RlYWQuCisKKyAgICAgICAgKiBBcGkvV2ViUGFn ZS5jcHA6CisgICAgICAgIChCbGFja0JlcnJ5OjpXZWJLaXQ6OldlYlBhZ2VQcml2YXRlOjptYXhp bXVtU2NhbGUpOgorCiAyMDEzLTAxLTMwICBFZCBCYWtlciAgPGVkYmFrZXJAcmltLmNvbT4KIAog ICAgICAgICBbQmxhY2tCZXJyeV0gU2NyZWVuc2hvdCBpcyBjbGlwcGVkIHdoZW4gY29udGVudCBp cyBzbWFsbGVyIHRoYW4gdGhlIGRlc2ludGF0aW9uIHNpemUKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVy cnkvQXBpL1dlYlBhZ2UuY3BwCmluZGV4IDQzYzYzMDkyY2IwY2JhYzhmM2M5NjkwODNkNzg2ODgw OGRlODllZjQuLjljNzRhODJhZGZmODNhMzVhMGRhMjhjMDc5MTAxYjY2NDU3YzUyNTIgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAKKysrIGIvU291 cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlLmNwcApAQCAtMTc2MywxMCArMTc2Mywy MiBAQCB2b2lkIFdlYlBhZ2U6OnNldE1heGltdW1TY2FsZShkb3VibGUgbWF4aW11bVNjYWxlKQog CiBkb3VibGUgV2ViUGFnZVByaXZhdGU6Om1heGltdW1TY2FsZSgpIGNvbnN0CiB7Ci0gICAgaWYg KG1fbWF4aW11bVNjYWxlID49IHpvb21Ub0ZpdFNjYWxlKCkgJiYgbV9tYXhpbXVtU2NhbGUgPj0g bV9taW5pbXVtU2NhbGUgJiYgcmVzcGVjdFZpZXdwb3J0KCkpCi0gICAgICAgIHJldHVybiBtX21h eGltdW1TY2FsZTsKLQotICAgIHJldHVybiBoYXNWaXJ0dWFsVmlld3BvcnQoKSA/IHN0ZDo6bWF4 PGRvdWJsZT4oem9vbVRvRml0U2NhbGUoKSwgNC4wKSA6IDQuMDsKKyAgICBkb3VibGUgem9vbVRv Rml0U2NhbGUgPSB0aGlzLT56b29tVG9GaXRTY2FsZSgpOworICAgIGlmIChtX21heGltdW1TY2Fs ZSA+PSBtX21pbmltdW1TY2FsZSAmJiByZXNwZWN0Vmlld3BvcnQoKSkgeworICAgICAgICBpZiAo bV9tYXhpbXVtU2NhbGUgPj0gem9vbVRvRml0U2NhbGUpCisgICAgICAgICAgICByZXR1cm4gbV9t YXhpbXVtU2NhbGU7CisKKyAgICAgICAgLy8gV2UgY2FuIGdldCBmbG9hdCBsYXlvdXRTaXplKDM0 Mi4yODQxMjIsIDUyMS40NDg0NjcpIGFuZCBtX21heGltdW1TY2FsZSgyLjI0Mzc1MCkKKyAgICAg ICAgLy8gYWZ0ZXIgY29tcHV0aW5nIHZpZXdwb3J0IG1ldGEgYW5kIGxheSBvdXQgdGhlIGNvbnRl bnRzIGF0IEludFNpemUoMzQyLCA1MjEpLgorICAgICAgICAvLyBUaGVyZWZvcmUsIHpvb21Ub0Zp dFNjYWxlKDIuMjQ1NjgxKSBjb3VsZCBiZSBhIGJpdCBsYXJnZXIgdGhhbiBtX21heGltdW1TY2Fs ZQorICAgICAgICAvLyBiYXNlZCBvbiB0aGF0IGNvbnRlbnRzIHNpemUgYW5kIHJlc3VsdHMgaW4g bWF4aW11bVNjYWxlKCkhPW1pbmltdW1TY2FsZSgpLCB3aGljaAorICAgICAgICAvLyBjYW4gbWFr ZSBub24tc2NhbGFibGUgcGFnZSBzY2FsYWJsZS4gU28gaWdub3JlIHRoZSByb3VuZGluZyBlcnJv ciBoZXJlIGFuZCBwaWNrIHVwCisgICAgICAgIC8vIHpvb21Ub0ZpdFNjYWxlIGluc3RlYWQuCisg ICAgICAgIGlmICh6b29tVG9GaXRTY2FsZSAtIG1fbWF4aW11bVNjYWxlIDwgMC4wMSkKKyAgICAg ICAgICAgIHJldHVybiB6b29tVG9GaXRTY2FsZTsKKyAgICB9CisKKyAgICByZXR1cm4gaGFzVmly dHVhbFZpZXdwb3J0KCkgPyBzdGQ6Om1heDxkb3VibGU+KHpvb21Ub0ZpdFNjYWxlLCA0LjApIDog NC4wOwogfQogCiBkb3VibGUgV2ViUGFnZTo6bWF4aW11bVNjYWxlKCkgY29uc3QK 186095 2013-02-01 11:55:58 -0800 2013-02-01 11:59:55 -0800 Patch bug-108252-20130201145241.patch text/plain 2275 jkjiang U3VidmVyc2lvbiBSZXZpc2lvbjogMTQxNjE2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Js YWNrYmVycnkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpp bmRleCA4ZTlhNTczN2JkN2MwMGIwMzc3YTM3YTkzYzYzNjEwYTc5NWYwMzk0Li4zYzcyYTE0MmEy YjY5YzAxMGVmNWFlZTc0M2RiNzRmNWRiMzliZTRjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2JsYWNrYmVycnkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cKQEAgLTEsMyArMSwyNCBAQAorMjAxMy0wMi0wMSAgSmFja3kgSmlhbmcgIDx6aGFqaWFu Z0ByaW0uY29tPgorCisgICAgICAgIFtCbGFja0JlcnJ5XSBab29taW5nIGluIGR1cmluZyBwYWdl IGxvYWQgb2Ygbm9uLXNjYWxhYmxlIHdlYnBhZ2UgcmVzdWx0cyBpbiBmaXhlZCBtYWduaWZpY2F0 aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDgy NTIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgSW50ZXJu YWxseSByZXZpZXdlZCBieSBLb25yYWQgUGlhc2Npay4KKworICAgICAgICBQUjogMjg0ODI4Cisg ICAgICAgIFdlIGdvdCBmbG9hdCBsYXlvdXRTaXplKDM0Mi4yODQxMjIsIDUyMS40NDg0NjcpIGFu ZAorICAgICAgICBtX21heGltdW1TY2FsZSgyLjI0Mzc1MCkgYWZ0ZXIgY29tcHV0aW5nIHZpZXdw b3J0IG1ldGEgYmFzZWQgb24gdGhlCisgICAgICAgIGRldmljZSBwaXhlbCByYXRpbyBhbmQgbGFp ZCBvdXQgdGhlIGNvbnRlbnRzIGF0IEludFNpemUoMzQyLCA1MjEpLgorICAgICAgICBUaGVyZWZv cmUsIHpvb21Ub0ZpdFNjYWxlKDIuMjQ1NjgxKSB3b3VsZCBiZSBhIGJpdCBsYXJnZXIgdGhhbgor ICAgICAgICBtX21heGltdW1TY2FsZSBiYXNlZCBvbiB0aGF0IGNvbnRlbnRzIHNpemUgYW5kIHJl c3VsdGVkIGluCisgICAgICAgIG1heGltdW1TY2FsZSgpIT1taW5pbXVtU2NhbGUoKSwgd2hpY2gg bWFkZSB0aGUgbm9uLXNjYWxhYmxlIHBhZ2UKKyAgICAgICAgc2NhbGFibGUuCisgICAgICAgIFJl dHVybiB6b29tVG9GaXRTY2FsZSBmb3IgbWF4aW11bVNjYWxlKCkgaW4gc3VjaCBraW5kIG9mIGNh c2UuCisKKyAgICAgICAgKiBBcGkvV2ViUGFnZS5jcHA6CisgICAgICAgIChCbGFja0JlcnJ5OjpX ZWJLaXQ6OldlYlBhZ2VQcml2YXRlOjptYXhpbXVtU2NhbGUpOgorCiAyMDEzLTAxLTMxICBKYWNr eSBKaWFuZyAgPHpoYWppYW5nQHJpbS5jb20+CiAKICAgICAgICAgW0JsYWNrQmVycnldIEJpbmcg SW1hZ2VzIHZpZXdwb3J0IGNhdXNlcyBsYXlvdXQgImZ1biIKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVy cnkvQXBpL1dlYlBhZ2UuY3BwCmluZGV4IDBkNGEyM2NiZDIwMmFkMTdhOGZmNWU0ZWY5ZmEyZDg2 OGE5YTRiODAuLjFiZjJmMjAxZDJlYjIzZDUwNDNiM2YxNDkxOTk5MGY2ZTc2YTkyNmMgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAKKysrIGIvU291 cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlLmNwcApAQCAtMTc2MywxMCArMTc2Mywx MSBAQCB2b2lkIFdlYlBhZ2U6OnNldE1heGltdW1TY2FsZShkb3VibGUgbWF4aW11bVNjYWxlKQog CiBkb3VibGUgV2ViUGFnZVByaXZhdGU6Om1heGltdW1TY2FsZSgpIGNvbnN0CiB7Ci0gICAgaWYg KG1fbWF4aW11bVNjYWxlID49IHpvb21Ub0ZpdFNjYWxlKCkgJiYgbV9tYXhpbXVtU2NhbGUgPj0g bV9taW5pbXVtU2NhbGUgJiYgcmVzcGVjdFZpZXdwb3J0KCkpCi0gICAgICAgIHJldHVybiBtX21h eGltdW1TY2FsZTsKKyAgICBkb3VibGUgem9vbVRvRml0U2NhbGUgPSB0aGlzLT56b29tVG9GaXRT Y2FsZSgpOworICAgIGlmIChtX21heGltdW1TY2FsZSA+PSBtX21pbmltdW1TY2FsZSAmJiByZXNw ZWN0Vmlld3BvcnQoKSkKKyAgICAgICAgcmV0dXJuIHN0ZDo6bWF4KHpvb21Ub0ZpdFNjYWxlLCBt X21heGltdW1TY2FsZSk7CiAKLSAgICByZXR1cm4gaGFzVmlydHVhbFZpZXdwb3J0KCkgPyBzdGQ6 Om1heDxkb3VibGU+KHpvb21Ub0ZpdFNjYWxlKCksIDQuMCkgOiA0LjA7CisgICAgcmV0dXJuIGhh c1ZpcnR1YWxWaWV3cG9ydCgpID8gc3RkOjptYXg8ZG91YmxlPih6b29tVG9GaXRTY2FsZSwgNC4w KSA6IDQuMDsKIH0KIAogZG91YmxlIFdlYlBhZ2U6Om1heGltdW1TY2FsZSgpIGNvbnN0Cg==