Fix minimize -> fullscreen transition.

Fix minimize -> fullscreen transition.

When the full-screen app is minimized and then restored, an onRestored event fires,
which causes the title bar to re-appear and the remote desktop to be resized. This
CL suppresses that notification.

I don't think this is easily testable, because there's no way of programmatically
restoring a window from a minimized state.

BUG=400921
Committed: https://crrev.com/eb838509ce28dc498b2632ff59c17184a4f99037
Cr-Commit-Position: refs/heads/master@{#312963}

[Jamie, 2015-01-22 02:35:48]

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

[Jamie, 2015-01-22 02:35:48]

PTAL

[kelvinp, 2015-01-22 17:42:08]

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
File remoting/webapp/crd/js/fullscreen_v2.js (right):

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:23: *     only want transitions from
full-screen to windowed to cause a callback,
s/cause/call

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:102: if (!this.isMinimized_) {
According to 
OnRestored is called whenever the window is restored from being minimized or
maximized.
When the window transits from maximized to restored, this.isMinimized will be
false.  This may cause the callback to be notified unintentionally.  You may
also want to listen to maximized as well as minimized.

[Jamie, 2015-01-22 23:27:31]

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
File remoting/webapp/crd/js/fullscreen_v2.js (right):

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:23: *     only want transitions from
full-screen to windowed to cause a callback,
On 2015/01/22 17:42:08, kelvinp wrote:
> s/cause/call

Cause is acceptable here. Apart from anything else, "call a callback" is a
little too alliterative for my tastes, and my comment wouldn't right-align as
nicely :D

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:102: if (!this.isMinimized_) {
On 2015/01/22 17:42:08, kelvinp wrote:
> According to 
> OnRestored is called whenever the window is restored from being minimized or
> maximized.
> When the window transits from maximized to restored, this.isMinimized will be
> false.  This may cause the callback to be notified unintentionally.  You may
> also want to listen to maximized as well as minimized.

That's a valid observation. I had a go at adding an isMaximized member as well,
but it's a lot harder to keep track of than isMinimized because it's not the
case that onRestored implies !maximized in the same way that it does for
minimized. It's also safe to invoke the callback without including that
logic--the worst-case scenario is that we tell the callback that full-screen is
not active when it already knew that. Contrast that with the bug this CL is
fixing, which caused the callback to be passed the wrong information.

I've added a comment to the code explaining why we don't track the maximized
state of the window. LMK if you think it needs more.

[kelvinp, 2015-01-23 00:14:59]

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
File remoting/webapp/crd/js/fullscreen_v2.js (right):

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:23: *     only want transitions from
full-screen to windowed to cause a callback,
On 2015/01/22 23:27:31, Jamie wrote:
> On 2015/01/22 17:42:08, kelvinp wrote:
> > s/cause/call
> 
> Cause is acceptable here. Apart from anything else, "call a callback" is a
> little too alliterative for my tastes, and my comment wouldn't right-align as
> nicely :D

Acknowledged.

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:102: if (!this.isMinimized_) {
On 2015/01/22 23:27:31, Jamie wrote:
> On 2015/01/22 17:42:08, kelvinp wrote:
> > According to 
> > OnRestored is called whenever the window is restored from being minimized or
> > maximized.
> > When the window transits from maximized to restored, this.isMinimized will
be
> > false.  This may cause the callback to be notified unintentionally.  You may
> > also want to listen to maximized as well as minimized.
> 
> That's a valid observation. I had a go at adding an isMaximized member as
well,
> but it's a lot harder to keep track of than isMinimized because it's not the
> case that onRestored implies !maximized in the same way that it does for
> minimized. It's also safe to invoke the callback without including that
> logic--the worst-case scenario is that we tell the callback that full-screen
is
> not active when it already knew that. Contrast that with the bug this CL is
> fixing, which caused the callback to be passed the wrong information.
> 
> I've added a comment to the code explaining why we don't track the maximized
> state of the window. LMK if you think it needs more.

Maybe we can add a member variable isFullScreened to remember the state that we
last notified our observers.  In this way, our observers won't be notified twice
when isFullScreened is not changed.

[kelvinp, 2015-01-23 00:15:11]

lgtm once the comment is addressed.

[Jamie, 2015-01-23 00:44:16]

PTAL

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
File remoting/webapp/crd/js/fullscreen_v2.js (right):

https://codereview.chromium.org/863123002/diff/1/remoting/webapp/crd/js/fulls...
remoting/webapp/crd/js/fullscreen_v2.js:102: if (!this.isMinimized_) {
On 2015/01/23 00:14:59, kelvinp wrote:
> On 2015/01/22 23:27:31, Jamie wrote:
> > On 2015/01/22 17:42:08, kelvinp wrote:
> > > According to 
> > > OnRestored is called whenever the window is restored from being minimized
or
> > > maximized.
> > > When the window transits from maximized to restored, this.isMinimized will
> be
> > > false.  This may cause the callback to be notified unintentionally.  You
may
> > > also want to listen to maximized as well as minimized.
> > 
> > That's a valid observation. I had a go at adding an isMaximized member as
> well,
> > but it's a lot harder to keep track of than isMinimized because it's not the
> > case that onRestored implies !maximized in the same way that it does for
> > minimized. It's also safe to invoke the callback without including that
> > logic--the worst-case scenario is that we tell the callback that full-screen
> is
> > not active when it already knew that. Contrast that with the bug this CL is
> > fixing, which caused the callback to be passed the wrong information.
> > 
> > I've added a comment to the code explaining why we don't track the maximized
> > state of the window. LMK if you think it needs more.
> 
> Maybe we can add a member variable isFullScreened to remember the state that
we
> last notified our observers.  In this way, our observers won't be notified
twice
> when isFullScreened is not changed.

Done.

[kelvinp, 2015-01-23 18:26:07]

https://codereview.chromium.org/863123002/diff/20001/remoting/webapp/crd/js/f...
File remoting/webapp/crd/js/fullscreen_v2.js (right):

https://codereview.chromium.org/863123002/diff/20001/remoting/webapp/crd/js/f...
remoting/webapp/crd/js/fullscreen_v2.js:35: this.previousCallbackState_ = null;
Nit: I think isFullScreen_ would be a better name. I don't feel strongly though.
 What do you think?

https://codereview.chromium.org/863123002/diff/20001/remoting/webapp/crd/js/f...
remoting/webapp/crd/js/fullscreen_v2.js:125: this.notifyCallbacksOnRestore_ =
false;
Now this.notifyCallbacksOnRestore_ can be removed :)

[Jamie, 2015-01-23 18:51:34]

PTAL

https://codereview.chromium.org/863123002/diff/20001/remoting/webapp/crd/js/f...
File remoting/webapp/crd/js/fullscreen_v2.js (right):

https://codereview.chromium.org/863123002/diff/20001/remoting/webapp/crd/js/f...
remoting/webapp/crd/js/fullscreen_v2.js:35: this.previousCallbackState_ = null;
On 2015/01/23 18:26:07, kelvinp wrote:
> Nit: I think isFullScreen_ would be a better name. I don't feel strongly
though.
>  What do you think?

I think there are a couple of options:

* Leave it as-is; maintaining the variable inside raiseEvent_ makes it very
clear what it does, and (I think) more obviously correct.
* Rename it to isFullscreen and maintain it in onFullscreened and onRestored;
this makes it nicely symmetric with isMinimized.

I prefer the first option (obviousness over symmetry), but in principle the
behaviour should be the same.

https://codereview.chromium.org/863123002/diff/20001/remoting/webapp/crd/js/f...
remoting/webapp/crd/js/fullscreen_v2.js:125: this.notifyCallbacksOnRestore_ =
false;
On 2015/01/23 18:26:07, kelvinp wrote:
> Now this.notifyCallbacksOnRestore_ can be removed :)

Done.

[Jamie, 2015-01-23 21:42:46]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:43:29]

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

[commit-bot: I haz the power, 2015-01-23 23:36:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-23 23:37:31]

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