Bug 90528 – [chromium] Remove dependency on ScrollbarTheme from the compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Make ScrollbarLayerChromium use WebScrollbarLayer

[Adrienne Walker]

Created attachment 150737 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

I think the ChangeLogs give a good overview of what all this new code is for. 

Belatedly, I this patch could just add WebScrollbar, WebScrollbarImpl, and WebScrollbarThemeClientImpl and I could have punted painting and geometry until later, but I was already done by the time I had that realization.  Sorry for the 100k patch of boilerplate.  :(

[WebKit Review Bot]

Comment on attachment 150737 [details]
Patch

Attachment 150737 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13139442

[Adrienne Walker]

Created attachment 151750 [details]
Rebased

[Adrienne Walker]

Created attachment 151786 [details]
Export the right functions

[WebKit Review Bot]

Comment on attachment 151786 [details]
Export the right functions

Attachment 151786 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13198546

New failing tests:
jquery/manipulation.html

[WebKit Review Bot]

Created attachment 151806 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adrienne Walker]

Comment on attachment 151786 [details]
Export the right functions

That test failure just looks like flake to me.

[Adam Barth]

> That test failure just looks like flake to me.

Yes, sorry.  That's one of the flakier tests.  We should mark it as flaky in TestExpectations.

[Adam Barth]

Comment on attachment 151786 [details]
Export the right functions

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

> Source/WebKit/chromium/src/WebScrollbarImpl.cpp:164
> +    switch (m_scrollbar->scrollbarOverlayStyle()) {
> +    default:
> +        ASSERT_NOT_REACHED();
> +    case WebCore::ScrollbarOverlayStyleDefault:
> +        return WebScrollbar::ScrollbarOverlayStyleDefault;
> +    case WebCore::ScrollbarOverlayStyleDark:
> +        return WebScrollbar::ScrollbarOverlayStyleDark;
> +    case WebCore::ScrollbarOverlayStyleLight:
> +        return WebScrollbar::ScrollbarOverlayStyleLight;
> +    }

Typically we use a COMPILE_ASSERT that these enums match and then just use static_cast to convert between them.

[Adam Barth]

I don't feel confident in reviewing this patch.  :(  I understand the big picture of what we're trying to do here, but I don't have much knowledge about the details.  I probably should have synced up with jamesr earlier to understand what he'd like me to do in these cases.

[Adrienne Walker]

Created attachment 154687 [details]
Rebased, static casts

[Adrienne Walker]

jamesr: This GTFO patch is all you, whenever you get a chance.

[James Robinson]

Thanks, will look ASAP (still have a bit of catching up to do before diving into patches, but hope to look by end of the week)

[James Robinson]

Comment on attachment 154687 [details]
Rebased, static casts

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

R=me (with many comments)

> Source/Platform/chromium/public/WebScrollbar.h:47
> +#if WEBKIT_IMPLEMENTATION
> +    WEBKIT_EXPORT static PassOwnPtr<WebScrollbar> create(WebCore::Scrollbar*);
> +#endif

I think WEBKIT_IMPLEMENTATION blocks are more commonly at the end of the public: section, with the rationale being that most people who read the interface don't need to know or care about implementation details

> Source/Platform/chromium/public/WebScrollbarThemeGeometry.h:29
> +#include <public/WebRect.h>

just "WebRect.h" - it's in the same directory so a non-qualified #include will work and I'm not sure that Source/Platform/chromium/ is on the include path of every file that might end up transitively including this header.

> Source/Platform/chromium/public/WebScrollbarThemeGeometry.h:72
> +    WebScrollbarThemeGeometry(WebCore::ScrollbarThemeComposite*);

do we want explicit? (I'm not sure, it might be helpful to have implicit conversions somewhere)

> Source/Platform/chromium/public/WebScrollbarThemePainter.h:29
> +#include <public/WebCanvas.h>

"WebCanvas.h"

> Source/WebKit/chromium/src/WebScrollbarImpl.h:40
> +    // Implement WebKit::WebScrollbar methods

OVERRIDEs, in that case?

> Source/WebKit/chromium/src/WebScrollbarThemeClientImpl.h:47
> +    virtual int x() const;

OVERRIDEs, please

[Adrienne Walker]

Created attachment 155303 [details]
Patch for landing

[Adrienne Walker]

(In reply to comment #16)
> (From update of attachment 154687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154687&action=review
> 
> R=me (with many comments)

Not that many, thanks.  :)

> > Source/Platform/chromium/public/WebScrollbar.h:47
> > +#if WEBKIT_IMPLEMENTATION
> > +    WEBKIT_EXPORT static PassOwnPtr<WebScrollbar> create(WebCore::Scrollbar*);
> > +#endif
> 
> I think WEBKIT_IMPLEMENTATION blocks are more commonly at the end of the public: section, with the rationale being that most people who read the interface don't need to know or care about implementation details

Thanks.  I don't think I recognized that was a stylistic choice before.

> > Source/Platform/chromium/public/WebScrollbarThemeGeometry.h:29
> > +#include <public/WebRect.h>
> 
> just "WebRect.h" - it's in the same directory so a non-qualified #include will work and I'm not sure that Source/Platform/chromium/ is on the include path of every file that might end up transitively including this header.

Done.

> > Source/Platform/chromium/public/WebScrollbarThemeGeometry.h:72
> > +    WebScrollbarThemeGeometry(WebCore::ScrollbarThemeComposite*);
> 
> do we want explicit? (I'm not sure, it might be helpful to have implicit conversions somewhere)

Yes.  Done.

> > Source/Platform/chromium/public/WebScrollbarThemePainter.h:29
> > +#include <public/WebCanvas.h>
> 
> "WebCanvas.h"

Done.

> > Source/WebKit/chromium/src/WebScrollbarImpl.h:40
> > +    // Implement WebKit::WebScrollbar methods
> 
> OVERRIDEs, in that case?

Done.

> > Source/WebKit/chromium/src/WebScrollbarThemeClientImpl.h:47
> > +    virtual int x() const;
> 
> OVERRIDEs, please

Done.

[WebKit Review Bot]

Comment on attachment 155303 [details]
Patch for landing

Clearing flags on attachment: 155303

Committed r124091: <http://trac.webkit.org/changeset/124091>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.