PR:206372.
Created attachment 178377 [details] Patch
Comment on attachment 178377 [details] Patch Patch looks good overall. Are you sure it is not causing extra layouts for instance on ltr?
Comment on attachment 178377 [details] 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?
Comment on attachment 178377 [details] 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.
Comment on attachment 178377 [details] 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.
(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.
Comment on attachment 178377 [details] 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.
(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?
(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).
(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.
Comment on attachment 178377 [details] Patch This seems fine, after discussing it on irc.
Committed r137049: <http://trac.webkit.org/changeset/137049>
Comment on attachment 178377 [details] 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.
(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.
(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.