Bug 104451 – [BlackBerry] Google results page rendering issue with RTL languages like arabic/hebrew

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jacky Jiang]

PR:206372.

[Jacky Jiang]

Created attachment 178377 [details]
Patch

[Rob Buis]

Comment on attachment 178377 [details]
Patch

Patch looks good overall. Are you sure it is not causing extra layouts for instance on ltr?

[Rob Buis]

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?

[Rob Buis]

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.

[Jacky Jiang]

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.

[Jacky Jiang]

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

[Rob Buis]

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.

[Rob Buis]

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

[Jacky Jiang]

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

[Jacky Jiang]

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

[Rob Buis]

Comment on attachment 178377 [details]
Patch

This seems fine, after discussing it on irc.

[Jacky Jiang]

Committed r137049: <http://trac.webkit.org/changeset/137049>

[Antonio Gomes]

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.

[Jacky Jiang]

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

[Rob Buis]

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