[webkit-reviews] review granted: [Bug 109560] Implement coordinated scrollbar for subframes and overflow:scroll : [Attachment 189644] Patch

Thu Feb 21 17:41:54 PST 2013

James Robinson <jamesr at chromium.org> has granted Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 109560: Implement coordinated scrollbar for subframes and overflow:scroll
https://bugs.webkit.org/show_bug.cgi?id=109560

Attachment 189644: Patch
https://bugs.webkit.org/attachment.cgi?id=189644&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=189644&action=review

R=me but the conditionals for early out still need a bit of work.

I suspect the lifetime cleanups may fix issues like
https://bugs.webkit.org/show_bug.cgi?id=108524 which would be a nice bonus.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:157
> +    if (!(isMainFrame ? supportWebScrollbarLayerForMainFrame() :
supportWebScrollbarLayerForAllLayer()))

this is too tricky to follow.  it also doesn't make much sense that we test for
the main frame at all if the platform claims not to support "all layer",
although I don't really know what "ForAllLayer" means.

Please expand this out.  I'd expect to see something like

if (can't support coordinated scrollbars on anything)
   return;

if (!isMainFrame && can only support coordinated scrollbars on main frame)
   return;

with better names.  I'd probably skip using functions and just have some locals
bols that are initialized inside #ifs

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:89
> +#if OS(DARWIN)
> +    static bool supportWebScrollbarLayerForMainFrame() { return false; }
> +    static bool supportWebScrollbarLayerForAllLayer() { return false; }
> +#elif OS(WINDOWS)
> +    static bool supportWebScrollbarLayerForMainFrame() { return true; }
> +    static bool supportWebScrollbarLayerForAllLayer() { return false; }
> +#else
> +    static bool supportWebScrollbarLayerForMainFrame() { return true; }
> +    static bool supportWebScrollbarLayerForAllLayer() { return true; }
> +#endif

Don't put these in the header - the only callers are in the .cpp.  Move to cpp
or just ditch completely

> Source/WebKit/chromium/tests/data/iframe-scrolling-inner.html:1
> +<html>

<!DOCTYPE html> too