kiosk: Fix zoom crash regression.

kiosk: Fix zoom crash regression.

BUG=412922
Committed: https://crrev.com/45c8467d67208ba6225ef3b31f5e0be2ff82f060
Cr-Commit-Position: refs/heads/master@{#294530}

[xiyuan, 2014-09-11 22:42:03]

xiyuan@chromium.org changed reviewers:
+ tapted@chromium.org

[xiyuan, 2014-09-11 22:42:03]

Trent, could you help to review this?

Thanks.

[tapted, 2014-09-11 23:55:01]

lgtm, just one optional suggestion

https://codereview.chromium.org/570473002/diff/1/chrome/browser/ui/apps/chrom...
File chrome/browser/ui/apps/chrome_app_delegate.cc (right):

https://codereview.chromium.org/570473002/diff/1/chrome/browser/ui/apps/chrom...
chrome/browser/ui/apps/chrome_app_delegate.cc:183:
ZoomController::CreateForWebContents(web_contents);
Do you think it's worth adding something near the other "future proof kiosk
mode" DCHECK, at

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

e.g.

  // Ensure there is a ZoomController in kiosk mode, otherwise the processing
  // the accelerators will cause a crash.
  DCHECK(!is_kiosk_app_mode ||
      ZoomController::FromWebContents(web_view()->GetWebContents());

pretty sure the construction order will make it safe. It's just tricky to see
the link between the code here and registering accelerators in
chrome_native_app_window_views.

[xiyuan, 2014-09-12 00:34:33]

Patchset #2 (id:20001) has been deleted

[xiyuan, 2014-09-12 00:45:27]

https://codereview.chromium.org/570473002/diff/1/chrome/browser/ui/apps/chrom...
File chrome/browser/ui/apps/chrome_app_delegate.cc (right):

https://codereview.chromium.org/570473002/diff/1/chrome/browser/ui/apps/chrom...
chrome/browser/ui/apps/chrome_app_delegate.cc:183:
ZoomController::CreateForWebContents(web_contents);
On 2014/09/11 23:55:01, tapted wrote:
> Do you think it's worth adding something near the other "future proof kiosk
> mode" DCHECK, at
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> e.g.
> 
>   // Ensure there is a ZoomController in kiosk mode, otherwise the processing
>   // the accelerators will cause a crash.
>   DCHECK(!is_kiosk_app_mode ||
>       ZoomController::FromWebContents(web_view()->GetWebContents());
> 
> pretty sure the construction order will make it safe. It's just tricky to see
> the link between the code here and registering accelerators in
> chrome_native_app_window_views.

Sounds good. Added the DCHECK there.

Do you prefer to create ZoomController in ChromeNativeAppWindowViews to keep
relevant code closer?

[tapted, 2014-09-12 00:52:56]

On 2014/09/12 00:45:27, xiyuan wrote:
> Do you prefer to create ZoomController in ChromeNativeAppWindowViews to keep
> relevant code closer?

Yeah, that crossed my mind, but I liked the feeling of consolidating the
CreateForWebContents at ChromeAppDelegate::InitWebContents

so, slgm

[xiyuan, 2014-09-12 01:03:05]

On 2014/09/12 00:52:56, tapted wrote:
> On 2014/09/12 00:45:27, xiyuan wrote:
> > Do you prefer to create ZoomController in ChromeNativeAppWindowViews to keep
> > relevant code closer?
> 
> Yeah, that crossed my mind, but I liked the feeling of consolidating the
> CreateForWebContents at ChromeAppDelegate::InitWebContents

Me too.

Thanks for the quick review.

[xiyuan, 2014-09-12 01:04:59]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-09-12 01:06:55]

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

[commit-bot: I haz the power, 2014-09-12 02:38:48]

Committed patchset #2 (id:40001) as 38078c2892f1399097dd04725ebb16d3470b02a7

[commit-bot: I haz the power, 2014-09-12 02:42:42]

Patchset 2 (id:??) landed as
https://crrev.com/45c8467d67208ba6225ef3b31f5e0be2ff82f060
Cr-Commit-Position: refs/heads/master@{#294530}