Update GraphicsContextSkia when the Chromium platform is Mac
Created attachment 97604 [details] Patch
Comment on attachment 97604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97604&action=review It's very unclear to me what code we're trying to share vs. not here. Would be more clear if the #ifdefed code was pulled off into static functions. > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:556 > +#if PLATFORM(CHROMIUM) && OS(DARWIN) > + float radius = (width - 1) / 2.0f; > + int outer = width; > + float inner = (width) / 2.0f; > + const SkScalar focusRingOutset = (width / 2.0f) + 0.25f; > +#else > const SkScalar focusRingOutset = WebCoreFloatToSkScalar(0.5); > +#endif I think I would have abstracted this into a static inline focusRingOutset(width) function.
Created attachment 97988 [details] Patch
Comment on attachment 97604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97604&action=review >> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:556 >> +#endif > > I think I would have abstracted this into a static inline focusRingOutset(width) function. Done.
Comment on attachment 97988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97988&action=review This looks much nicer. Thank you. > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:579 > + const SkScalar focusRingOutset = getFocusRingOutset(width); WebKit style doesn't use "get" in method names.
I guess I generalized a little: http://www.webkit.org/coding/coding-style.html: Precede setters with the word "set". Use bare words for getters. Setter and getter names should match the names of the variables being set/gotten. Right: void setCount(size_t); // sets m_count size_t count(); // returns m_count Wrong: void setCount(size_t); // sets m_theCount size_t getCount();
Comment on attachment 97988 [details] Patch I'm not sure it's worth the round-trip for just the "get" though.
I imagine I will have to continue to tweak this code to make the look of the focus ring better -- I'm happy to rename it to remove the 'get' the next time I'm in there.
Comment on attachment 97988 [details] Patch Clearing flags on attachment: 97988 Committed r89366: <http://trac.webkit.org/changeset/89366>
All reviewed patches have been landed. Closing bug.