Bug 106267 - [chromium] Add OpaqueRegionSkia::currentTrackingOpaqueRect
Summary: [chromium] Add OpaqueRegionSkia::currentTrackingOpaqueRect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-07 15:38 PST by Alok Priyadarshi
Modified: 2013-01-08 11:17 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.66 KB, patch)
2013-01-07 16:03 PST, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2013-01-07 15:38:49 PST
Minor implementation cleanup. Make currentTrackingOpaqueRect() a member function of OpaqueRegionSkia so that we do not need to pass member variables everywhere.
Comment 1 Alok Priyadarshi 2013-01-07 16:03:31 PST
Created attachment 181590 [details]
Patch
Comment 2 Dana Jansens 2013-01-08 10:06:39 PST
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?
Comment 3 Alok Priyadarshi 2013-01-08 10:38:49 PST
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.
Comment 4 Dana Jansens 2013-01-08 10:40:39 PST
(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.
Comment 5 Stephen White 2013-01-08 11:02:05 PST
Comment on attachment 181590 [details]
Patch

Seems ok.  r=me
Comment 6 WebKit Review Bot 2013-01-08 11:17:18 PST
Comment on attachment 181590 [details]
Patch

Clearing flags on attachment: 181590

Committed r139083: <http://trac.webkit.org/changeset/139083>
Comment 7 WebKit Review Bot 2013-01-08 11:17:21 PST
All reviewed patches have been landed.  Closing bug.