Bug 90528 - [chromium] Remove dependency on ScrollbarTheme from the compositor
Summary: [chromium] Remove dependency on ScrollbarTheme from the compositor
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: Adrienne Walker
URL:
Keywords:
Depends on: 90094
Blocks: 91032
  Show dependency treegraph
 
Reported: 2012-07-04 01:23 PDT by Adrienne Walker
Modified: 2012-07-30 14:11 PDT (History)
11 users (show)

See Also:


Attachments
Patch (104.37 KB, patch)
2012-07-04 01:43 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Rebased (104.94 KB, patch)
2012-07-11 12:24 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Export the right functions (105.07 KB, patch)
2012-07-11 14:48 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (375.30 KB, application/zip)
2012-07-11 15:43 PDT, WebKit Review Bot
no flags Details
Rebased, static casts (104.10 KB, patch)
2012-07-26 10:54 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch for landing (105.65 KB, patch)
2012-07-30 09:35 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-07-04 01:23:58 PDT
[chromium] Make ScrollbarLayerChromium use WebScrollbarLayer
Comment 1 Adrienne Walker 2012-07-04 01:43:38 PDT
Created attachment 150737 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-04 01:47:10 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adrienne Walker 2012-07-04 01:47:10 PDT
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.  :(
Comment 4 WebKit Review Bot 2012-07-04 01:58:54 PDT
Comment on attachment 150737 [details]
Patch

Attachment 150737 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13139442
Comment 5 Adrienne Walker 2012-07-11 12:24:43 PDT
Created attachment 151750 [details]
Rebased
Comment 6 Adrienne Walker 2012-07-11 14:48:54 PDT
Created attachment 151786 [details]
Export the right functions
Comment 7 WebKit Review Bot 2012-07-11 15:43:11 PDT
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
Comment 8 WebKit Review Bot 2012-07-11 15:43:15 PDT
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
Comment 9 Adrienne Walker 2012-07-11 16:59:09 PDT
Comment on attachment 151786 [details]
Export the right functions

That test failure just looks like flake to me.
Comment 10 Adam Barth 2012-07-12 10:55:37 PDT
> 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.
Comment 11 Adam Barth 2012-07-12 10:59:49 PDT
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.
Comment 12 Adam Barth 2012-07-12 11:03:12 PDT
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.
Comment 13 Adrienne Walker 2012-07-26 10:54:07 PDT
Created attachment 154687 [details]
Rebased, static casts
Comment 14 Adrienne Walker 2012-07-26 10:56:01 PDT
jamesr: This GTFO patch is all you, whenever you get a chance.
Comment 15 James Robinson 2012-07-26 15:04:36 PDT
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)
Comment 16 James Robinson 2012-07-27 11:04:16 PDT
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
Comment 17 Adrienne Walker 2012-07-30 09:35:55 PDT
Created attachment 155303 [details]
Patch for landing
Comment 18 Adrienne Walker 2012-07-30 09:37:44 PDT
(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.
Comment 19 WebKit Review Bot 2012-07-30 14:11:28 PDT
Comment on attachment 155303 [details]
Patch for landing

Clearing flags on attachment: 155303

Committed r124091: <http://trac.webkit.org/changeset/124091>
Comment 20 WebKit Review Bot 2012-07-30 14:11:35 PDT
All reviewed patches have been landed.  Closing bug.