[chromium] Make ScrollbarLayerChromium use WebScrollbarLayer
Created attachment 150737 [details] Patch
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.
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 on attachment 150737 [details] Patch Attachment 150737 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13139442
Created attachment 151750 [details] Rebased
Created attachment 151786 [details] Export the right functions
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
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 on attachment 151786 [details] Export the right functions That test failure just looks like flake to me.
> 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 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.
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.
Created attachment 154687 [details] Rebased, static casts
jamesr: This GTFO patch is all you, whenever you get a chance.
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 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
Created attachment 155303 [details] Patch for landing
(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 on attachment 155303 [details] Patch for landing Clearing flags on attachment: 155303 Committed r124091: <http://trac.webkit.org/changeset/124091>
All reviewed patches have been landed. Closing bug.