Fix crash on clicking the web notification tray

Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
TEST=crash doesn't happen

Committed: https://crrev.com/e65c8d661f1207d8fd4f08622d259dcb90510bca
Cr-Commit-Position: refs/heads/master@{#405112}

[yoshiki, 2016-07-07 06:43:37]

Description was changed from

==========
rebased

BUG=
==========

to

==========
Fix crashes on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
==========

[yoshiki, 2016-07-07 06:50:15]

Description was changed from

==========
Fix crashes on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
==========

to

==========
Fix crashe on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
==========

[yoshiki, 2016-07-07 06:52:01]

Description was changed from

==========
Fix crashe on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
==========

to

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
==========

[yoshiki, 2016-07-07 06:52:38]

Description was changed from

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
==========

to

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
TEST=crash doesn't happen
==========

[yoshiki, 2016-07-07 06:52:38]

yoshiki@chromium.org changed reviewers:
+ stevenjb@chromium.org

[yoshiki, 2016-07-07 06:52:58]

Steven, PTAL

[stevenjb, 2016-07-07 16:57:12]

https://codereview.chromium.org/2124093003/diff/1/ui/message_center/views/mes...
File ui/message_center/views/message_center_bubble.cc (right):

https://codereview.chromium.org/2124093003/diff/1/ui/message_center/views/mes...
ui/message_center/views/message_center_bubble.cc:113:
->GetNextFocusableView(nullptr, nullptr, false, false);
If we are going to DCHECK on bubble_view()->GetFocusManager() we should go ahead
and assign it to a local variable, DCHECK that, then use it here.

https://codereview.chromium.org/2124093003/diff/1/ui/message_center/views/mes...
ui/message_center/views/message_center_bubble.cc:114: if (next_focusable_view)
Add a comment explaining why next_focusable_view might be null.

[yoshiki, 2016-07-11 18:19:33]

Patchset #2 (id:20001) has been deleted

[yoshiki, 2016-07-11 18:19:48]

Steven, PTAL

https://codereview.chromium.org/2124093003/diff/1/ui/message_center/views/mes...
File ui/message_center/views/message_center_bubble.cc (right):

https://codereview.chromium.org/2124093003/diff/1/ui/message_center/views/mes...
ui/message_center/views/message_center_bubble.cc:113:
->GetNextFocusableView(nullptr, nullptr, false, false);
On 2016/07/07 16:57:12, stevenjb wrote:
> If we are going to DCHECK on bubble_view()->GetFocusManager() we should go
ahead
> and assign it to a local variable, DCHECK that, then use it here.

Done.

https://codereview.chromium.org/2124093003/diff/1/ui/message_center/views/mes...
ui/message_center/views/message_center_bubble.cc:114: if (next_focusable_view)
On 2016/07/07 16:57:12, stevenjb wrote:
> Add a comment explaining why next_focusable_view might be null.

Done.

[stevenjb, 2016-07-11 18:25:10]

lgtm w/nit

https://codereview.chromium.org/2124093003/diff/40001/ui/message_center/views...
File ui/message_center/views/message_center_bubble.cc (right):

https://codereview.chromium.org/2124093003/diff/40001/ui/message_center/views...
ui/message_center/views/message_center_bubble.cc:110: views::FocusManager*
focus_manager = new_bubble_view->GetFocusManager();
Since we are using bubble_view() above we should use that here also to avoid
confusion (or use new_bubble_view throughout).

[yoshiki, 2016-07-12 03:43:43]

Thanks!

https://codereview.chromium.org/2124093003/diff/40001/ui/message_center/views...
File ui/message_center/views/message_center_bubble.cc (right):

https://codereview.chromium.org/2124093003/diff/40001/ui/message_center/views...
ui/message_center/views/message_center_bubble.cc:110: views::FocusManager*
focus_manager = new_bubble_view->GetFocusManager();
On 2016/07/11 18:25:10, stevenjb wrote:
> Since we are using bubble_view() above we should use that here also to avoid
> confusion (or use new_bubble_view throughout).

Done.

[yoshiki, 2016-07-12 03:43:58]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-07-12 03:43:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2124093003/#ps60001
(title: "For comment #10")

[commit-bot: I haz the power, 2016-07-12 03:45:04]

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, 2016-07-12 06:43:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 06:43:56]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-07-12 06:45:04]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-07-12 06:45:35]

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, 2016-07-12 08:57:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 08:57:58]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-07-12 15:54:54]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-07-12 15:55:51]

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, 2016-07-12 19:18:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 19:18:35]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-07-13 09:02:13]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-07-13 09:02:29]

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, 2016-07-13 10:25:52]

Description was changed from

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
TEST=crash doesn't happen
==========

to

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
TEST=crash doesn't happen
==========

[commit-bot: I haz the power, 2016-07-13 10:25:53]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-13 10:26:03]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 10:27:06]

Description was changed from

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
TEST=crash doesn't happen
==========

to

==========
Fix crash on clicking the web notification tray

This patch adds null-check for focusable view.

BUG=625357
TEST=crash doesn't happen

Committed: https://crrev.com/e65c8d661f1207d8fd4f08622d259dcb90510bca
Cr-Commit-Position: refs/heads/master@{#405112}
==========

[commit-bot: I haz the power, 2016-07-13 10:27:07]

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