| Summary: | [Chromium] Draw search tickmarks on overlay scrollbars | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sailesh Agrawal <sail> | ||||||||
| Component: | New Bugs | Assignee: | Sailesh Agrawal <sail> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dglazkov, dpranke, eric, jamesr, mihaip, ojan, rsesek, thakis, tony, webkit.review.bot, yutak | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Sailesh Agrawal
2011-06-15 23:41:56 PDT
Created attachment 97407 [details]
Patch
Adding Cole and Robert for feedback on the UI. Screenshots here: http://www.dropmocks.com/mWaIN Looks like you added me instead of Cole. I like the screenshots fwiw. Trying to add Cole gets me a "alcor@google.com" didn't match anything error. I sent him an email instead. Got an LGTM from Cole. Please take a look. 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. 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? Created attachment 97488 [details]
Patch
> 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. Comment on attachment 97488 [details] Patch Clearing flags on attachment: 97488 Committed r89073: <http://trac.webkit.org/changeset/89073> All reviewed patches have been landed. Closing bug. 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?
Reverted r89073 for reason: Broke Chromium Clang build Committed r89120: <http://trac.webkit.org/changeset/89120> 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. :( 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. Created attachment 97886 [details]
Patch
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. Comment on attachment 97886 [details] Patch Clearing flags on attachment: 97886 Committed r89326: <http://trac.webkit.org/changeset/89326> All reviewed patches have been landed. Closing bug. |