[#2] Pass gfx structs by const ref (gfx::Rect, gfx::RectF)

Pass gfx::Rect and gfx::RectF by const ref.

Avoid unneccessary copy of structures gfx::Rect & gfx::RectF 
by passing them by const ref rather than value. 

Any struct of size > 4 bytes should be passed by const ref. 
Passing by ref for these structs is faster than passing 
by value, especially when invoking function has multiple parameters. 

Pass by value creates unneccessary overhead which should be avoided.

BUG=159273
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244224

[prashant.n, 2013-12-11 09:34:34]

Sanity cl for passing by const ref.

[Tom Hudson, 2013-12-11 09:48:30]

Not clear why you're removing the presubmit check?

[prashant.n, 2013-12-11 11:56:17]

The presubmit condition added checked whether const ref for gfx::Rect (and some
other structs) is present or not and if present, it was throwing errors as
earlier we had decided to pass small structs by value.

Now we want to pass all these structs by const ref.

Basically we'll want to pass all structs having size > 4 bytes by const ref. So
no need to have check in cc/PRESUBMIT.py.

[enne (OOO), 2013-12-11 15:59:36]

Is there data to support doing this for Vector/Point/Size/Time too, since you're
removing those checks?

[prashant.n, 2013-12-12 12:44:36]

@enne, I added the data in bug for Size, the same results we will get for Point,
Vector as both are using 2 ints. For TimeTicks and Time I'll check.

[danakj, 2013-12-12 17:23:15]

On Thu, Dec 12, 2013 at 4:44 AM, <prashant.n@samsung.com> wrote:

> @enne, I added the data in bug for Size, the same results we will get for
> Point,
> Vector as both are using 2 ints. For TimeTicks and Time I'll check.
>

I'll wait fir your release build data for this.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[prashant.n, 2013-12-24 04:19:55]

The new results (release build) with different cases have been added in comment
#30, 31 to the bug.

[enne (OOO), 2014-01-02 18:42:56]

Thanks for all of your hard work investigating and gathering data.  I feel very
convinced by your conclusions that >4 byte objects should be passed by ref and
not by value.

Can you change over all of cc for all of these types, rather than just changing
two files? I would really prefer not to leave cc in a mixed state where some
functions pass a type by value and others pass it by ref.  Adding the opposite
presubmit rule might make it easy to catch most of them.

https://codereview.chromium.org/93663004/diff/1/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (left):

https://codereview.chromium.org/93663004/diff/1/cc/PRESUBMIT.py#oldcode105
cc/PRESUBMIT.py:105: def CheckPassByValue(input_api,
The presubmit here was nice because it communicated to everyone what was desired
without having to depend on code review to catch it.

Do you think it'd be plausible to come up with something that could check
headers for "gfx::Rect identifier" inside of parentheses that aren't "const
gfx::Rect& identifier"?

[enne (OOO), 2014-01-06 19:32:14]

As per your email, if you don't want to make this change at once, then I'd
prefer one patch to remove the presubmit, then n patches to update the code, and
one patch to add a new presubmit.

[prashant.n, 2014-01-09 14:28:06]

PTAL

[enne (OOO), 2014-01-09 18:24:11]

lgtm

[commit-bot: I haz the power, 2014-01-09 18:27:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/93663004/70001

[commit-bot: I haz the power, 2014-01-09 18:28:26]

Failed to apply patch for cc/resources/resource_update.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/resource_update.cc
  Hunk #1 succeeded at 10 with fuzz 2 (offset -2 lines).
  Hunk #2 FAILED at 28.
  1 out of 2 hunks FAILED -- saving rejects to file
cc/resources/resource_update.cc.rej

Patch:       cc/resources/resource_update.cc
Index: cc/resources/resource_update.cc
diff --git a/cc/resources/resource_update.cc b/cc/resources/resource_update.cc
index
5760ddf4a0dde663b3fdb6bb336d66638c1dc37d..85ba128196cee81a798718d78a806441f2c8db3d
100644
--- a/cc/resources/resource_update.cc
+++ b/cc/resources/resource_update.cc
@@ -12,8 +12,8 @@ namespace cc {
 
 ResourceUpdate ResourceUpdate::Create(PrioritizedResource* texture,
                                       const SkBitmap* bitmap,
-                                      gfx::Rect content_rect,
-                                      gfx::Rect source_rect,
+                                      const gfx::Rect& content_rect,
+                                      const gfx::Rect& source_rect,
                                       gfx::Vector2d dest_offset) {
   CHECK(content_rect.Contains(source_rect));
   ResourceUpdate update;
@@ -28,8 +28,8 @@ ResourceUpdate ResourceUpdate::Create(PrioritizedResource*
texture,
 ResourceUpdate ResourceUpdate::CreateFromCanvas(
     PrioritizedResource* resource,
     const skia::RefPtr<SkCanvas>& canvas,
-    gfx::Rect content_rect,
-    gfx::Rect source_rect,
+    const gfx::Rect& content_rect,
+    const gfx::Rect& source_rect,
     gfx::Vector2d dest_offset) {
   CHECK(content_rect.Contains(source_rect));
   ResourceUpdate update;

[prashant.n, 2014-01-10 04:38:22]

Merged the other part of the change (present in CL 131223002) for complete
change to avoid compilation errors on other platforms & avoid merge issues.
Added piman also for OWNER for gpu/command_buffer/ files.

[prashant.n, 2014-01-10 04:38:43]

PTAL

[piman, 2014-01-10 04:51:01]

LGTM if this is the current consensus.

[commit-bot: I haz the power, 2014-01-10 04:57:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/93663004/140001

[commit-bot: I haz the power, 2014-01-10 04:58:16]

Failed to apply patch for cc/resources/resource_update.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/resource_update.cc
  Hunk #1 succeeded at 10 with fuzz 2 (offset -2 lines).
  Hunk #2 FAILED at 28.
  1 out of 2 hunks FAILED -- saving rejects to file
cc/resources/resource_update.cc.rej

Patch:       cc/resources/resource_update.cc
Index: cc/resources/resource_update.cc
diff --git a/cc/resources/resource_update.cc b/cc/resources/resource_update.cc
index
5760ddf4a0dde663b3fdb6bb336d66638c1dc37d..85ba128196cee81a798718d78a806441f2c8db3d
100644
--- a/cc/resources/resource_update.cc
+++ b/cc/resources/resource_update.cc
@@ -12,8 +12,8 @@ namespace cc {
 
 ResourceUpdate ResourceUpdate::Create(PrioritizedResource* texture,
                                       const SkBitmap* bitmap,
-                                      gfx::Rect content_rect,
-                                      gfx::Rect source_rect,
+                                      const gfx::Rect& content_rect,
+                                      const gfx::Rect& source_rect,
                                       gfx::Vector2d dest_offset) {
   CHECK(content_rect.Contains(source_rect));
   ResourceUpdate update;
@@ -28,8 +28,8 @@ ResourceUpdate ResourceUpdate::Create(PrioritizedResource*
texture,
 ResourceUpdate ResourceUpdate::CreateFromCanvas(
     PrioritizedResource* resource,
     const skia::RefPtr<SkCanvas>& canvas,
-    gfx::Rect content_rect,
-    gfx::Rect source_rect,
+    const gfx::Rect& content_rect,
+    const gfx::Rect& source_rect,
     gfx::Vector2d dest_offset) {
   CHECK(content_rect.Contains(source_rect));
   ResourceUpdate update;

[commit-bot: I haz the power, 2014-01-10 06:46:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/93663004/220001

[commit-bot: I haz the power, 2014-01-10 07:15:26]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[prashant.n, 2014-01-10 08:05:52]

Can somebody please rubberstamp this cl as following new files have been added
for fixing clang build break.
content/renderer/gpu/compositor_software_output_device.h
content/renderer/gpu/compositor_software_output_device.cc

[prashant.n, 2014-01-10 12:13:48]

Can somebody please rubberstamp the change again? Added @bulach for LGTM for
content/browser/android/ file.

New files have been added for fixing build on non-linux platforms : 

content/browser/android/in_process/synchronous_compositor_output_surface.cc
content/browser/aura/software_output_device_ozone.h
content/browser/aura/software_output_device_ozone.cc
content/browser/aura/software_output_device_win.h
content/browser/aura/software_output_device_win.cc
content/renderer/gpu/compositor_software_output_device.h
content/renderer/gpu/compositor_software_output_device.cc

[prashant.n, 2014-01-10 12:14:24]

Added @bulach for LGTM to content/browser/android file.

[bulach, 2014-01-10 13:45:43]

lgtm for
content/browser/android/in_process/synchronous_compositor_output_surface.cc,
thanks!

[commit-bot: I haz the power, 2014-01-10 18:00:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/93663004/520002

[commit-bot: I haz the power, 2014-01-10 20:05:09]

Change committed as 244224