Inactive password bubble should dissappear after timeout.

Inactive password bubble should dissappear after timeout.

The timer is set when the inactive bubble shows up or when the user moves the mouse out of it. The active bubble doesn't have the timer.

BUG=394287
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287776

[vasilii, 2014-08-05 11:45:27]

Hi Mike,

please review.

[Mike West, 2014-08-05 16:47:59]

Generally LGTM. Please wait for Daniel's feedback before landing.

Also, please add some detail to the commit message so that it's clear what you
changed in order to achieve the effect you were going for.

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left):

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:609: void
ManagePasswordsBubbleView::Init() {
Nit: Please don't move unrelated code. CLs are cheap, and if you do cleanup in
separate CLs, it's much easier to understand the diffs.

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:245:
base::OneShotTimer<ManagePasswordsBubbleView> timer_;
Perhaps we should move this out to the platform agnostic bits so Cocoa can reuse
it. CCing Daniel.

[vasilii, 2014-08-06 10:36:10]

Daniel, your feedback required.

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left):

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:609: void
ManagePasswordsBubbleView::Init() {
On 2014/08/05 16:47:59, Mike West wrote:
> Nit: Please don't move unrelated code. CLs are cheap, and if you do cleanup in
> separate CLs, it's much easier to understand the diffs.

Done.

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:245:
base::OneShotTimer<ManagePasswordsBubbleView> timer_;
On 2014/08/05 16:47:59, Mike West wrote:
> Perhaps we should move this out to the platform agnostic bits so Cocoa can
reuse
> it. CCing Daniel.

It sounds unclear. We can move it to ManagePasswordsBubbleModel or
ManagePasswordsUIController. However, we'll be up to our eyes in callbacks. The
things are more complicated. The bubble should also fade away slowly by timeout.
I can easily move |kBubbleCloseDelay| to some common class though.

[dconnelly, 2014-08-06 11:17:59]

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:245:
base::OneShotTimer<ManagePasswordsBubbleView> timer_;
On 2014/08/06 10:36:10, vasilii wrote:
> On 2014/08/05 16:47:59, Mike West wrote:
> > Perhaps we should move this out to the platform agnostic bits so Cocoa can
> reuse
> > it. CCing Daniel.
> 
> It sounds unclear. We can move it to ManagePasswordsBubbleModel or
> ManagePasswordsUIController. However, we'll be up to our eyes in callbacks.
The
> things are more complicated. The bubble should also fade away slowly by
timeout.
> I can easily move |kBubbleCloseDelay| to some common class though.

I can't tell that this bug reproduces on Mac -- that is, clicking in the content
area after the bubble is auto-displayed does hide the bubble without requiring
an initial bubble click (please correct me if this isn't the bug) -- so I'd keep
it in here for now and we can move it up later if necessary.

[vasilii, 2014-08-06 11:20:46]

On 2014/08/06 11:17:59, dconnelly wrote:
>
https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):
> 
>
https://codereview.chromium.org/441953002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:245:
> base::OneShotTimer<ManagePasswordsBubbleView> timer_;
> On 2014/08/06 10:36:10, vasilii wrote:
> > On 2014/08/05 16:47:59, Mike West wrote:
> > > Perhaps we should move this out to the platform agnostic bits so Cocoa can
> > reuse
> > > it. CCing Daniel.
> > 
> > It sounds unclear. We can move it to ManagePasswordsBubbleModel or
> > ManagePasswordsUIController. However, we'll be up to our eyes in callbacks.
> The
> > things are more complicated. The bubble should also fade away slowly by
> timeout.
> > I can easily move |kBubbleCloseDelay| to some common class though.
> 
> I can't tell that this bug reproduces on Mac -- that is, clicking in the
content
> area after the bubble is auto-displayed does hide the bubble without requiring
> an initial bubble click (please correct me if this isn't the bug) -- so I'd
keep
> it in here for now and we can move it up later if necessary.

Are you stealing focus from our users?

[chromium-reviews, 2014-08-06 11:27:28]

On Wed, Aug 6, 2014 at 1:20 PM, <vasilii@chromium.org> wrote:

> On 2014/08/06 11:17:59, dconnelly wrote:
>
> https://codereview.chromium.org/441953002/diff/20001/
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h
>
>> File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h
>> (right):
>>
>
>
> https://codereview.chromium.org/441953002/diff/20001/
> chrome/browser/ui/views/passwords/manage_passwords_
> bubble_view.h#newcode245
>
>> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:245:
>> base::OneShotTimer<ManagePasswordsBubbleView> timer_;
>> On 2014/08/06 10:36:10, vasilii wrote:
>> > On 2014/08/05 16:47:59, Mike West wrote:
>> > > Perhaps we should move this out to the platform agnostic bits so
>> Cocoa can
>> > reuse
>> > > it. CCing Daniel.
>> >
>> > It sounds unclear. We can move it to ManagePasswordsBubbleModel or
>> > ManagePasswordsUIController. However, we'll be up to our eyes in
>> callbacks.
>> The
>> > things are more complicated. The bubble should also fade away slowly by
>> timeout.
>> > I can easily move |kBubbleCloseDelay| to some common class though.
>>
>
>  I can't tell that this bug reproduces on Mac -- that is, clicking in the
>>
> content
>
>> area after the bubble is auto-displayed does hide the bubble without
>> requiring
>> an initial bubble click (please correct me if this isn't the bug) -- so
>> I'd
>>
> keep
>
>> it in here for now and we can move it up later if necessary.
>>
>
> Are you stealing focus from our users?
>

If you mean is the bubble initially focused, then yes. All Mac bubbles pop
up initially focused.


>
> https://codereview.chromium.org/441953002/
>



-- 
Daniel Connelly | Software Engineer – Chrome | dconnelly@google.com |

Google Germany GmbH
Dienerstr. 12
80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vasilii, 2014-08-06 11:32:18]

I'm landing the CL as is then.

[vasilii, 2014-08-06 11:32:35]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-06 11:33:16]

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

[commit-bot: I haz the power, 2014-08-06 13:12:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-06 15:52:59]

Change committed as 287776