CRD Viewport Management refactor

This CL moves the viewport management related implementation
(e.g. scrolling, letterboxing the plugin, resizing the host) from
remoting.DesktopConnectedView into smaller single-purposed classes.

Summary of changes:
1. Moves bumpScrolling implementation into the remoting.BumpScroller.
2. Moves all viewport related implementation from remoting.DesktopConnectedView
   into remoting.DesktopViewport

BUG=457890
Committed: https://crrev.com/1e259c47f603e4b7cc0e9f5e5d4bea56369b3b32
Cr-Commit-Position: refs/heads/master@{#317707}

[kelvinp, 2015-02-11 23:51:45]

kelvinp@chromium.org changed reviewers:
+ garykac@chromium.org, jamiewalch@chromium.org

[kelvinp, 2015-02-11 23:57:33]

PTAL

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/b...
File remoting/webapp/crd/js/bump_scroller.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/b...
remoting/webapp/crd/js/bump_scroller.js:39: bumpScrollStarted:
'bumpScrollStarted',
The bumpScrolling browsertest will be changed to use this class in part 2 of the
CL.

[garykac, 2015-02-12 18:18:22]

https://codereview.chromium.org/918783002/diff/20001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:45:
'webapp/crd/js/client_plugin_host_desktop_impl.js',
An _impl file without a non-_impl file feels odd...

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin.js:339: remoting.ClientPlugin.HostDesktop =
function() {};
Why not just remoting.HostDesktop?

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin.js:350: shapeChanged:  'shapeChanged'
2 spaces

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:21: * @param {HTMLElement}
rootElement (The scroller element)
scroller element?

[Jamie, 2015-02-12 21:07:29]

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_client_plugin.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_client_plugin.js:176:
remoting.MockClientPlugin.HostDesktop = function() {
As with HostDesktopImpl, I don't think this needs to be a nested class. You
could just call it MockHostDesktop.

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin_host_desktop_impl.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin_host_desktop_impl.js:29:
remoting.ClientPlugin.HostDesktopImpl = function(plugin, postMessageCallback) {
I think it's confusing to have a nested type declared in a different file to its
parent. I think this can just be remoting.HostDesktop. You should also consider
moving HostDesktop out of ClientPlugin. As a rule-of-thumb, nested types make
sense when the nested name would be too generic out-of-context (such as
Application.Delegate) or if the nested type only makes sense in the context of
its container. The host desktop is a concept that exists independently of the
client plugin, so I don't think a nested type is the right model.

[kelvinp, 2015-02-12 23:26:41]

Addressed reviewers feedback.  After Gary landed his CL, I will switch
remoting.DesktopConnectedView to use the Viewport class.

https://codereview.chromium.org/918783002/diff/20001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:45:
'webapp/crd/js/client_plugin_host_desktop_impl.js',
On 2015/02/12 18:18:22, garykac wrote:
> An _impl file without a non-_impl file feels odd...

Done.

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin.js:339: remoting.ClientPlugin.HostDesktop =
function() {};
On 2015/02/12 18:18:22, garykac wrote:
> Why not just remoting.HostDesktop?

Done.

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin.js:350: shapeChanged:  'shapeChanged'
On 2015/02/12 18:18:22, garykac wrote:
> 2 spaces

Done.

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_plugin_host_desktop_impl.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_plugin_host_desktop_impl.js:29:
remoting.ClientPlugin.HostDesktopImpl = function(plugin, postMessageCallback) {
On 2015/02/12 21:07:29, Jamie wrote:
> I think it's confusing to have a nested type declared in a different file to
its
> parent. I think this can just be remoting.HostDesktop. You should also
consider
> moving HostDesktop out of ClientPlugin. As a rule-of-thumb, nested types make
> sense when the nested name would be too generic out-of-context (such as
> Application.Delegate) or if the nested type only makes sense in the context of
> its container. The host desktop is a concept that exists independently of the
> client plugin, so I don't think a nested type is the right model.

I removed ClientPlugin from the interface.  But the implementation of the
HostDesktop is heavily dependent on the clientPlugin. So I will leave the
implementation inside ClientPlugin

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/20001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:21: * @param {HTMLElement}
rootElement (The scroller element)
On 2015/02/12 18:18:22, garykac wrote:
> scroller element?

Done.

[kelvinp, 2015-02-17 01:25:43]

PTAL

[kelvinp, 2015-02-17 01:26:14]

Patchset #4 (id:60001) has been deleted

[Jamie, 2015-02-18 00:33:11]

It feels like this CL is conflating a number of slightly different refactorings.
I'm not sure how best to split it up, but I think you'll get to an LG-to-M
faster if you can.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:266: return
Promise.all([started, stopped]).then(function() {
Promise.all is not order-dependent, right? So I think this change introduces the
possibility of false-positive if STOPPED fires before STARTED. Since it doesn't
simplify the code significantly, I'd prefer to leave it as it was.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_client_plugin.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_client_plugin.js:153:
remoting.MockClientPlugin.HostDesktop = function() {
Although we don't run jscompile over this file, it's still helpful to annotate
important type information such as @implements. If nothing else, it makes it
clearer that this file is defining two classes.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/b...
File remoting/webapp/crd/js/bump_scroller.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/b...
remoting/webapp/crd/js/bump_scroller.js:54: if (this.bumpScrollTimer_) {
I've never found any documentation that states that zero is not a valid timer
id, so I always compare explicitly against null rather than using implicit bool.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/b...
remoting/webapp/crd/js/bump_scroller.js:100: if
(this.viewport_.scroll(lateAdjustment * dx, lateAdjustment * dy)) {
There's an awkward relationship between the BumpScroller and Viewport classes
here, where BumpScroller is responsible for starting and repeating the scroll
events, but Viewport is responsible for stopping them. In the interests of
keeping this CL as close as possible to a simple code-move, I don't think it
needs to be fixed here, but it doesn't feel like the right design--can you add a
TODO?

Moving more of the logic into BumpScroller would give a better balance in terms
of LoC between the two classes, and would probably make this class more testable
too.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:59:
remoting.windowShape.setDesktopRects));
This is missing a bind(). Also, having a global dependency here is not ideal. I
think there are a couple of options:

* Treat the window shape as an intrinsic property of the viewport. in which case
get rid of remoting.windowShape and make it a member of this class.
* Treat the window shape as independent of the viewport, in which case it should
hook shape change notification itself, not relying on this class to do so.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:144: * scrollLeft properties don't
work.
Off-topic for this CL, but this may no longer be true for apps v2. If we could
use the appropriate scroll properties for our bump-scrolling, it would likely be
smoother.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:193: * @param {boolean=} enable True
to enable bump-scrolling, false to disable it.
This parameter is not optional.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:251:
remoting.DesktopViewport.prototype.resizeHostDesktop = function() {
This doesn't feel like it belongs on the DesktopViewport class. Can it be moved
to HostDestop? Maybe pass in an optional scale factor to HostDesktop.resize?

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:314: dimensions.height === 0) {
No need for a new line here.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:334:
remoting.app.onVideoStreamingStarted();
Can we eliminate this global dependency?

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host.js:20: * TODO(kelvinp):Make fields private and
expose them via getters.
This is basically a struct; I don't think there's much value in using getters.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host.js:42: /** @type {remoting.Host.Options} */
Either make this private, or remove the getter. Since nothing else in the class
has getters, I'd lean towards the latter.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_desktop.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_desktop.js:22: /** @return {boolean} */
When defining an interface, more comments are almost always better than fewer.
Please add comments to the methods and events defined by the interface.

[kelvinp, 2015-02-18 02:33:35]

Separated the HostDesktop refactor in its own CL
https://codereview.chromium.org/929323003/

[kelvinp, 2015-02-20 23:24:17]

PTAL

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:266: return
Promise.all([started, stopped]).then(function() {
On 2015/02/18 00:33:10, Jamie wrote:
> Promise.all is not order-dependent, right? So I think this change introduces
the
> possibility of false-positive if STOPPED fires before STARTED. Since it
doesn't
> simplify the code significantly, I'd prefer to leave it as it was.

Done.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/b...
File remoting/webapp/crd/js/bump_scroller.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/b...
remoting/webapp/crd/js/bump_scroller.js:54: if (this.bumpScrollTimer_) {
On 2015/02/18 00:33:10, Jamie wrote:
> I've never found any documentation that states that zero is not a valid timer
> id, so I always compare explicitly against null rather than using implicit
bool.

Done.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/b...
remoting/webapp/crd/js/bump_scroller.js:100: if
(this.viewport_.scroll(lateAdjustment * dx, lateAdjustment * dy)) {
On 2015/02/18 00:33:10, Jamie wrote:
> There's an awkward relationship between the BumpScroller and Viewport classes
> here, where BumpScroller is responsible for starting and repeating the scroll
> events, but Viewport is responsible for stopping them. In the interests of
> keeping this CL as close as possible to a simple code-move, I don't think it
> needs to be fixed here, but it doesn't feel like the right design--can you add
a
> TODO?
> 
> Moving more of the logic into BumpScroller would give a better balance in
terms
> of LoC between the two classes, and would probably make this class more
testable
> too.

As discussed, the Scroll is a perfect valid function in the viewport class, what
is awkward is its return value.  I have modified Scroll to return false if the
scroll is invalid.  The bumpscroller class can then just depends on that well
defined behavior.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:59:
remoting.windowShape.setDesktopRects));
On 2015/02/18 00:33:10, Jamie wrote:
> This is missing a bind(). Also, having a global dependency here is not ideal.
I
> think there are a couple of options:
> 
> * Treat the window shape as an intrinsic property of the viewport. in which
case
> get rid of remoting.windowShape and make it a member of this class.
> * Treat the window shape as independent of the viewport, in which case it
should
> hook shape change notification itself, not relying on this class to do so.

I acknowledged that this is not ideal, but don't want to introduce more churn in
this change.  When the whole refactor is completed, there would be a class
AppConnectedView for looking glass, which will directly hooked into hostDesktop
and handle window shape changes.  I will add a TODO statement.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:144: * scrollLeft properties don't
work.
On 2015/02/18 00:33:10, Jamie wrote:
> Off-topic for this CL, but this may no longer be true for apps v2. If we could
> use the appropriate scroll properties for our bump-scrolling, it would likely
be
> smoother.

Acknowledged.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:193: * @param {boolean=} enable True
to enable bump-scrolling, false to disable it.
On 2015/02/18 00:33:10, Jamie wrote:
> This parameter is not optional.

Done.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:251:
remoting.DesktopViewport.prototype.resizeHostDesktop = function() {
On 2015/02/18 00:33:10, Jamie wrote:
> This doesn't feel like it belongs on the DesktopViewport class. Can it be
moved
> to HostDestop? Maybe pass in an optional scale factor to HostDesktop.resize?

Removed.  The desktopViewport is only created when the session is connected.  By
that time, the capabilities are negotiated and it will auto resize the host if
resize to client is set to true.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:314: dimensions.height === 0) {
On 2015/02/18 00:33:10, Jamie wrote:
> No need for a new line here.

Done.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:334:
remoting.app.onVideoStreamingStarted();
On 2015/02/18 00:33:10, Jamie wrote:
> Can we eliminate this global dependency?

Yes, that would be the end goal.  We will expose events off the mediaChannel()
of the plugin and the app can listens to it. However, the plugin refactor would
need to be done we can do any further work on this.

[Jamie, 2015-02-21 02:01:32]

The diffs look wrong for the latest patch-set. I had one comment based on the
changes you made in response to the previous round, but I haven't looked at the
latest patch-set in any detail.

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:251:
remoting.DesktopViewport.prototype.resizeHostDesktop = function() {
On 2015/02/20 23:24:16, kelvinp wrote:
> On 2015/02/18 00:33:10, Jamie wrote:
> > This doesn't feel like it belongs on the DesktopViewport class. Can it be
> moved
> > to HostDestop? Maybe pass in an optional scale factor to HostDesktop.resize?
> 
> Removed.  The desktopViewport is only created when the session is connected. 
By
> that time, the capabilities are negotiated and it will auto resize the host if
> resize to client is set to true.

It doesn't look like you've removed this.

[kelvinp, 2015-02-23 17:56:00]

PTAL

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_viewport.js:251:
remoting.DesktopViewport.prototype.resizeHostDesktop = function() {
On 2015/02/21 02:01:32, Jamie wrote:
> On 2015/02/20 23:24:16, kelvinp wrote:
> > On 2015/02/18 00:33:10, Jamie wrote:
> > > This doesn't feel like it belongs on the DesktopViewport class. Can it be
> > moved
> > > to HostDestop? Maybe pass in an optional scale factor to
HostDesktop.resize?
> > 
> > Removed.  The desktopViewport is only created when the session is connected.

> By
> > that time, the capabilities are negotiated and it will auto resize the host
if
> > resize to client is set to true.
> 
> It doesn't look like you've removed this.

I made the function private in the latest iteration.  This function is called if
resizeToClient is set to true when the viewport gets resized.
HostDesktop cannot handle this completely by itself as it doesn't know the
client area.

[Jamie, 2015-02-23 21:22:51]

lgtm

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/bump_scroller.js (right):

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/bump_scroller.js:34:
this.defineEvents(Object.keys(remoting.BumpScroller.Events));
base.values

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:125: }
Add an explicit return false here (and below).

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_viewport.js:57: // TODO(kelvinp): Move window
shape related logic into looking glass
Delete "looking glass specific".

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_viewport.js:294: // TODO(kelvinp): Remove the
check once looking glass no longer depends on
s/looking glass/app-remoting/

[kelvinp, 2015-02-23 23:42:30]

FYI

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/bump_scroller.js (right):

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/bump_scroller.js:34:
this.defineEvents(Object.keys(remoting.BumpScroller.Events));
On 2015/02/23 21:22:51, Jamie wrote:
> base.values

Done.

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:125: }
On 2015/02/23 21:22:51, Jamie wrote:
> Add an explicit return false here (and below).

Done.

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_viewport.js (right):

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_viewport.js:57: // TODO(kelvinp): Move window
shape related logic into looking glass
On 2015/02/23 21:22:51, Jamie wrote:
> Delete "looking glass specific".

Done.

https://codereview.chromium.org/918783002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_viewport.js:294: // TODO(kelvinp): Remove the
check once looking glass no longer depends on
On 2015/02/23 21:22:51, Jamie wrote:
> s/looking glass/app-remoting/

Done.

[kelvinp, 2015-02-23 23:42:35]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-02-23 23:42:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/918783002/#ps160001
(title: "Reviewer's feedback")

[commit-bot: I haz the power, 2015-02-23 23:52:27]

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

[commit-bot: I haz the power, 2015-02-24 01:03:14]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-02-24 01:03:59]

Patchset 8 (id:??) landed as
https://crrev.com/1e259c47f603e4b7cc0e9f5e5d4bea56369b3b32
Cr-Commit-Position: refs/heads/master@{#317707}