Make scroll offset type of float in cc

Use ScrollOffset instead of vector2d to track scroll offset in cc

This is for preparing converting blink side scroll offset to be double
type. Assuming blink side starts to use double scroll offset, it can
be passed back from main/blink to compositor without converting to int
type.

BUG=414283
Committed: https://crrev.com/f57925d399f2eda19fdaab9191257e51a3af04a0
Cr-Commit-Position: refs/heads/master@{#297692}

[Yufeng Shen (Slow to review), 2014-09-18 20:38:18]

miletus@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-09-18 21:21:52]

Left a comment on the bug about using floats for this, I'd like to discuss about
that.

https://codereview.chromium.org/584503005/diff/20001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/584503005/diff/20001/cc/blink/web_layer_impl....
cc/blink/web_layer_impl.h:105: // TODO(miletus@): Remove the WebPoint version
once blink side starts to
nit: no @

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer.h#newcod...
cc/layers/layer.h:264: void SetScrollOffset(gfx::Vector2dF scroll_offset);
this should be a const& while you're here

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer_impl_uni...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer_impl_uni...
cc/layers/layer_impl_unittest.cc:452: EXPECT_VECTOR2DF_EQ(gfx::Vector2dF(),
layer()->TotalScrollOffset());
you can just use EXPECT_EQ for these now

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1110: // TODO(miletus@): make viewport delta
Vector2DF.
nit: no @, capital M

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2549: LOG(ERROR) << "consumed_by top_control";
remove

[danakj, 2014-09-18 21:23:49]

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:213: EXPECT_TRUE(std::abs(diff_x) <
0.1e-10);
You can use EXPECT_FLOAT_EQ for this probably. Or EXPECT_NEAR

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:229: EXPECT_VECTOR2DF_EQ(scroll_delta,
scroll_info.scrolls[i].scroll_delta);
EXPECT_EQ?

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_scroll.cc:55:
EXPECT_VECTOR2DF_EQ(initial_scroll_, scroll_layer->scroll_offset());
EXPECT_EQ?

[Yufeng Shen (Slow to review), 2014-09-18 22:47:56]

https://codereview.chromium.org/584503005/diff/20001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/584503005/diff/20001/cc/blink/web_layer_impl....
cc/blink/web_layer_impl.h:105: // TODO(miletus@): Remove the WebPoint version
once blink side starts to
On 2014/09/18 21:21:52, danakj wrote:
> nit: no @

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer.h#newcod...
cc/layers/layer.h:264: void SetScrollOffset(gfx::Vector2dF scroll_offset);
On 2014/09/18 21:21:52, danakj wrote:
> this should be a const& while you're here

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer_impl_uni...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/layers/layer_impl_uni...
cc/layers/layer_impl_unittest.cc:452: EXPECT_VECTOR2DF_EQ(gfx::Vector2dF(),
layer()->TotalScrollOffset());
On 2014/09/18 21:21:52, danakj wrote:
> you can just use EXPECT_EQ for these now

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1110: // TODO(miletus@): make viewport delta
Vector2DF.
On 2014/09/18 21:21:52, danakj wrote:
> nit: no @, capital M

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2549: LOG(ERROR) << "consumed_by top_control";
On 2014/09/18 21:21:52, danakj wrote:
> remove

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:213: EXPECT_TRUE(std::abs(diff_x) <
0.1e-10);
On 2014/09/18 21:23:49, danakj wrote:
> You can use EXPECT_FLOAT_EQ for this probably. Or EXPECT_NEAR

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:229: EXPECT_VECTOR2DF_EQ(scroll_delta,
scroll_info.scrolls[i].scroll_delta);
On 2014/09/18 21:23:49, danakj wrote:
> EXPECT_EQ?

Done.

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/584503005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_scroll.cc:55:
EXPECT_VECTOR2DF_EQ(initial_scroll_, scroll_layer->scroll_offset());
On 2014/09/18 21:23:49, danakj wrote:
> EXPECT_EQ?

Done.

[Yufeng Shen (Slow to review), 2014-09-23 21:38:04]

Updated to use ScrollOffset (internally using double) to track the scroll offset
in cc.

Adding/fixing tests will be followed.


Dana, PTAL, thanks.

