104451 2012-12-08 12:29:37 -0800 [BlackBerry] Google results page rendering issue with RTL languages like arabic/hebrew 2012-12-09 12:14:43 -0800 1 1 1 Unclassified WebKit WebKit BlackBerry 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jkjiang jkjiang mifenton rwlbuis staikos tonikitoo webkit.review.bot yong.li.webkit oldest_to_newest 786577 0 jkjiang 2012-12-08 12:29:37 -0800 PR:206372. 786602 1 178377 jkjiang 2012-12-08 13:23:04 -0800 Created attachment 178377 Patch 786604 2 178377 rwlbuis 2012-12-08 13:34:35 -0800 Comment on attachment 178377 Patch Patch looks good overall. Are you sure it is not causing extra layouts for instance on ltr? 786605 3 178377 rwlbuis 2012-12-08 13:35:18 -0800 Comment on attachment 178377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:1557 > +void WebPagePrivate::overflowExceedsContentsSize() Should this be called setOverflowExceedsContentsSize? 786606 4 178377 rwlbuis 2012-12-08 13:37:07 -0800 Comment on attachment 178377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:1561 > + if (setViewMode(viewMode())) { Here you are setting m_viewMode to the same value, so I guess you want to do the extra stuff in setViewMode, not just setting m_viewMode. Consider splitting it into two methods in a future patch, one for setting m_viewMode, and one to do the extra stuff, above line just looks weird, like a no-op on first glance. 786609 5 178377 jkjiang 2012-12-08 13:50:58 -0800 Comment on attachment 178377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review >> Source/WebKit/blackberry/Api/WebPage.cpp:1557 >> +void WebPagePrivate::overflowExceedsContentsSize() > > Should this be called setOverflowExceedsContentsSize? I think we are fine as that is callback alike in FrameView::adjustViewSize(), just notify the WebPage through Chrome that we overflowed. >> Source/WebKit/blackberry/Api/WebPage.cpp:1561 >> + if (setViewMode(viewMode())) { > > Here you are setting m_viewMode to the same value, so I guess you want to do the extra stuff in setViewMode, not just setting m_viewMode. Consider splitting it into two methods in a future patch, one for setting m_viewMode, and one to do the extra stuff, above line just looks weird, like a no-op on first glance. Hmm, we actually will recalculate fixedLayoutSize and apply fixedLayoutSize to WebCore in setViewMode. Probably I guess we think when view mode is changed which means fixed layout width should be changed as well. 786611 6 jkjiang 2012-12-08 14:00:51 -0800 (In reply to comment #2) > (From update of attachment 178377 [details]) > Patch looks good overall. Are you sure it is not causing extra layouts for instance on ltr? It will cause an extra layout both on RTL and LTR page, as this is trying to layout larger and get other render Objects updated. 786612 7 178377 rwlbuis 2012-12-08 14:03:29 -0800 Comment on attachment 178377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review > Source/WebKit/blackberry/ChangeLog:12 > + The other renders still stay at the old width unfortunately which Note, renders should be renderers. > Source/WebKit/blackberry/ChangeLog:17 > + DEFAULT_MAX_LAYOUT_WIDTH and update the other renders. Ditto. 786613 8 rwlbuis 2012-12-08 14:04:53 -0800 (In reply to comment #6) > (In reply to comment #2) > > (From update of attachment 178377 [details] [details]) > > Patch looks good overall. Are you sure it is not causing extra layouts for instance on ltr? > It will cause an extra layout both on RTL and LTR page, as this is trying to layout larger and get other render Objects updated. Do you mean extra layout *always*, or for some special condition? Like for any random page visit? 786616 9 jkjiang 2012-12-08 14:10:25 -0800 (In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #2) > > > (From update of attachment 178377 [details] [details] [details]) > > > Patch looks good overall. Are you sure it is not causing extra layouts for instance on ltr? > > It will cause an extra layout both on RTL and LTR page, as this is trying to layout larger and get other render Objects updated. > > Do you mean extra layout *always*, or for some special condition? Like for any random page visit? For the page has the following conditions: 1. overflow > ContentsSize. 2. absoluteVisibleOverflowSize().width() < DEFAULT_MAX_LAYOUT_WIDTH(1024) 3. without viewport , !hasVirtualViewport() If the page (both RTL or LTR) has all the conditions, then we will try to relayout to update the renders as that's possible the issue is the same as Google resultsRTL page(PR:206372). 786618 10 jkjiang 2012-12-08 14:13:34 -0800 (In reply to comment #7) > (From update of attachment 178377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review > > > Source/WebKit/blackberry/ChangeLog:12 > > + The other renders still stay at the old width unfortunately which > > Note, renders should be renderers. > > > Source/WebKit/blackberry/ChangeLog:17 > > + DEFAULT_MAX_LAYOUT_WIDTH and update the other renders. > > Ditto. Right, will update. 786620 11 178377 rwlbuis 2012-12-08 14:20:50 -0800 Comment on attachment 178377 Patch This seems fine, after discussing it on irc. 786630 12 jkjiang 2012-12-08 15:09:29 -0800 Committed r137049: <http://trac.webkit.org/changeset/137049> 786839 13 178377 tonikitoo 2012-12-09 11:51:52 -0800 Comment on attachment 178377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review >>> Source/WebKit/blackberry/Api/WebPage.cpp:1557 >>> +void WebPagePrivate::overflowExceedsContentsSize() >> >> Should this be called setOverflowExceedsContentsSize? > > I think we are fine as that is callback alike in FrameView::adjustViewSize(), just notify the WebPage through Chrome that we overflowed. Correct name should be overflowDidExceedContentsSize. 786843 14 jkjiang 2012-12-09 12:10:53 -0800 (In reply to comment #13) > (From update of attachment 178377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review > > >>> Source/WebKit/blackberry/Api/WebPage.cpp:1557 > >>> +void WebPagePrivate::overflowExceedsContentsSize() > >> > >> Should this be called setOverflowExceedsContentsSize? > > > > I think we are fine as that is callback alike in FrameView::adjustViewSize(), just notify the WebPage through Chrome that we overflowed. > > Correct name should be overflowDidExceedContentsSize. Sounds better to me, we may should clean it up next time if people agree on that. 786844 15 rwlbuis 2012-12-09 12:14:43 -0800 (In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 178377 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=178377&action=review > > > > >>> Source/WebKit/blackberry/Api/WebPage.cpp:1557 > > >>> +void WebPagePrivate::overflowExceedsContentsSize() > > >> > > >> Should this be called setOverflowExceedsContentsSize? > > > > > > I think we are fine as that is callback alike in FrameView::adjustViewSize(), just notify the WebPage through Chrome that we overflowed. > > > > Correct name should be overflowDidExceedContentsSize. > > Sounds better to me, we may should clean it up next time if people agree on that. Yes, sounds good. 178377 2012-12-08 13:23:04 -0800 2012-12-09 11:51:52 -0800 Patch bug-104451-20121208162026.patch text/plain 3037 jkjiang U3VidmVyc2lvbiBSZXZpc2lvbjogMTM3MDQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Js YWNrYmVycnkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpp bmRleCAzOTVmNWFhNjlkYzcwNTFiMTdmOTkxNGYxMGFlY2E4ODQyMTFiMjE1Li5mNTFjYWFiMGNj MGMyNzcyZTdiNmYxZGI5NmMzZTBkOGFhYzhjZjZmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2JsYWNrYmVycnkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cKQEAgLTEsMyArMSwyNyBAQAorMjAxMi0xMi0wOCAgSmFja3kgSmlhbmcgIDx6aGFqaWFu Z0ByaW0uY29tPgorCisgICAgICAgIFtCbGFja0JlcnJ5XSBHb29nbGUgcmVzdWx0cyBwYWdlIHJl bmRlcmluZyBpc3N1ZSB3aXRoIFJUTCBsYW5ndWFnZXMgbGlrZSBhcmFiaWMvaGVicmV3CisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDQ0NTEKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQUjogMjA2MzcyCisg ICAgICAgIE9uIEdvb2dsZSByZXN1bHRzIHBhZ2UsIHdlIGxheW91dCB0aGUgY29udGVudHMgYXQg dGhlIHdpZHRoIG9mIDgzMy4KKyAgICAgICAgSG93ZXZlciwgImFwcGJhciIgYW5kICJ0b3BfbmF2 IiBlbGVtZW50cyB3aGljaCBoYXZlIHdpZHRoIDk4MCBjYXVzZQorICAgICAgICBvdmVyZmxvdyBk dXJpbmcgdGhlIGxhc3QgbGF5b3V0IGFuZCBtYWtlIHRoZSBjb250ZW50cyB3aWR0aCBsYXJnZXIu CisgICAgICAgIFRoZSBvdGhlciByZW5kZXJzIHN0aWxsIHN0YXkgYXQgdGhlIG9sZCB3aWR0aCB1 bmZvcnR1bmF0ZWx5IHdoaWNoCisgICAgICAgIHJlc3VsdHMgaW4gYmxhbmsgYXJlYXMgb24gdGhl IGxlZnQgc2lkZSBvZiB0aGUgR29vZ2xlIHJlc3VsdHMgUlRMIHBhZ2UuCisgICAgICAgIEdpdmUg aXQgYSBjaGFuY2UgdG8gcmVxdWVzdCBhbm90aGVyIGxheW91dCBpZiBvdmVyZmxvdyBleGNlZWRz IHRoZQorICAgICAgICBjb250ZW50cyBzaXplIGFuZCB0aGUgcGFnZSBkb2Vzbid0IGhhdmUgdmly dHVhbCB2aWV3cG9ydCwgYXMgdGhpcworICAgICAgICBsYXlvdXQgd2lsbCBwaWNrIHVwIHRoZSBh YnNvbHV0ZSB2aXNpYmxlIG92ZXJmbG93IHdpZHRoIHdpdGhpbgorICAgICAgICBERUZBVUxUX01B WF9MQVlPVVRfV0lEVEggYW5kIHVwZGF0ZSB0aGUgb3RoZXIgcmVuZGVycy4KKworICAgICAgICAq IEFwaS9XZWJQYWdlLmNwcDoKKyAgICAgICAgKEJsYWNrQmVycnk6OldlYktpdDo6V2ViUGFnZVBy aXZhdGU6Om92ZXJmbG93RXhjZWVkc0NvbnRlbnRzU2l6ZSk6CisgICAgICAgIChXZWJLaXQpOgor ICAgICAgICAqIEFwaS9XZWJQYWdlX3AuaDoKKyAgICAgICAgKFdlYlBhZ2VQcml2YXRlKToKKwog MjAxMi0xMi0wOCAgQ2hyaXMgSHV0dGVuLUN6YXBza2kgIDxjaHV0dGVuQHJpbS5jb20+CiAKICAg ICAgICAgW0JsYWNrQmVycnldIFJlbW92ZSBhYm91dDpjcmVkaXRzCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL1dlYlBhZ2UuY3BwIGIvU291cmNlL1dlYktpdC9ibGFj a2JlcnJ5L0FwaS9XZWJQYWdlLmNwcAppbmRleCA3OTllYjU2ZDkzNzQ2ZjE2YWE5NjM5OGM3OTI4 ZWEzN2JiNWU2OWRkLi40ZDMyOTlkYmY5ZjJlZDAxODcyNjRhMWY4YjMxYzg5YWQ1ZDgyYjNkIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL1dlYlBhZ2UuY3BwCisrKyBi L1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAKQEAgLTE1NTQsNiArMTU1 NCwxNyBAQCB2b2lkIFdlYlBhZ2VQcml2YXRlOjpjb250ZW50c1NpemVDaGFuZ2VkKGNvbnN0IElu dFNpemUmIGNvbnRlbnRzU2l6ZSkKICNlbmRpZgogfQogCit2b2lkIFdlYlBhZ2VQcml2YXRlOjpv dmVyZmxvd0V4Y2VlZHNDb250ZW50c1NpemUoKQoreworICAgIG1fb3ZlcmZsb3dFeGNlZWRzQ29u dGVudHNTaXplID0gdHJ1ZTsKKyAgICBpZiAoYWJzb2x1dGVWaXNpYmxlT3ZlcmZsb3dTaXplKCku d2lkdGgoKSA8IERFRkFVTFRfTUFYX0xBWU9VVF9XSURUSCAmJiAhaGFzVmlydHVhbFZpZXdwb3J0 KCkpIHsKKyAgICAgICAgaWYgKHNldFZpZXdNb2RlKHZpZXdNb2RlKCkpKSB7CisgICAgICAgICAg ICBzZXROZWVkc0xheW91dCgpOworICAgICAgICAgICAgcmVxdWVzdExheW91dElmTmVlZGVkKCk7 CisgICAgICAgIH0KKyAgICB9Cit9CisKIHZvaWQgV2ViUGFnZVByaXZhdGU6OmxheW91dEZpbmlz aGVkKCkKIHsKICAgICBpZiAoIW1fY29udGVudHNTaXplQ2hhbmdlZCAmJiAhbV9vdmVyZmxvd0V4 Y2VlZHNDb250ZW50c1NpemUpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkv QXBpL1dlYlBhZ2VfcC5oIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlX3Au aAppbmRleCBlZGVhN2JjMjY0Y2YwNmQ4NWFlY2QyNDQyMGYwN2ZmMTQ3MjIzNzIwLi5hMzM5Mjc0 YTQ3NWVhMzliYjE3NWQ0MzM3ZDAyZDFlZjBkZmI0NmRlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0L2JsYWNrYmVycnkvQXBpL1dlYlBhZ2VfcC5oCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2ti ZXJyeS9BcGkvV2ViUGFnZV9wLmgKQEAgLTIyMSw3ICsyMjEsNyBAQCBwdWJsaWM6CiAgICAgdm9p ZCBleGl0RnVsbFNjcmVlbkZvckVsZW1lbnQoV2ViQ29yZTo6RWxlbWVudCopOwogI2VuZGlmCiAg ICAgdm9pZCBjb250ZW50c1NpemVDaGFuZ2VkKGNvbnN0IFdlYkNvcmU6OkludFNpemUmKTsKLSAg ICB2b2lkIG92ZXJmbG93RXhjZWVkc0NvbnRlbnRzU2l6ZSgpIHsgbV9vdmVyZmxvd0V4Y2VlZHND b250ZW50c1NpemUgPSB0cnVlOyB9CisgICAgdm9pZCBvdmVyZmxvd0V4Y2VlZHNDb250ZW50c1Np emUoKTsKICAgICB2b2lkIGxheW91dEZpbmlzaGVkKCk7CiAgICAgdm9pZCBzZXROZWVkVG91Y2hF dmVudHMoYm9vbCk7CiAgICAgdm9pZCBub3RpZnlQb3B1cEF1dG9maWxsRGlhbG9nKGNvbnN0IFZl Y3RvcjxTdHJpbmc+Jik7Cg==