Bug 49840 – GraphicsContext: Make drawFocusRing() take a Path instead of a Vector<Path>

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

This method:
void drawFocusRing(const Vector<Path>&, int width, int offset, const Color&);

..is only ever used by RenderImage::paintFocusRings(), where a single Path is put into a Vector<Path> and passed to this function.

[Andreas Kling]

Created attachment 74440 [details]
Proposed patch

[Eric Seidel (no email)]

Attachment 74440 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6184087

[Andreas Kling]

(In reply to comment #2)
> Attachment 74440 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/6184087

/Users/kling/src/webkit/WebCore/platform/graphics/mac/GraphicsContextMac.mm:60: warning: unused parameter 'offset'

[Nikolas Zimmermann]

Comment on attachment 74440 [details]
Proposed patch

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

Almost there, I'm just wondering about the need of the offset parameter used in drawFocusRing/drawFocusRect.
The parameter offset is used in GraphicsContextMac.mm, drawFocusRect, but not in drawFocusRing, that causes your mac build problem.

In GraphicsContextQt the parameter is unused as well, do we need it? We have two options:
a) if we need it, and ignore it for now, add a FIXME and file a bug about it.
b) use the parameter, cover its usage by a test, if it doesn't affect existing tets.

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

offset is unused causing the mac build error.

> WebCore/platform/graphics/mac/GraphicsContextMac.mm:-66
> -    offset += radius;

It built before, as offset was touched here, even if the code below doesn't use it.

> WebCore/rendering/RenderImage.cpp:361
> +        paintInfo.context->drawFocusRing(areaElement->getPath(this), style->outlineWidth(), style->outlineOffset(), style->visitedDependentColor(CSSPropertyOutlineColor));

Hm, the style->outlineOffset() parameter is passed to drawFocusRing. For example in GraphicsContextCG it remains unused, that's why your mac build failed according to EWS.
In GraphicsContextQt is's also unused. Can you identify whether we need this parameter? I guess we do, and current implementations just don't respect them.

[Andreas Kling]

Created attachment 74491 [details]
Proposed patch

Fix warning about unused "offset" variables.
Added FIXMEs about it, will open a bug for that when this lands.

[Nikolas Zimmermann]

Comment on attachment 74491 [details]
Proposed patch

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

> WebCore/platform/graphics/mac/GraphicsContextMac.mm:62
> +    // FIXME: Use 'offset' for something?

Can you file a bug first, and reference the bug id here?

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:805
> +    // FIXME: Use 'width' and 'offset' for something?

Can you file a bug first, and reference the bug id here?

[Andreas Kling]

Committed r72528: <http://trac.webkit.org/changeset/72528>