Bug 147134 - Facebook in tiled fullscreen is slow
Summary: Facebook in tiled fullscreen is slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-20 17:57 PDT by Simon Fraser (smfr)
Modified: 2015-07-20 19:02 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2015-07-20 18:19 PDT, Simon Fraser (smfr)
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2015-07-20 17:57:39 PDT
Facebook in tiled fullscreen is slow
Comment 1 Simon Fraser (smfr) 2015-07-20 18:19:30 PDT
Created attachment 257154 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-07-20 18:20:00 PDT
rdar://problem/21823349
Comment 3 Said Abou-Hallawa 2015-07-20 18:39:10 PDT
Comment on attachment 257154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257154&action=review

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:291
> +    if (!documentWidthChanged && !viewWidthChanged)

why the check for m_webPage.mainFrame()->view()->needsLayout() was removed?

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:300
> +    if (m_isScalingViewToFitDocument && !documentWidthChanged) {

Does m_isScalingViewToFitDocument need to be a member of the class?

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:336
> +        IntSize fixedLayoutSize(documentWidth, std::ceil((m_webPage.size().height() - m_webPage.corePage()->topContentInset()) / viewScale));

This block is very similar to the above one, can't the code be refactored such that the same code is not repeated?
Comment 4 Tim Horton 2015-07-20 18:45:10 PDT
Comment on attachment 257154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257154&action=review

> Source/WebKit2/ChangeLog:13
> +        This happened because the the fixed layout size was computed using ceil(m_webPage.size().width() / viewScale)

Do the scrollbars flicker all weirdly now when resizing a simple page inside the scaling region?

>> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:291
>> +    if (!documentWidthChanged && !viewWidthChanged)
> 
> why the check for m_webPage.mainFrame()->view()->needsLayout() was removed?

Because smfr added a layoutIfNeeded above, so it will always be true.

>> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:300
>> +    if (m_isScalingViewToFitDocument && !documentWidthChanged) {
> 
> Does m_isScalingViewToFitDocument need to be a member of the class?

Yes? And it already is.
Comment 5 Simon Fraser (smfr) 2015-07-20 18:58:42 PDT
(In reply to comment #4)
> Comment on attachment 257154 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257154&action=review
> 
> > Source/WebKit2/ChangeLog:13
> > +        This happened because the the fixed layout size was computed using ceil(m_webPage.size().width() / viewScale)
> 
> Do the scrollbars flicker all weirdly now when resizing a simple page inside
> the scaling region?

Not any worse than without the patch.
> > Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:336
> > +        IntSize fixedLayoutSize(documentWidth, std::ceil((m_webPage.size().height() - m_webPage.corePage()->topContentInset()) / viewScale));
> 
> This block is very similar to the above one, can't the code be refactored
> such that the same code is not repeated?

I tried to use a lambda and it wasn't clean.
Comment 6 Simon Fraser (smfr) 2015-07-20 19:02:44 PDT
https://trac.webkit.org/r187054