Bug 93482 – [BlackBerry] Tie up the scrolling machinery to the graphics tree when applicable for in-region scroll

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Antonio Gomes]

PR #187672

WebKit side needs to expose a way to client to translate the proper layer in the ui/compositing thread.

[Antonio Gomes]

Created attachment 157229 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/PlatformBlackBerry.cmake', u..." exit_code: 1
Source/WebKit/blackberry/Api/InRegionScroller_p.h:19:  #ifndef header guard has wrong style, please use: InRegionScroller_p_h  [build/header_guard] [5]
Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 8 files


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

[Antonio Gomes]

Created attachment 157235 [details]
patch - fixed style errors

[Rob Buis]

Comment on attachment 157235 [details]
patch - fixed style errors

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

Looks good, can be cleaned up some more.

> Source/WebKit/ChangeLog:3
> +        [BlackBerry] Tie up the scrolling machinery to  the graphics tree when applicable for in-region scroll

Some unneeded whitespace in there.

> Source/WebKit/PlatformBlackBerry.cmake:-104
> -    blackberry/WebKitSupport/InspectorOverlay.cpp

Why remove this?

> Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp:91
> +        if (usesCompositedScrolling) {

usesCompositedScrolling variable does not seem needed.

[Konrad Piascik]

Comment on attachment 157235 [details]
patch - fixed style errors

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

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:278
> +            layer->scrollToOffset(newOffset.x(), newOffset.y());

Use the new updated API for RenderLayer::scrollToOffset(IntSize)

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:376
> +    return o->isPositioned() && o->style()->position() == FixedPosition;

This needs to be changed.

[Antonio Gomes]

Created attachment 157248 [details]
patch - fixed rob's suggestions

[Antonio Gomes]

Created attachment 157251 [details]
patch - fixed konrad's suggestions

[Rob Buis]

Comment on attachment 157251 [details]
patch - fixed konrad's suggestions

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

This looks good in general but please check your headers.

> Source/WebKit/blackberry/Api/InRegionScroller.h:32
> +

Empty line not needed.

> Source/WebKit/blackberry/Api/InRegionScroller_p.h:22
> +#include "IntRect.h"

Is this actually used?

> Source/WebKit/blackberry/Api/InRegionScroller_p.h:29
> +class LayerCompositingThread;

Does not seem used?

> Source/WebKit/blackberry/Api/InRegionScroller_p.h:40
> +

Empty line not needed

> Source/WebKit/blackberry/Api/InRegionScroller_p.h:57
> +private:

I wonder if we need private in this class.

[Antonio Gomes]

Created attachment 157258 [details]
patch - fixed rob's suggestions

> > Source/WebKit/blackberry/Api/InRegionScroller.h:32
> > +
> 
> Empty line not needed.

Fixed

> > Source/WebKit/blackberry/Api/InRegionScroller_p.h:22
> > +#include "IntRect.h"
> 
> Is this actually used?

Yes and no, but I replaced by IntSize.h and IntPoint.h to be explicit.
 
> > Source/WebKit/blackberry/Api/InRegionScroller_p.h:29
> > +class LayerCompositingThread;
> 
> Does not seem used?

No, removed.

> > Source/WebKit/blackberry/Api/InRegionScroller_p.h:40
> > +
> 
> Empty line not needed

Fixed.

> > Source/WebKit/blackberry/Api/InRegionScroller_p.h:57
> > +private:
> 
> I wonder if we need private in this class.

Although it is uncommon and I agree to generally make all member of private classes asa public, I do not want these private methods to be called at all by anyone else. They are internal helpers.

[Rob Buis]

Comment on attachment 157258 [details]
patch - fixed rob's suggestions

Looks good.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/..." exit_code: 1
Source/WebKit/blackberry/Api/InRegionScroller_p.h:22:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


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

[Antonio Gomes]

Committed <http://trac.webkit.org/changeset/125071>