[danakj, 2014-09-25 15:13:31]

https://codereview.chromium.org/584503005/diff/80001/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/584503005/diff/80001/cc/blink/web_layer_impl....
cc/blink/web_layer_impl.cc:272: const std::pair<double, double>& position) {
are you allowed to use std::pair in the blink public API? i guess you'll sort
that out on that side of the fence

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:313: gfx::Vector2d ScrollDelta() const { return
gfx::Vector2d(); }
This is Vector2dF on the impl side, can you make this match?

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:314: gfx::Vector2dF TotalScrollOffset() const {
TODO to change this return type to ScrollOffset?

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:370: gfx::Vector2dF max_delta = MaxScrollOffset() -
ToVector2dF(scroll_offset_);
to ToVector2dF after the subtraction - MaxScrollOffset should be a ScrollOffset
type..

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:385: gfx::Vector2d MaxScrollOffset() const;
Should this and ClampScroll be ScrollOffsets?

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:392: gfx::Vector2dF TotalScrollOffset() const;
TODO to change to ScrollOffset?

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:394: void SetSentScrollDelta(const gfx::ScrollOffset&
sent_scroll_delta);
Why isn't the delta a Vector2dF?

We should be consistent, all deltas (relative to an offset) are one thing all
offsets (relative to origin) are one thing.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:653: gfx::ScrollOffset sent_scroll_delta_;
impl-side deltas are Vector2dF can this be that?

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:654: gfx::Vector2dF last_scroll_offset_;
shouldnt this be a ScrollOffset?

https://codereview.chromium.org/584503005/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1069: gfx::ScrollOffset inner_viewport_scroll_delta;
these are deltas (relative to an offset) not an offset (relative to origin) from
the name, so why the change?

https://codereview.chromium.org/584503005/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1109: // TODO(miletus): Make viewport delta
ScrollOffset.
Do you think all deltas should be ScrollOffset type?

https://codereview.chromium.org/584503005/diff/80001/ui/gfx/geometry/scroll_o...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/584503005/diff/80001/ui/gfx/geometry/scroll_o...
ui/gfx/geometry/scroll_offset.h:68: inline Vector2d ToFlooredVector2d(const
ScrollOffset& v) {
make this a member

https://codereview.chromium.org/584503005/diff/80001/ui/gfx/geometry/scroll_o...
ui/gfx/geometry/scroll_offset.h:74: inline Vector2dF ToVector2dF(const
ScrollOffset& v) {
make a member

[Yufeng Shen (Slow to review), 2014-09-25 20:06:24]

Thanks for the review.  PTAL

https://codereview.chromium.org/584503005/diff/80001/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/584503005/diff/80001/cc/blink/web_layer_impl....
cc/blink/web_layer_impl.cc:272: const std::pair<double, double>& position) {
On 2014/09/25 15:13:30, danakj wrote:
> are you allowed to use std::pair in the blink public API? i guess you'll sort
> that out on that side of the fence

right, it is suggested to use a new type WebDoublePoint.

I will sync this patch with the patch that adds this new type
to blink side.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:313: gfx::Vector2d ScrollDelta() const { return
gfx::Vector2d(); }
On 2014/09/25 15:13:30, danakj wrote:
> This is Vector2dF on the impl side, can you make this match?

Done.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:314: gfx::Vector2dF TotalScrollOffset() const {
On 2014/09/25 15:13:30, danakj wrote:
> TODO to change this return type to ScrollOffset?

let me do this in this patch

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:370: gfx::Vector2dF max_delta = MaxScrollOffset() -
ToVector2dF(scroll_offset_);
On 2014/09/25 15:13:30, danakj wrote:
> to ToVector2dF after the subtraction - MaxScrollOffset should be a
ScrollOffset
> type..

Done.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:385: gfx::Vector2d MaxScrollOffset() const;
On 2014/09/25 15:13:31, danakj wrote:
> Should this and ClampScroll be ScrollOffsets?

Done.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:392: gfx::Vector2dF TotalScrollOffset() const;
On 2014/09/25 15:13:31, danakj wrote:
> TODO to change to ScrollOffset?

done in this patch.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:394: void SetSentScrollDelta(const gfx::ScrollOffset&
sent_scroll_delta);
On 2014/09/25 15:13:30, danakj wrote:
> Why isn't the delta a Vector2dF?
> 
> We should be consistent, all deltas (relative to an offset) are one thing all
> offsets (relative to origin) are one thing.

right, I change all the scroll delta to Vector2dF, and all the scroll offset
to be ScrollOffset.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:653: gfx::ScrollOffset sent_scroll_delta_;
On 2014/09/25 15:13:30, danakj wrote:
> impl-side deltas are Vector2dF can this be that?

Done.

https://codereview.chromium.org/584503005/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:654: gfx::Vector2dF last_scroll_offset_;
On 2014/09/25 15:13:31, danakj wrote:
> shouldnt this be a ScrollOffset?

Done.

https://codereview.chromium.org/584503005/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1069: gfx::ScrollOffset inner_viewport_scroll_delta;
On 2014/09/25 15:13:31, danakj wrote:
> these are deltas (relative to an offset) not an offset (relative to origin)
from
> the name, so why the change?

Done.

https://codereview.chromium.org/584503005/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1109: // TODO(miletus): Make viewport delta
ScrollOffset.
On 2014/09/25 15:13:31, danakj wrote:
> Do you think all deltas should be ScrollOffset type?

delta changed to be vector2dF to be consistent.

https://codereview.chromium.org/584503005/diff/80001/ui/gfx/geometry/scroll_o...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/584503005/diff/80001/ui/gfx/geometry/scroll_o...
ui/gfx/geometry/scroll_offset.h:68: inline Vector2d ToFlooredVector2d(const
ScrollOffset& v) {
On 2014/09/25 15:13:31, danakj wrote:
> make this a member 

Done.

https://codereview.chromium.org/584503005/diff/80001/ui/gfx/geometry/scroll_o...
ui/gfx/geometry/scroll_offset.h:74: inline Vector2dF ToVector2dF(const
ScrollOffset& v) {
On 2014/09/25 15:13:31, danakj wrote:
> make a member

Done.

[danakj, 2014-09-25 22:01:27]

Thanks, I think this looks really nice overall

https://codereview.chromium.org/584503005/diff/100001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:547:
current_scroll_offset.ToVector2dF());
ScrollOffsetAnimationCurve should be ScrollOffsets?
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/scrol...

https://codereview.chromium.org/584503005/diff/100001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/584503005/diff/100001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.h:108: virtual void
setScrollPositionFloat(blink::WebDoublePoint position);
const&

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:968: layer->ScrollDelta() - layer->sent_scroll_delta());;
double;;

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:369: gfx::Vector2dF min_delta =
(-scroll_offset_).ToVector2dF();
drop the (), should still work right?

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:756: void LayerImpl::OnScrollOffsetAnimated(const
gfx::Vector2dF& scroll_offset) {
this should be a ScrollOffset?

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1217: gfx::ScrollOffset
LayerImpl::ClampScrollToMaxScrollOffset() {
This looks like a delta, it's an offset minus an offset, so let's make this
Vector2dF?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1103:
gfx::ScrollOffset(inner_viewport_scroll_delta));;
double;;

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1107:
gfx::ScrollOffset(outer_viewport_scroll_delta));
Throwing this out as an idea, idk if it's a good one. I see lots of 

offset + gfx::ScrollOffset(delta)

we could have a

gfx::ScrollOffset ScrollOffsetWithDelta(offset, delta) that did this. It would
actually make a bit more characters at call sites, though. Would it be more
clear? What do you think?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1114:
client_->ApplyViewportDeltas(gfx::ToFlooredVector2d(viewport_scroll_delta),
why flooring here? not sure why you changed anything here

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.h:16: #include "ui/gfx/geometry/scroll_offset.h"
not needed? move to .cc

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:446:
PageScaleAnimation::Create(scroll_total.ToVector2dF(),
TODO to pass a ScrollOffset?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1486: metadata.root_scroll_offset =
active_tree_->TotalScrollOffset().ToVector2dF();
TODO to change the IPC to hold a ScrollOffset?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2378: gfx::Vector2dF new_target =
curve->target_value() + scroll_delta;
target_value should be a ScrollOffset? So a bunch of things here should be?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2384: new_target);
UpdateTarget should be a ScrollOffset?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2420:
ScrollOffsetAnimationCurve::Create(target_offset.ToVector2dF(),
should just pass a ScrollOffset?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2422:
curve->SetInitialValue(current_offset.ToVector2dF());
and here?

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2886: gfx::ScrollOffset max_offset =
This would become Vector2dF based on other comments

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2983: gfx::Vector2dF next_scroll =
ScrollOffset

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2986: ScrollViewportBy(next_scroll -
scroll_total.ToVector2dF());
convert after subtracting

https://codereview.chromium.org/584503005/diff/100001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/584503005/diff/100001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:286:
compositor_client_->UpdateRootLayerState(total_scroll_offset.ToVector2dF(),
these look like they should stay ScrollOffsets?

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:14: class GFX_EXPORT ScrollOffset {
Please add PrintTo like we have for other geometry types so EXPECT_EQ works. See
https://codereview.chromium.org/368903003/ for where this was added.

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:16: ScrollOffset() : x_(0), y_(0) {}
Can you add some unit tests for this class, similar to what exists for Vector2d?

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:55: Vector2d ToFlooredVector2d() const {
Sorry, can I take this back? ._. Now that I see how it looks like this, I think
you were right before. Can you move them back out, but give them a name to
better scope them, like ScrollOffsetToFlooredVector2d() and
ScrollOffsetToVector2dF()>?

[Yufeng Shen (Slow to review), 2014-09-25 22:15:01]

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1107:
gfx::ScrollOffset(outer_viewport_scroll_delta));
On 2014/09/25 22:01:26, danakj wrote:
> Throwing this out as an idea, idk if it's a good one. I see lots of 
> 
> offset + gfx::ScrollOffset(delta)
> 
> we could have a
> 
> gfx::ScrollOffset ScrollOffsetWithDelta(offset, delta) that did this. It would
> actually make a bit more characters at call sites, though. Would it be more
> clear? What do you think?

what do you think if I have an overload

ScrollOffset operator+(const ScrollOffset& lhs, const Vector2dF& rhs) ?
ScrollOffset operator+(const ScrollOffset& lhs, const Vector2d& rhs)

there are a lot of calls in the unittest files that have this 
offset + delta or offset + delta + delta pattern
having these overload will make the change much simpler.

[danakj, 2014-09-25 22:30:52]

On Thu, Sep 25, 2014 at 6:15 PM, <miletus@chromium.org> wrote:

>
> https://codereview.chromium.org/584503005/diff/100001/cc/
> trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
>
> https://codereview.chromium.org/584503005/diff/100001/cc/
> trees/layer_tree_host.cc#newcode1107
> cc/trees/layer_tree_host.cc:1107:
> gfx::ScrollOffset(outer_viewport_scroll_delta));
> On 2014/09/25 22:01:26, danakj wrote:
>
>> Throwing this out as an idea, idk if it's a good one. I see lots of
>>
>
>  offset + gfx::ScrollOffset(delta)
>>
>
>  we could have a
>>
>
>  gfx::ScrollOffset ScrollOffsetWithDelta(offset, delta) that did this.
>>
> It would
>
>> actually make a bit more characters at call sites, though. Would it be
>>
> more
>
>> clear? What do you think?
>>
>
> what do you think if I have an overload
>
> ScrollOffset operator+(const ScrollOffset& lhs, const Vector2dF& rhs) ?
> ScrollOffset operator+(const ScrollOffset& lhs, const Vector2d& rhs)
>
> there are a lot of calls in the unittest files that have this
> offset + delta or offset + delta + delta pattern
> having these overload will make the change much simpler.
>

I thought about it but I regret the implicit int->float conversions we did
for other types, so I think it should be more explicit with a function
name, not an operator.

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

[chromium-reviews, 2014-09-25 22:32:31]

On Thu, Sep 25, 2014 at 6:30 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Sep 25, 2014 at 6:15 PM, <miletus@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/584503005/diff/100001/cc/
>> trees/layer_tree_host.cc
>> File cc/trees/layer_tree_host.cc (right):
>>
>> https://codereview.chromium.org/584503005/diff/100001/cc/
>> trees/layer_tree_host.cc#newcode1107
>> cc/trees/layer_tree_host.cc:1107:
>> gfx::ScrollOffset(outer_viewport_scroll_delta));
>> On 2014/09/25 22:01:26, danakj wrote:
>>
>>> Throwing this out as an idea, idk if it's a good one. I see lots of
>>>
>>
>>  offset + gfx::ScrollOffset(delta)
>>>
>>
>>  we could have a
>>>
>>
>>  gfx::ScrollOffset ScrollOffsetWithDelta(offset, delta) that did this.
>>>
>> It would
>>
>>> actually make a bit more characters at call sites, though. Would it be
>>>
>> more
>>
>>> clear? What do you think?
>>>
>>
>> what do you think if I have an overload
>>
>> ScrollOffset operator+(const ScrollOffset& lhs, const Vector2dF& rhs) ?
>> ScrollOffset operator+(const ScrollOffset& lhs, const Vector2d& rhs)
>>
>> there are a lot of calls in the unittest files that have this
>> offset + delta or offset + delta + delta pattern
>> having these overload will make the change much simpler.
>>
>
> I thought about it but I regret the implicit int->float conversions we did
> for other types, so I think it should be more explicit with a function
> name, not an operator.
>


But this is int/float -> double conversion and there is no loss of
precision happening here.

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

[danakj, 2014-09-25 22:34:54]

On Thu, Sep 25, 2014 at 6:32 PM, Yufeng Shen <miletus@google.com> wrote:

>
>
> On Thu, Sep 25, 2014 at 6:30 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Thu, Sep 25, 2014 at 6:15 PM, <miletus@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/584503005/diff/100001/cc/
>>> trees/layer_tree_host.cc
>>> File cc/trees/layer_tree_host.cc (right):
>>>
>>> https://codereview.chromium.org/584503005/diff/100001/cc/
>>> trees/layer_tree_host.cc#newcode1107
>>> cc/trees/layer_tree_host.cc:1107:
>>> gfx::ScrollOffset(outer_viewport_scroll_delta));
>>> On 2014/09/25 22:01:26, danakj wrote:
>>>
>>>> Throwing this out as an idea, idk if it's a good one. I see lots of
>>>>
>>>
>>>  offset + gfx::ScrollOffset(delta)
>>>>
>>>
>>>  we could have a
>>>>
>>>
>>>  gfx::ScrollOffset ScrollOffsetWithDelta(offset, delta) that did this.
>>>>
>>> It would
>>>
>>>> actually make a bit more characters at call sites, though. Would it be
>>>>
>>> more
>>>
>>>> clear? What do you think?
>>>>
>>>
>>> what do you think if I have an overload
>>>
>>> ScrollOffset operator+(const ScrollOffset& lhs, const Vector2dF& rhs) ?
>>> ScrollOffset operator+(const ScrollOffset& lhs, const Vector2d& rhs)
>>>
>>> there are a lot of calls in the unittest files that have this
>>> offset + delta or offset + delta + delta pattern
>>> having these overload will make the change much simpler.
>>>
>>
>> I thought about it but I regret the implicit int->float conversions we
>> did for other types, so I think it should be more explicit with a function
>> name, not an operator.
>>
>
>
> But this is int/float -> double conversion and there is no loss of
> precision happening here.
>

Right, it's a safe operation. But converting is actually more expensive
than I thought previously, so making it explicit is good so it doesn't
happen at times you didn't expect.

For instance there's places in Layer that were returning a Vector2d, but
places using it would expect a Vector2dF. Now we have an implicit
conversion for no reason, but we'd see that if it was explicit.

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

[Yufeng Shen (Slow to review), 2014-09-26 20:19:08]

The change is really contagious. Lets defer to TODO for any further
non-essential change. 

PTAL, thanks.

https://codereview.chromium.org/584503005/diff/100001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:547:
current_scroll_offset.ToVector2dF());
On 2014/09/25 22:01:26, danakj wrote:
> ScrollOffsetAnimationCurve should be ScrollOffsets?
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/scrol...

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/584503005/diff/100001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.h:108: virtual void
setScrollPositionFloat(blink::WebDoublePoint position);
On 2014/09/25 22:01:26, danakj wrote:
> const&

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:968: layer->ScrollDelta() - layer->sent_scroll_delta());;
On 2014/09/25 22:01:26, danakj wrote:
> double;;

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:369: gfx::Vector2dF min_delta =
(-scroll_offset_).ToVector2dF();
On 2014/09/25 22:01:26, danakj wrote:
> drop the (), should still work right?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:756: void LayerImpl::OnScrollOffsetAnimated(const
gfx::Vector2dF& scroll_offset) {
On 2014/09/25 22:01:26, danakj wrote:
> this should be a ScrollOffset?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1217: gfx::ScrollOffset
LayerImpl::ClampScrollToMaxScrollOffset() {
On 2014/09/25 22:01:26, danakj wrote:
> This looks like a delta, it's an offset minus an offset, so let's make this
> Vector2dF?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1103:
gfx::ScrollOffset(inner_viewport_scroll_delta));;
On 2014/09/25 22:01:26, danakj wrote:
> double;;

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1114:
client_->ApplyViewportDeltas(gfx::ToFlooredVector2d(viewport_scroll_delta),
On 2014/09/25 22:01:26, danakj wrote:
> why flooring here? not sure why you changed anything here

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.h:16: #include "ui/gfx/geometry/scroll_offset.h"
On 2014/09/25 22:01:26, danakj wrote:
> not needed? move to .cc

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:446:
PageScaleAnimation::Create(scroll_total.ToVector2dF(),
On 2014/09/25 22:01:27, danakj wrote:
> TODO to pass a ScrollOffset?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1486: metadata.root_scroll_offset =
active_tree_->TotalScrollOffset().ToVector2dF();
On 2014/09/25 22:01:27, danakj wrote:
> TODO to change the IPC to hold a ScrollOffset?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2378: gfx::Vector2dF new_target =
curve->target_value() + scroll_delta;
On 2014/09/25 22:01:27, danakj wrote:
> target_value should be a ScrollOffset? So a bunch of things here should be?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2384: new_target);
On 2014/09/25 22:01:27, danakj wrote:
> UpdateTarget should be a ScrollOffset?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2420:
ScrollOffsetAnimationCurve::Create(target_offset.ToVector2dF(),
On 2014/09/25 22:01:26, danakj wrote:
> should just pass a ScrollOffset?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2422:
curve->SetInitialValue(current_offset.ToVector2dF());
On 2014/09/25 22:01:27, danakj wrote:
> and here?

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2886: gfx::ScrollOffset max_offset =
On 2014/09/25 22:01:26, danakj wrote:
> This would become Vector2dF based on other comments

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2983: gfx::Vector2dF next_scroll =
On 2014/09/25 22:01:27, danakj wrote:
> ScrollOffset

Done.

https://codereview.chromium.org/584503005/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2986: ScrollViewportBy(next_scroll -
scroll_total.ToVector2dF());
On 2014/09/25 22:01:26, danakj wrote:
> convert after subtracting

Done.

https://codereview.chromium.org/584503005/diff/100001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/584503005/diff/100001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:286:
compositor_client_->UpdateRootLayerState(total_scroll_offset.ToVector2dF(),
On 2014/09/25 22:01:27, danakj wrote:
> these look like they should stay ScrollOffsets?

Done.

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:14: class GFX_EXPORT ScrollOffset {
On 2014/09/25 22:01:27, danakj wrote:
> Please add PrintTo like we have for other geometry types so EXPECT_EQ works.
See
> https://codereview.chromium.org/368903003/ for where this was added.

Done.

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:16: ScrollOffset() : x_(0), y_(0) {}
On 2014/09/25 22:01:27, danakj wrote:
> Can you add some unit tests for this class, similar to what exists for
Vector2d?

Done.

https://codereview.chromium.org/584503005/diff/100001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:55: Vector2d ToFlooredVector2d() const {
On 2014/09/25 22:01:27, danakj wrote:
> Sorry, can I take this back? ._. Now that I see how it looks like this, I
think
> you were right before. Can you move them back out, but give them a name to
> better scope them, like ScrollOffsetToFlooredVector2d() and
> ScrollOffsetToVector2dF()>?

Done.

[danakj, 2014-09-27 00:00:44]

Thanks! This looks really nice. I have one more suggestion. I see a lot of

delta = gfx::ScrollOffsetToVector2dF(offset2 - offset1);

How about instead we have

delta = offset2.DeltaFrom(offset1);

and then we can remove the operator-() as this seems to be all the uses of it
(or vast majaority).

"Getting a delta from another offset" seems like a nicer conceptual operation to
read than "Casting the difference between to offsets to a vector". What do you
think?

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_value_observer.h (right):

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_value_observer.h:9: #include
"ui/gfx/geometry/scroll_offset.h"
forward declare it instead

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_value_provider.h (right):

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_value_provider.h:9: #include
"ui/gfx/geometry/scroll_offset.h"
can you forward declare it instead?

https://codereview.chromium.org/584503005/diff/120001/cc/input/layer_scroll_o...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/584503005/diff/120001/cc/input/layer_scroll_o...
cc/input/layer_scroll_offset_delegate.h:9: #include
"ui/gfx/geometry/scroll_offset.h"
can you forward declare it?

https://codereview.chromium.org/584503005/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:43: #include
"ui/gfx/geometry/vector2d_conversions.h"
unused?

https://codereview.chromium.org/584503005/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1084: layer->scroll_offset() +
ScrollOffsetWithDelta

https://codereview.chromium.org/584503005/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/584503005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:262: return
gfx::ScrollOffset(
TODO that GetTotalRootLayerScrollOffset should return a ScrollOffset

https://codereview.chromium.org/584503005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:286:
compositor_client_->UpdateRootLayerState(
TODO that we should pass ScrollOffsets here

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.cc (right):

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.cc:11: std::string ScrollOffset::ToString() const
{
nit: move to bottom like it is ordered in .cc

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.cc:15: bool ScrollOffset::IsZero() const {
these 3 method are all trivial, can you move them to the header file?

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:77: inline ScrollOffset operator+(const
ScrollOffset& lhs,
can we remove the operator+? Seems like ScrollOffsetWithDelta() covers most of
the uses, and we can use Add() for the relatively few (any?) leftovers

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/gfx_tests.gyp
File ui/gfx/gfx_tests.gyp (right):

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/gfx_tests.gyp#ne...
ui/gfx/gfx_tests.gyp:54: 'geometry/scroll_offset_unittest.cc',
where's the equivalent GN change?

[Yufeng Shen (Slow to review), 2014-09-29 19:27:34]

"
I see a lot of

delta = gfx::ScrollOffsetToVector2dF(offset2 - offset1);

How about instead we have

delta = offset2.DeltaFrom(offset1);

and then we can remove the operator-() as this seems to be all the uses of it
(or vast majaority).

"Getting a delta from another offset" seems like a nicer conceptual operation to
read than "Casting the difference between to offsets to a vector". What do you
think?

"


yes, I have added the DeltaFrom function and changed the pattern
delta = gfx::ScrollOffsetToVector2dF(offset2 - offset1);
over to use DeltaFrom.

operator+(-) has one extra usage in  LayerTreeImpl::GetDelegatedScrollOffset().
I have left comments at that file.

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_value_observer.h (right):

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_value_observer.h:9: #include
"ui/gfx/geometry/scroll_offset.h"
On 2014/09/27 00:00:43, danakj wrote:
> forward declare it instead

Done.

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_value_provider.h (right):

https://codereview.chromium.org/584503005/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_value_provider.h:9: #include
"ui/gfx/geometry/scroll_offset.h"
On 2014/09/27 00:00:43, danakj wrote:
> can you forward declare it instead?

Done.

https://codereview.chromium.org/584503005/diff/120001/cc/input/layer_scroll_o...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/584503005/diff/120001/cc/input/layer_scroll_o...
cc/input/layer_scroll_offset_delegate.h:9: #include
"ui/gfx/geometry/scroll_offset.h"
On 2014/09/27 00:00:43, danakj wrote:
> can you forward declare it?

This one does not seem to work.

https://codereview.chromium.org/584503005/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/584503005/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:43: #include
"ui/gfx/geometry/vector2d_conversions.h"
On 2014/09/27 00:00:44, danakj wrote:
> unused?

Done.

https://codereview.chromium.org/584503005/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1084: layer->scroll_offset() +
On 2014/09/27 00:00:44, danakj wrote:
> ScrollOffsetWithDelta

Done.

https://codereview.chromium.org/584503005/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/584503005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:262: return
gfx::ScrollOffset(
On 2014/09/27 00:00:44, danakj wrote:
> TODO that GetTotalRootLayerScrollOffset should return a ScrollOffset

Done.

https://codereview.chromium.org/584503005/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:286:
compositor_client_->UpdateRootLayerState(
On 2014/09/27 00:00:44, danakj wrote:
> TODO that we should pass ScrollOffsets here

Done.

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.cc (right):

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.cc:11: std::string ScrollOffset::ToString() const
{
On 2014/09/27 00:00:44, danakj wrote:
> nit: move to bottom like it is ordered in .cc

with the 3 methods below moved into .cc, the order
is now correct :)

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.cc:15: bool ScrollOffset::IsZero() const {
On 2014/09/27 00:00:44, danakj wrote:
> these 3 method are all trivial, can you move them to the header file?

Done.

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:77: inline ScrollOffset operator+(const
ScrollOffset& lhs,
On 2014/09/27 00:00:44, danakj wrote:
> can we remove the operator+? Seems like ScrollOffsetWithDelta() covers most of
> the uses, and we can use Add() for the relatively few (any?) leftovers

LayerTreeImpl::GetDelegatedScrollOffset() has some use cases where
inner/outer_view_port
scroll offset are added/subtracted. Using Add/Subtract would work, but
Add/Subtract is changing the caller'
s value and we often just want a temporary sum value, operator+ and operator-
seems does the work well.

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/gfx_tests.gyp
File ui/gfx/gfx_tests.gyp (right):

https://codereview.chromium.org/584503005/diff/120001/ui/gfx/gfx_tests.gyp#ne...
ui/gfx/gfx_tests.gyp:54: 'geometry/scroll_offset_unittest.cc',
On 2014/09/27 00:00:44, danakj wrote:
> where's the equivalent GN change?

Done.

[danakj, 2014-10-01 03:08:08]

LGTM thanks I think this is very clean

https://codereview.chromium.org/584503005/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/584503005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3362: gfx::ScrollOffset
scroll_delta(10.f, 10.f);
make this var name and type match, either way seems fine but change one

[Yufeng Shen (Slow to review), 2014-10-01 16:30:15]

https://codereview.chromium.org/584503005/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/584503005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3362: gfx::ScrollOffset
scroll_delta(10.f, 10.f);
On 2014/10/01 03:08:07, danakj wrote:
> make this var name and type match, either way seems fine but change one

Done.

[Yufeng Shen (Slow to review), 2014-10-01 16:32:06]

miletus@chromium.org changed reviewers:
+ jdduke@chromium.org

[Yufeng Shen (Slow to review), 2014-10-01 16:32:32]

+ Jared for OWNER of

content/browser/android/in_process/*

[jdduke (slow), 2014-10-01 16:48:46]

https://codereview.chromium.org/584503005/diff/180001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/584503005/diff/180001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:262: //
TODO(miletus): Make GetTotalRootLayerScrollOffset return
Is there a bug you can reference here (also below)?

[Yufeng Shen (Slow to review), 2014-10-01 16:54:37]

https://codereview.chromium.org/584503005/diff/180001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/584503005/diff/180001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:262: //
TODO(miletus): Make GetTotalRootLayerScrollOffset return
On 2014/10/01 16:48:46, jdduke wrote:
> Is there a bug you can reference here (also below)?

Done.

[jdduke (slow), 2014-10-01 17:04:25]

content/browser/android/in_process lgtm.

[Yufeng Shen (Slow to review), 2014-10-01 17:17:44]

The CQ bit was checked by miletus@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584503005/200001

[commit-bot: I haz the power, 2014-10-01 19:38:43]

Committed patchset #11 (id:200001) as 381e7546497794f6ee980963910e1fdbed4d73c6

[commit-bot: I haz the power, 2014-10-01 19:39:17]

Patchset 11 (id:??) landed as
https://crrev.com/f57925d399f2eda19fdaab9191257e51a3af04a0
Cr-Commit-Position: refs/heads/master@{#297692}