[webkit-reviews] review granted: [Bug 132593] [Mac] Allow focus rings to redraw themselves if necessary : [Attachment 230940] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 132593: [Mac] Allow focus rings to redraw themselves if necessary
https://bugs.webkit.org/show_bug.cgi?id=132593

Attachment 230940: Patch
https://bugs.webkit.org/attachment.cgi?id=230940&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230940&action=review


> Source/WebCore/page/FocusController.cpp:172
> +    m_focusRepaintTimer.stop();

I think timers stop themselves on destruction.

> Source/WebCore/page/FocusController.cpp:915
> +    m_focusRepaintTimer.startOneShot(0);

Why repaint at > 60fps?

> Source/WebCore/page/FocusController.h:93
> +    void focusedElementNeedsRepaint();

This sounds like it is returning state. I think it should be
setFocusedElementNeedsRepaintSoon().

> Source/WebCore/page/FocusController.h:94
> +    double timeSinceFocusWasSet() const;

Aren't the cool kids using std::chromo these days?

> Source/WebCore/platform/ControlStates.h:108
> +    double m_timeSinceFocusWasSet;

Hmm, wouldn't this value be continually changing? Seems odd to store it. Is it
something else here, like time used for painting the next focus ring state?

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:73
> +void GraphicsContext::drawFocusRing(const Path& path, int /* width */, int
/*offset*/, const Color&)

Spacing in comments is inconsistent.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:91
> +    for (IntRect rect : rects)

const IntRect& ? Or auto&?

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:107
> +    for (IntRect rect : rects)

Ditto.

> Source/WebCore/platform/mac/ThemeMac.mm:287
> -	   { 3, 4, 4, 2 },
> -	   { 4, 3, 3, 3 },
> -	   { 4, 3, 3, 3 },
> +	   { 7, 8, 8, 6 },
> +	   { 8, 7, 7, 7 },
> +	   { 8, 8, 7, 7 },

Why aren't these different on different OSes?

> Source/WebCore/rendering/RenderElement.cpp:374
> +	  
view().setMaximalOutlineSize(std::max(theme().platformFocusRingMaxWidth(),
static_cast<int>(m_style->outlineSize())));

So m_style->outlineSize() is a lie now? Won't that break repainting, e.g. in
RenderElement::styleWillChange() which does if (m_parent &&
(newStyle.outlineSize() < m_style->outlineSize()...