The Password bubble should fade out when user types in the input fields on the web page.

The Password bubble should fade out when user types in the input fields on the web page.

BUG=394287
Committed: https://crrev.com/5092c26c486dbb29c83d58a4f33afde90ef3f96b
Cr-Commit-Position: refs/heads/master@{#293899}

[vasilii, 2014-09-08 13:20:38]

vasilii@chromium.org changed reviewers:
+ mkwst@chromium.org

[vasilii, 2014-09-08 13:21:00]

Hi Mike,

please review.

[Mike West, 2014-09-08 14:53:16]

LGTM with a few nits.

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:659:
ManagePasswordsBubbleView* bubble) : bubble_(bubble) {
Nit: Shouldn't the initialization be on a separate line? What does `git cl
format` say?

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:665: if
(window)
Nit: You can do this in one line. `if (aura::Window* ...)`

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:671:
content::WebContents* web_contents = bubble_->model()->web_contents();
Nit: I'd skip |web_contents|; it'll never be NULL, so just grab the
RenderViewHost directly.

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:687: return
web_contents ? web_contents->GetNativeView() : NULL;
Nit: Will |web_contents| ever be null? Doesn't the bubble view only exist in the
context of a WebContents?

[vasilii, 2014-09-08 16:25:44]

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:659:
ManagePasswordsBubbleView* bubble) : bubble_(bubble) {
On 2014/09/08 14:53:16, Mike West wrote:
> Nit: Shouldn't the initialization be on a separate line? What does `git cl
> format` say?

Done.

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:665: if
(window)
On 2014/09/08 14:53:16, Mike West wrote:
> Nit: You can do this in one line. `if (aura::Window* ...)`

Done. Some reviewers don't like assignment in if().

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:671:
content::WebContents* web_contents = bubble_->model()->web_contents();
On 2014/09/08 14:53:16, Mike West wrote:
> Nit: I'd skip |web_contents|; it'll never be NULL, so just grab the
> RenderViewHost directly.

content::RenderViewHost* rvh =
bubble_->model()->web_contents()->GetRenderViewHost();

two lines anyway.

https://codereview.chromium.org/541683003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:687: return
web_contents ? web_contents->GetNativeView() : NULL;
On 2014/09/08 14:53:16, Mike West wrote:
> Nit: Will |web_contents| ever be null? Doesn't the bubble view only exist in
the
> context of a WebContents?

It's NULL if you close the tab with the bubble. The bubble and WebContents are
destroyed simultaneously.

[vasilii, 2014-09-08 16:26:10]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:30:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/541683003/20001

[commit-bot: I haz the power, 2014-09-08 19:39:52]

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

[commit-bot: I haz the power, 2014-09-08 19:39:53]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vasilii, 2014-09-09 07:57:59]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-09 07:59:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/541683003/20001

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

Committed patchset #2 (id:20001) as fd60cf9adfbf39982d64afeb9248bce16f00336f

[commit-bot: I haz the power, 2014-09-10 03:52:06]

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