Bug 62783 – [Chromium] Draw search tickmarks on overlay scrollbars

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

[Sailesh Agrawal]

[Chromium] Draw search tickmarks on overlay scrollbars

[Sailesh Agrawal]

Created attachment 97407 [details]
Patch

[Sailesh Agrawal]

Adding Cole and Robert for feedback on the UI. Screenshots here:
http://www.dropmocks.com/mWaIN

[Nico Weber]

Looks like you added me instead of Cole. I like the screenshots fwiw.

[Sailesh Agrawal]

Trying to add Cole gets me a
   "[e-mail]" didn't match anything
error. I sent him an email instead.

[Sailesh Agrawal]

Got an LGTM from Cole. Please take a look.

[Mihai Parparita]

Comment on attachment 97407 [details]
Patch

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

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:488
> +        if (wkScrollbarPainterTrackAlpha(scrollbarPainter) >= 1.0)

Might make more sense fo the tickmarks to use the same alpha as the track alpha, so that they fade out gradually too.

[Eric Seidel (no email)]

Comment on attachment 97407 [details]
Patch

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

> Source/WebCore/platform/chromium/ScrollbarOverlayUtilitiesChromiumMac.h:61
> +void wkScrollbarPainterPaintTrack(WKScrollbarPainterRef, bool enabled, double value, CGFloat proportion, NSRect frameRect);
> +void wkScrollbarPainterPaintKnob(WKScrollbarPainterRef);

I assume this is our version of WebKitSystemInterface.h?

[Sailesh Agrawal]

Created attachment 97488 [details]
Patch

[Sailesh Agrawal]

> Might make more sense fo the tickmarks to use the same alpha as the track alpha, so that they fade out gradually too.

Good idea. Fixed.
 
> I assume this is our version of WebKitSystemInterface.h?

Yea.

[WebKit Review Bot]

Comment on attachment 97488 [details]
Patch

Clearing flags on attachment: 97488

Committed r89073: <http://trac.webkit.org/changeset/89073>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Hi, your change seemed to cause a compile error in our clang bots:

In file included from /b/build/slave/Mac_Clang_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/chromium/ScrollAnimatorChromiumMac.mm:36:
../platform/chromium/ScrollbarThemeChromiumMac.h:74:10: error: 'WebCore::ScrollbarThemeChromiumMac::paintTickmarks' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
    void paintTickmarks(Scrollbar*, GraphicsContext* drawingContext, bool canDrawDirectly, float alpha);
         ^
../platform/ScrollbarThemeComposite.h:65:18: note: hidden overloaded virtual function 'WebCore::ScrollbarThemeComposite::paintTickmarks' declared here
    virtual void paintTickmarks(GraphicsContext*, Scrollbar*, const IntRect&) {}
                 ^
1 error generated.

Could you fix this at your earliest convenience?

[Yuta Kitamura]

Reverted r89073 for reason:

Broke Chromium Clang build

Committed r89120: <http://trac.webkit.org/changeset/89120>

[Eric Seidel (no email)]

We need a clang EWS and clang bot on build.webkit.org to stop this "this broke clang but nothing else" rollout nonsense.

This is at least the 3rd bug I've seen this happen to in the last week. :(

[Eric Seidel (no email)]

Also, why should a build error ever cause a rollout?  I would imagine build errors are generally super-easy to fix?

Not that I'm against rollouts!  It just seems silly to do this dance every time for a clang-only build error.

[Sailesh Agrawal]

Created attachment 97886 [details]
Patch

[Sailesh Agrawal]

Fixed clang error.
The problem was that there was already a paintTickmarks() method in the super class. I changed the patch to use the same method signature as the super class. The patch passes the chromium clang bog:
http://build.chromium.org/p/tryserver.chromium/builders/mac_clang/builds/899

Please take another look. Thanks.

[WebKit Review Bot]

Comment on attachment 97886 [details]
Patch

Clearing flags on attachment: 97886

Committed r89326: <http://trac.webkit.org/changeset/89326>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.