[Remoting Client] DesktopViewport Implementation

[Remoting Client] DesktopViewport Implementation

This CL implements a class and some geometry tools to manipulate the viewport
on a mobile client. For now this is only for iOS only. Later we should try to
also use this on Android to reduce platform-specific codepaths.

Review-Url: https://codereview.chromium.org/2843373005
Cr-Commit-Position: refs/heads/master@{#468378}
Committed: https://chromium.googlesource.com/chromium/src/+/10b769e9ee208a2d2f08dec880126bf4731f5e1e

[Yuwei, 2017-04-28 02:02:25]

Patchset #1 (id:1) has been deleted

[Yuwei, 2017-04-28 02:10:51]

yuweih@chromium.org changed reviewers:
+ joedow@chromium.org

[Yuwei, 2017-04-28 02:10:51]

PTAL

The number of lines looks scary but many of them are just ASCII arts and
comments :P

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
File remoting/client/simple_matrix.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
remoting/client/simple_matrix.h:16: class SimpleMatrix {
It's possible to use Skia to do matrix calculations but I think:

1. It will increase the binary size by about 1MB.
2. It seems too heavyweight for our simple matrix calculations. (and we
technically only care about three variables)

So I just manually implemented this :)

[joedow, 2017-04-28 21:29:53]

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
File remoting/client/desktop_viewport.cc (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:19: DesktopViewport::DesktopViewport() :
desktop_to_surface_(0.f, {0.f, 0.f}) {}
Add a default c'tor to SimpleMatrix so you don't need to init with zeroes?

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:49: LOG(ERROR) << "Viewport is not ready.";
Should this be a DCHECK?  It seems like this is unexpected.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:53: // <---Desktop Width---->
I like the ASCII art, I  think it would be good to describe how this works a bit
more.  The gist is that it resizes the desktop such that it fits the viewport in
one dimension.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:164: if
(!on_transformation_changed_.is_null()) {
nit: You don't need to call is_null(): 
if (on_transformation_changed_) { on_transformation_changed_.Run(); }

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:178: bounds.left = viewport_size.x * 0.5f;
divide by 2?  I think that is a more natural way to express that you want half
of something.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
File remoting/client/desktop_viewport.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:12: 
You should include 'remoting/client/viewport_geometry.h' as well since you use
Point and Bound and they are defined there.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:18: // the desktop onto the client's view
surface on the screen.
This is a good comment, but I think it would be good to add a bit more about
what 'desktop' and 'surface' are as a concept and how they interact.  This is
the main concept for a maintainer to wrap their head around and I think it could
be described a bit more to clarify.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:31: // Sets the desktop size.
nit: You could just refer to the actual member -> 'Sets |desktop_size_|.'  The
same goes for a few of the other comments in the CL.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:39: // and the surface size are set.
"This will be called once both the desktop and the surface size are set."

Can you clarify this comment?  It is a public method so it could be called
multiple times.  Or is the comment telling the consumer of the class to call it
once both are set?

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:84: SimpleMatrix desktop_to_surface_;
desktop_to_surface_transform_?

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
File remoting/client/desktop_viewport_unittest.cc (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:20: class DesktopViewportTest :
public testing::Test {
This system is a bit different than other unit tests I've seen in our codebase. 
The most common pattern is to set your environment up (callback) save the result
after you call the method being tested, call base::RunLoop().RunUntilIdle() or
similar) and then compare the results.

The mechanism below works but I had to read the code more closely since it needs
additional checks in each method (to ensure it isn't being called multiple
times).

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:26: void ExpectTransformation(const
tracked_objects::Location& from_here,
I'd rename this to SetExpectedTransform() or SetExpectedTransformResult() to
make it more clear what its purpose is.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:37: std::array<float, 9>
expected_matrix_;
expected_transfrom_?  The method above doesn't mention matrix so that term seems
out of place.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:80: expect_matrix_ = false;
If SimpleMatrix had an 'isEmpty()' method, you could use it to determine if you
are expecting the callback to be run.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
File remoting/client/simple_matrix.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
remoting/client/simple_matrix.h:6: #define REMOTING_CLIENT_SIMPLE_MATRIX_H_
Maybe just call this matrix?  It isn't clear why it is simple and we have no
other versions that are complex (to my knowledge).

Perhaps a ViewMatrix is a better term, it seems like the comments below indicate
that is its intended purpose.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
remoting/client/simple_matrix.h:16: class SimpleMatrix {
On 2017/04/28 02:10:51, Yuwei wrote:
> It's possible to use Skia to do matrix calculations but I think:
> 
> 1. It will increase the binary size by about 1MB.
> 2. It seems too heavyweight for our simple matrix calculations. (and we
> technically only care about three variables)
> 
> So I just manually implemented this :)

I think saving ~1MB is a good reason since our package is ~7.5MB at the moment.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
File remoting/client/viewport_geometry.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
remoting/client/viewport_geometry.h:8: #include <array>
I don't think you use array here

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
remoting/client/viewport_geometry.h:15: };
I wonder if these should be public types exposed via the ViewMatrix or perhaps
DesktopViewport?  Creating them in the global remoting namespace seems too
broad.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
remoting/client/viewport_geometry.h:29: Point ConstrainPointToBounds(const
Bounds& bounds, const Point& point);
This is only used in DesktopView so move it into an anonymous namespace there?

[Yuwei, 2017-04-28 23:53:41]

PTAL. Thanks!

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
File remoting/client/desktop_viewport.cc (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:19: DesktopViewport::DesktopViewport() :
desktop_to_surface_(0.f, {0.f, 0.f}) {}
On 2017/04/28 21:29:52, joedow wrote:
> Add a default c'tor to SimpleMatrix so you don't need to init with zeroes?

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:49: LOG(ERROR) << "Viewport is not ready.";
On 2017/04/28 21:29:52, joedow wrote:
> Should this be a DCHECK?  It seems like this is unexpected.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:53: // <---Desktop Width---->
On 2017/04/28 21:29:52, joedow wrote:
> I like the ASCII art, I  think it would be good to describe how this works a
bit
> more.  The gist is that it resizes the desktop such that it fits the viewport
in
> one dimension.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:164: if
(!on_transformation_changed_.is_null()) {
On 2017/04/28 21:29:52, joedow wrote:
> nit: You don't need to call is_null(): 
> if (on_transformation_changed_) { on_transformation_changed_.Run(); }

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.cc:178: bounds.left = viewport_size.x * 0.5f;
On 2017/04/28 21:29:52, joedow wrote:
> divide by 2?  I think that is a more natural way to express that you want half
> of something.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
File remoting/client/desktop_viewport.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:12: 
On 2017/04/28 21:29:53, joedow wrote:
> You should include 'remoting/client/viewport_geometry.h' as well since you use
> Point and Bound and they are defined there.

Obsolete. I broke down viewport_geometry and moved structs inside
DesktopViewport and SimpleMatrix.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:18: // the desktop onto the client's view
surface on the screen.
On 2017/04/28 21:29:53, joedow wrote:
> This is a good comment, but I think it would be good to add a bit more about
> what 'desktop' and 'surface' are as a concept and how they interact.  This is
> the main concept for a maintainer to wrap their head around and I think it
could
> be described a bit more to clarify.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:31: // Sets the desktop size.
On 2017/04/28 21:29:53, joedow wrote:
> nit: You could just refer to the actual member -> 'Sets |desktop_size_|.'  The
> same goes for a few of the other comments in the CL.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:39: // and the surface size are set.
On 2017/04/28 21:29:53, joedow wrote:
> "This will be called once both the desktop and the surface size are set."
> 
> Can you clarify this comment?  It is a public method so it could be called
> multiple times.  Or is the comment telling the consumer of the class to call
it
> once both are set?

I just moved it to the private block. SetDesktopSize() and SetSurfaceSize() will
call it when necessary so for now there is no need to allow user to call this
function.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport.h:84: SimpleMatrix desktop_to_surface_;
On 2017/04/28 21:29:53, joedow wrote:
> desktop_to_surface_transform_?

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
File remoting/client/desktop_viewport_unittest.cc (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:20: class DesktopViewportTest :
public testing::Test {
On 2017/04/28 21:29:53, joedow wrote:
> This system is a bit different than other unit tests I've seen in our
codebase. 
> The most common pattern is to set your environment up (callback) save the
result
> after you call the method being tested, call base::RunLoop().RunUntilIdle() or
> similar) and then compare the results.
> 
> The mechanism below works but I had to read the code more closely since it
needs
> additional checks in each method (to ensure it isn't being called multiple
> times).

I just reversed the order to use the more common capture then assert way.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:26: void ExpectTransformation(const
tracked_objects::Location& from_here,
On 2017/04/28 21:29:53, joedow wrote:
> I'd rename this to SetExpectedTransform() or SetExpectedTransformResult() to
> make it more clear what its purpose is.

Renamed to AssertTransformationReceived.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:37: std::array<float, 9>
expected_matrix_;
On 2017/04/28 21:29:53, joedow wrote:
> expected_transfrom_?  The method above doesn't mention matrix so that term
seems
> out of place.

Use |received_transformation_| now

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/desktop...
remoting/client/desktop_viewport_unittest.cc:80: expect_matrix_ = false;
On 2017/04/28 21:29:53, joedow wrote:
> If SimpleMatrix had an 'isEmpty()' method, you could use it to determine if
you
> are expecting the callback to be run.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
File remoting/client/simple_matrix.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/simple_...
remoting/client/simple_matrix.h:6: #define REMOTING_CLIENT_SIMPLE_MATRIX_H_
On 2017/04/28 21:29:53, joedow wrote:
> Maybe just call this matrix?  It isn't clear why it is simple and we have no
> other versions that are complex (to my knowledge).

I called it simple because it only needs 3 parameters to represent a 3x3
matrix..

> Perhaps a ViewMatrix is a better term, it seems like the comments below
indicate
> that is its intended purpose.

Done.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
File remoting/client/viewport_geometry.h (right):

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
remoting/client/viewport_geometry.h:8: #include <array>
On 2017/04/28 21:29:53, joedow wrote:
> I don't think you use array here

Obsolete. File deleted.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
remoting/client/viewport_geometry.h:15: };
On 2017/04/28 21:29:53, joedow wrote:
> I wonder if these should be public types exposed via the ViewMatrix or perhaps
> DesktopViewport?  Creating them in the global remoting namespace seems too
> broad.

Moved into ViewMatrix.

https://codereview.chromium.org/2843373005/diff/20001/remoting/client/viewpor...
remoting/client/viewport_geometry.h:29: Point ConstrainPointToBounds(const
Bounds& bounds, const Point& point);
On 2017/04/28 21:29:53, joedow wrote:
> This is only used in DesktopView so move it into an anonymous namespace there?
 

Moved into DesktopViewport

[joedow, 2017-05-01 15:01:47]

lgtm

[Yuwei, 2017-05-01 17:53:21]

On 2017/05/01 15:01:47, joedow wrote:
> lgtm

Thanks!

[Yuwei, 2017-05-01 17:53:24]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-01 17:54:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 19:05:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493661204669840,
"parent_rev": "978ff041e16a75594225bb33bfd470f92c170314", "commit_rev":
"10b769e9ee208a2d2f08dec880126bf4731f5e1e"}

[commit-bot: I haz the power, 2017-05-01 19:06:02]

Description was changed from

==========
[Remoting Client] DesktopViewport Implementation

This CL implements a class and some geometry tools to manipulate the viewport
on a mobile client. For now this is only for iOS only. Later we should try to
also use this on Android to reduce platform-specific codepaths.
==========

to

==========
[Remoting Client] DesktopViewport Implementation

This CL implements a class and some geometry tools to manipulate the viewport
on a mobile client. For now this is only for iOS only. Later we should try to
also use this on Android to reduce platform-specific codepaths.

Review-Url: https://codereview.chromium.org/2843373005
Cr-Commit-Position: refs/heads/master@{#468378}
Committed:
https://chromium.googlesource.com/chromium/src/+/10b769e9ee208a2d2f08dec88012...
==========

[commit-bot: I haz the power, 2017-05-01 19:06:03]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/10b769e9ee208a2d2f08dec88012...