Bug 106267 – [chromium] Add OpaqueRegionSkia::currentTrackingOpaqueRect

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

Minor implementation cleanup. Make currentTrackingOpaqueRect() a member function of OpaqueRegionSkia so that we do not need to pass member variables everywhere.

[Alok Priyadarshi]

Created attachment 181590 [details]
Patch

[Dana Jansens]

Comment on attachment 181590 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:431
> +SkRect& OpaqueRegionSkia::currentTrackingOpaqueRect()

This is fine.. it's adding an extra function call to the stack though, instead of using the inline method. Is this worth it for some reason? I'm not sure of the motivation here. What's wrong with the current method?

[Alok Priyadarshi]

This patch comes from https://bugs.webkit.org/show_bug.cgi?id=105051, which I decided to abandon. I am trying to extract all the minor cleanup changes and land them separately.

The motivation is to reduce duplicated code and potential for bugs. The function arguments (m_opaqueRect, m_canvasLayerStack) are duplicated in all call sites.

[Dana Jansens]

(In reply to comment #3)
> This patch comes from https://bugs.webkit.org/show_bug.cgi?id=105051, which I decided to abandon. I am trying to extract all the minor cleanup changes and land them separately.
> 
> The motivation is to reduce duplicated code and potential for bugs. The function arguments (m_opaqueRect, m_canvasLayerStack) are duplicated in all call sites.

I don't see this as a particularly dangerous pattern, but I don't feel strongly either way. I'll defer to senorblanco.

[Stephen White]

Comment on attachment 181590 [details]
Patch

Seems ok.  r=me

[WebKit Review Bot]

Comment on attachment 181590 [details]
Patch

Clearing flags on attachment: 181590

Committed r139083: <http://trac.webkit.org/changeset/139083>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.