Bug 62867 – Update GraphicsContextSkia when the Chromium platform is Mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Cary Clark]

Update GraphicsContextSkia when the Chromium platform is Mac

[Cary Clark]

Created attachment 97604 [details]
Patch

[Eric Seidel (no email)]

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.

[Cary Clark]

Created attachment 97988 [details]
Patch

[Cary Clark]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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();

[Eric Seidel (no email)]

Comment on attachment 97988 [details]
Patch

I'm not sure it's worth the round-trip for just the "get" though.

[Cary Clark]

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.

[WebKit Review Bot]

Comment on attachment 97988 [details]
Patch

Clearing flags on attachment: 97988

Committed r89366: <http://trac.webkit.org/changeset/89366>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.