Bug 116514 – New Flickr doesn't get fast scrolling but should

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

We don't get fast scrolling on the new Flickr pages, e.g. <http://www.flickr.com/photos/smfr/>

During loading, there's a background-attachment:fixed that kicks us out. However, when loading is complete this fixed background is removed, but we fail to go into fast scrolling mode.

<rdar://problem/13946443>

[Simon Fraser (smfr)]

Created attachment 202379 [details]
Patch

[WebKit Commit Bot]

Attachment 202379 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac-wk2/tiled-drawing/slow-scrolling-background-toggle-expected.txt', u'LayoutTests/platform/mac-wk2/tiled-drawing/slow-scrolling-background-toggle.html', u'LayoutTests/platform/mac-wk2/tiled-drawing/slow-scrolling-expected.txt', u'LayoutTests/platform/mac-wk2/tiled-drawing/slow-scrolling-hidden-background-toggle-expected.txt', u'LayoutTests/platform/mac-wk2/tiled-drawing/slow-scrolling-hidden-background-toggle.html', u'LayoutTests/platform/mac-wk2/tiled-drawing/slow-scrolling.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderObject.cpp']" exit_code: 1
Source/WebCore/rendering/RenderObject.cpp:133:  Do not add platform specific code in WebCore outside of platform.  [build/webcore_platform_layering_violation] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Style bot complaint is existing code.

[Darin Adler]

Comment on attachment 202379 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:130
> +    UNUSED_PARAM(frameView);

Seems wrong to have this UNUSED_PARAM be unconditional, when the use of the parameter is conditional.

>> Source/WebCore/rendering/RenderObject.cpp:133
>> +#if PLATFORM(QT)
> 
> Do not add platform specific code in WebCore outside of platform.  [build/webcore_platform_layering_violation] [5]

I don’t think the style bot is being helpful here.

> Source/WebCore/rendering/RenderObject.cpp:2490
> +    if (view()->frameView()) {

I think we should put the FrameView into a local because it’s used two more times below and the code would be more concise without repeating it.

> Source/WebCore/rendering/RenderObject.cpp:2491
> +        bool repaintFixedBackgroundsOnScroll = shouldRepaintFixedBackgroundsOnScroll(view()->frameView());

Why the local variable here? I think it makes the code harder to read.

[Simon Fraser (smfr)]

(In reply to comment #4)
> (From update of attachment 202379 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202379&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:130
> > +    UNUSED_PARAM(frameView);
> 
> Seems wrong to have this UNUSED_PARAM be unconditional, when the use of the parameter is conditional.

I thought it was simpler than having the UNUSED_PARAM inside an #else; it's harmless to have it when the param is actually used, but I can clean this up.

> > Do not add platform specific code in WebCore outside of platform.  [build/webcore_platform_layering_violation] [5]
> 
> I don’t think the style bot is being helpful here.

Agreed!

> > Source/WebCore/rendering/RenderObject.cpp:2490
> > +    if (view()->frameView()) {
> 
> I think we should put the FrameView into a local because it’s used two more times below and the code would be more concise without repeating it.

Will do.

> > Source/WebCore/rendering/RenderObject.cpp:2491
> > +        bool repaintFixedBackgroundsOnScroll = shouldRepaintFixedBackgroundsOnScroll(view()->frameView());
> 
> Why the local variable here? I think it makes the code harder to read.

I can remove this.

Simon

[Simon Fraser (smfr)]

https://trac.webkit.org/r150529