ui: Make gfx::Size::Scale() mutate the class. Add gfx::ScaleSize() similar to Rect/Point.

ui: Make gfx::Size::Scale() mutate the class. Add gfx::ScaleSize() similar to Rect/Point.

Covered by existing unit tests.

R=sky
BUG=160158
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167077

[tfarina, 2012-11-09 13:05:37]

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h
File ui/gfx/rect_f.h (left):

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h#oldcode31
ui/gfx/rect_f.h:31: /// Scales the rectangle by |scale|.
nit: just two //

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h#oldcode39
ui/gfx/rect_f.h:39: SizeF newSize = size().Scale(x_scale, y_scale);
nit: newSize, tisk, tisk :(

s/newSize/new_size.

[tfarina, 2012-11-09 13:22:05]

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/size_f.h
File ui/gfx/size_f.h (right):

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/size_f.h#newcode41
ui/gfx/size_f.h:41: UI_EXPORT SizeF ScaleSize(const SizeF& p, float x_scale,
float y_scale);
why we want these free functions? The member functions aren't enough?

[Ian Vollick, 2012-11-09 13:33:30]

On 2012/11/09 13:22:05, tfarina wrote:
> https://codereview.chromium.org/11377068/diff/2001/ui/gfx/size_f.h
> File ui/gfx/size_f.h (right):
> 
> https://codereview.chromium.org/11377068/diff/2001/ui/gfx/size_f.h#newcode41
> ui/gfx/size_f.h:41: UI_EXPORT SizeF ScaleSize(const SizeF& p, float x_scale,
> float y_scale);
> why we want these free functions? The member functions aren't enough?
Isn't the idea that member functions consistently mutate the class, and we add
free functions when we need to return new values?

[danakj, 2012-11-09 16:09:33]

On Fri, Nov 9, 2012 at 8:33 AM,  <vollick@chromium.org> wrote:
> On 2012/11/09 13:22:05, tfarina wrote:
>>
>> https://codereview.chromium.org/11377068/diff/2001/ui/gfx/size_f.h
>> File ui/gfx/size_f.h (right):
>
>
>>
>> https://codereview.chromium.org/11377068/diff/2001/ui/gfx/size_f.h#newcode41
>> ui/gfx/size_f.h:41: UI_EXPORT SizeF ScaleSize(const SizeF& p, float
>> x_scale,
>> float y_scale);
>> why we want these free functions? The member functions aren't enough?
>
> Isn't the idea that member functions consistently mutate the class, and we
> add
> free functions when we need to return new values?

Yes. Like Rect and Vector2d. Now these are inconsistent/missing, and
people are already mixing things up in cc/ since I left size/point
alone so far.

[danakj, 2012-11-09 16:11:38]

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h
File ui/gfx/rect_f.h (left):

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h#oldcode31
ui/gfx/rect_f.h:31: /// Scales the rectangle by |scale|.
On 2012/11/09 13:05:37, tfarina wrote:
> nit: just two //

Done.

https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h#oldcode39
ui/gfx/rect_f.h:39: SizeF newSize = size().Scale(x_scale, y_scale);
On 2012/11/09 13:05:37, tfarina wrote:
> nit: newSize, tisk, tisk :(
> 
> s/newSize/new_size.

I'm removing this code.

[tfarina, 2012-11-09 17:47:33]

On Fri, Nov 9, 2012 at 2:11 PM,  <danakj@chromium.org> wrote:
> https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h#oldcode39
> ui/gfx/rect_f.h:39: SizeF newSize = size().Scale(x_scale, y_scale);
> On 2012/11/09 13:05:37, tfarina wrote:
>>
>> nit: newSize, tisk, tisk :(
>
>> s/newSize/new_size.
>
> I'm removing this code.
>
I know, it was just a reminder.

-- 
Thiago

[danakj, 2012-11-09 18:18:03]

On Fri, Nov 9, 2012 at 12:47 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Fri, Nov 9, 2012 at 2:11 PM,  <danakj@chromium.org> wrote:
>> https://codereview.chromium.org/11377068/diff/2001/ui/gfx/rect_f.h#oldcode39
>> ui/gfx/rect_f.h:39: SizeF newSize = size().Scale(x_scale, y_scale);
>> On 2012/11/09 13:05:37, tfarina wrote:
>>>
>>> nit: newSize, tisk, tisk :(
>>
>>> s/newSize/new_size.
>>
>> I'm removing this code.
>>
> I know, it was just a reminder.

I see, I added this in the previous CL, I wasn't sure :)

[danakj, 2012-11-09 21:17:31]

+jamesr for cc/ and webkit/

[danakj, 2012-11-09 21:18:12]

(oh, and content/renderer please)

[jamesr, 2012-11-09 21:49:54]

lgtm

[danakj, 2012-11-09 22:27:22]

ping for sky, in case it got lost in the patch flood.

[sky, 2012-11-09 23:01:14]

LGTM

[commit-bot: I haz the power, 2012-11-09 23:08:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11377068/8032

[commit-bot: I haz the power, 2012-11-09 23:08:13]

Failed to apply patch for cc/layer_tree_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host_impl.cc
  Hunk #2 FAILED at 299.
  Hunk #3 succeeded at 986 (offset 8 lines).
  Hunk #4 succeeded at 999 (offset 8 lines).
  Hunk #5 succeeded at 1289 (offset 8 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
cc/layer_tree_host_impl.cc.rej

Patch:       cc/layer_tree_host_impl.cc
Index: cc/layer_tree_host_impl.cc
diff --git a/cc/layer_tree_host_impl.cc b/cc/layer_tree_host_impl.cc
index
c9c0820218f08db66d8956e3614006a376aef2fe..4739e9aa36a162bddbfe77b07101393800564c1c
100644
--- a/cc/layer_tree_host_impl.cc
+++ b/cc/layer_tree_host_impl.cc
@@ -100,12 +100,9 @@ bool PinchZoomViewport::setPageScaleFactorAndLimits(float
pageScaleFactor, float
 
 gfx::RectF PinchZoomViewport::bounds() const
 {
-    gfx::SizeF scaledViewportSize = m_layoutViewportSize;
-    scaledViewportSize = scaledViewportSize.Scale(1 / totalPageScaleFactor());
-
-    gfx::RectF bounds(gfx::PointF(), scaledViewportSize);
+    gfx::RectF bounds(gfx::PointF(), m_layoutViewportSize);
+    bounds.Scale(1 / totalPageScaleFactor());
     bounds.Offset(m_pinchViewportScrollDelta);
-
     return bounds;
 }
 
@@ -302,7 +299,7 @@ void
LayerTreeHostImpl::startPageScaleAnimation(gfx::Vector2d targetOffset, bool
     gfx::Vector2dF scrollTotal = m_rootScrollLayerImpl->scrollOffset() +
m_rootScrollLayerImpl->scrollDelta();
     scrollTotal.Scale(m_pinchZoomViewport.pageScaleDelta());
     float scaleTotal = m_pinchZoomViewport.totalPageScaleFactor();
-    gfx::Size scaledContentSize =
gfx::ToFlooredSize(contentSize().Scale(m_pinchZoomViewport.pageScaleDelta()));
+    gfx::Size scaledContentSize =
gfx::ToFlooredSize(gfx::ScaleSize(contentSize(),
m_pinchZoomViewport.pageScaleDelta()));
 
     double startTimeSeconds = (startTime - base::TimeTicks()).InSecondsF();
     m_pageScaleAnimation =
PageScaleAnimation::create(gfx::ToFlooredVector2d(scrollTotal), scaleTotal,
m_deviceViewportSize, scaledContentSize, startTimeSeconds);
@@ -981,10 +978,8 @@ void LayerTreeHostImpl::updateMaxScrollOffset()
     gfx::SizeF viewBounds = m_deviceViewportSize;
     if (LayerImpl* clipLayer = m_rootScrollLayerImpl->parent()) {
         // Compensate for non-overlay scrollbars.
-        if (clipLayer->masksToBounds()) {
-            viewBounds = clipLayer->bounds();
-            viewBounds = viewBounds.Scale(m_deviceScaleFactor);
-        }
+        if (clipLayer->masksToBounds())
+            viewBounds = gfx::ScaleSize(clipLayer->bounds(),
m_deviceScaleFactor);
     }
 
     gfx::Size contentBounds = contentSize();
@@ -996,7 +991,7 @@ void LayerTreeHostImpl::updateMaxScrollOffset()
         contentBounds.set_width(contentBounds.width() / pageScaleFactor);
         contentBounds.set_height(contentBounds.height() / pageScaleFactor);
     } else {
-        viewBounds = viewBounds.Scale(1 /
m_pinchZoomViewport.pageScaleDelta());
+        viewBounds.Scale(1 / m_pinchZoomViewport.pageScaleDelta());
     }
 
     gfx::Vector2dF maxScroll = BottomRight(gfx::Rect(contentBounds)) -
BottomRight(gfx::RectF(viewBounds));
@@ -1286,7 +1281,7 @@ void
LayerTreeHostImpl::computePinchZoomDeltas(ScrollAndScaleSet* scrollInfo)
     scrollBegin.Scale(m_pinchZoomViewport.pageScaleDelta());
     float scaleBegin = m_pinchZoomViewport.totalPageScaleFactor();
     float pageScaleDeltaToSend = m_pinchZoomViewport.minPageScaleFactor() /
m_pinchZoomViewport.pageScaleFactor();
-    gfx::SizeF scaledContentsSize = contentSize().Scale(pageScaleDeltaToSend);
+    gfx::SizeF scaledContentsSize = gfx::ScaleSize(contentSize(),
pageScaleDeltaToSend);
 
     gfx::Vector2d anchorOffset = m_previousPinchAnchor.OffsetFromOrigin();
     gfx::Vector2dF scrollEnd = scrollBegin + anchorOffset;

[commit-bot: I haz the power, 2012-11-09 23:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11377068/11001

[commit-bot: I haz the power, 2012-11-10 00:07:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-10 00:16:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11377068/9004

[commit-bot: I haz the power, 2012-11-10 05:26:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11377068/72

[commit-bot: I haz the power, 2012-11-10 05:26:18]

Failed to apply patch for cc/draw_quad_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/draw_quad_unittest.cc
  Hunk #1 FAILED at 238.
  1 out of 1 hunk FAILED -- saving rejects to file cc/draw_quad_unittest.cc.rej

Patch:       cc/draw_quad_unittest.cc
Index: cc/draw_quad_unittest.cc
diff --git a/cc/draw_quad_unittest.cc b/cc/draw_quad_unittest.cc
index
a73bbecbe9a3f4ebbd0673ad9aee76be86e9dbc7..ffdeccc025d7223ff2d693dd57b097500f238e07
100644
--- a/cc/draw_quad_unittest.cc
+++ b/cc/draw_quad_unittest.cc
@@ -238,7 +238,7 @@ TEST(DrawQuadTest, copyTextureDrawQuad)
 {
     unsigned resourceId = 82;
     bool premultipliedAlpha = true;
-    gfx::RectF uvRect(0.5, 224, -51, 36);
+    gfx::RectF uvRect(0.5, 224, 51, 36);
     bool flipped = true;
 
     CREATE_SHARED_STATE();

[commit-bot: I haz the power, 2012-11-10 05:30:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11377068/14006

[commit-bot: I haz the power, 2012-11-10 06:13:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-10 06:28:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11377068/17005

[commit-bot: I haz the power, 2012-11-10 09:34:31]

Change committed as 167077