Chromecast: call ClosePage(), wait 50ms before deleting WebContents

Chromecast: call ClosePage(), wait 50ms before deleting WebContents.

Without this, Chromecast apps will not have time to handle
window.unload events when user switches to a new app.

R=lcwu@chromium.org,byungchul@chromium.org
BUG=400876
Committed: https://crrev.com/cb10249278aacc078e0e95936cf68da1d95b64a3
Cr-Commit-Position: refs/heads/master@{#296811}

[lcwu1, 2014-09-25 19:10:55]

The commit message is not entirely accurate. We should say

"Call ClosePage to deliver windlow.unload event and wait for 50ms after closing
page before deleting the web contents."

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/android/apk...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java
(right):

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java:101:
* @param gracefully Whether or not to give a time interval to deliver
window.unload event
I would change this comment to be something like "Whether or not to call RWH
ClosePage to deliver unload event".

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_android.cc (right):

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_android.cc:84: //
CastWindowAndroid::CloseContents().
This should probably be something like this:
"Close page first, which would lead the window.unload event to be fired. The
window itself would be then destroyed inside CloseContents when the browser
receives the ACK from the renderer that the page is closed."

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_android.h (right):

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_android.h:49: // Gives a time
interval to deliver window.unload event before window close.
I think the comment should be something like "this function calls ClosePage and
waits for the acknowledgement from the renderer before closing/deleting the
shell window".

[gunsch, 2014-09-25 19:18:15]

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/android/apk...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java
(right):

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java:101:
* @param gracefully Whether or not to give a time interval to deliver
window.unload event
On 2014/09/25 19:10:54, lcwu1 wrote:
> I would change this comment to be something like "Whether or not to call RWH
> ClosePage to deliver unload event".

Done.

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_android.cc (right):

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_android.cc:84: //
CastWindowAndroid::CloseContents().
On 2014/09/25 19:10:54, lcwu1 wrote:
> This should probably be something like this:
> "Close page first, which would lead the window.unload event to be fired. The
> window itself would be then destroyed inside CloseContents when the browser
> receives the ACK from the renderer that the page is closed."

Done.

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_android.h (right):

https://codereview.chromium.org/601293002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_android.h:49: // Gives a time
interval to deliver window.unload event before window close.
On 2014/09/25 19:10:54, lcwu1 wrote:
> I think the comment should be something like "this function calls ClosePage
and
> waits for the acknowledgement from the renderer before closing/deleting the
> shell window".

Done.

[byungchul, 2014-09-25 20:07:36]

lgtm

[lcwu1, 2014-09-25 20:22:51]

LGTM with a nit in comment.

https://codereview.chromium.org/601293002/diff/20001/chromecast/shell/android...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java
(right):

https://codereview.chromium.org/601293002/diff/20001/chromecast/shell/android...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java:101:
* @param gracefully Whether or not to call RWH::ClosePage to deliver unload
event.
Nit: It's RVH (RenderViewHost) ClosePage, not NWH (RenderWdigetHost) that we
call.

[gunsch, 2014-09-25 20:48:21]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2014-09-25 20:48:35]

https://codereview.chromium.org/601293002/diff/20001/chromecast/shell/android...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java
(right):

https://codereview.chromium.org/601293002/diff/20001/chromecast/shell/android...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java:101:
* @param gracefully Whether or not to call RWH::ClosePage to deliver unload
event.
On 2014/09/25 20:22:50, lcwu1 wrote:
> Nit: It's RVH (RenderViewHost) ClosePage, not NWH (RenderWdigetHost) that we
> call.

Done.

[commit-bot: I haz the power, 2014-09-25 20:49:52]

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

[commit-bot: I haz the power, 2014-09-25 22:40:12]

Committed patchset #3 (id:40001) as bd3d7913a979dce4bbca672cb17b600ca3372f8b

[commit-bot: I haz the power, 2014-09-25 22:40:47]

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