rAc: Enforce a 3 second grace period for generated card bubble.

rAc: Enforce a 3 second grace period for generated card bubble.

Bubble may not be dismissed for the first 3 seconds (except by clicking on the Learn More link). The length of the grace period is subject to future tweaking.

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

[sky, 2013-09-04 14:10:14]

https://codereview.chromium.org/23533030/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/23533030/diff/1/ui/views/widget/widget.cc#new...
ui/views/widget/widget.cc:921: capture_explicitly_set_ = true;
Do you verify all callers of SetCapture are ok with new behavior? I suspect tab
dragging isn't, but I didn't verify that.

[Evan Stade, 2013-09-04 23:00:38]

https://codereview.chromium.org/23533030/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/23533030/diff/1/ui/views/widget/widget.cc#new...
ui/views/widget/widget.cc:921: capture_explicitly_set_ = true;
On 2013/09/04 14:10:15, sky wrote:
> Do you verify all callers of SetCapture are ok with new behavior? I suspect
tab
> dragging isn't, but I didn't verify that.

You are quite right. Do you like this new approach better?

[sky, 2013-09-04 23:07:06]

How about nuking SetCaptureToWidget and add
set_release_capture_on_mouse_released(bool)? Doing that means you could nuke the
one override to ShouldReleaseCaptureOnMouseReleased and instead use the setter.

[sky, 2013-09-04 23:07:24]

Also, I don't think the boolean should auto-reset.

[Evan Stade, 2013-09-04 23:22:10]

On 2013/09/04 23:07:24, sky wrote:
> Also, I don't think the boolean should auto-reset.

done.

[Evan Stade, 2013-09-04 23:28:35]

On 2013/09/04 23:22:10, Evan Stade wrote:
> On 2013/09/04 23:07:24, sky wrote:
> > Also, I don't think the boolean should auto-reset.
> 
> done.

and added a test.

[sky, 2013-09-04 23:32:15]

https://codereview.chromium.org/23533030/diff/14001/ui/views/widget/widget_un...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/23533030/diff/14001/ui/views/widget/widget_un...
ui/views/widget/widget_unittest.cc:1377: TEST_F(WidgetTest, CaptureAutoReset) {
Any time you're messing with capture your window needs to be foreground. You
aren't guaranteed of that when running in unit test (since they may run in
parallel). This should be moved to interactive ui tests. I believe Ananta just
created a widget_interactiveuitest file.

[Evan Stade, 2013-09-04 23:35:52]

https://codereview.chromium.org/23533030/diff/14001/ui/views/widget/widget_un...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/23533030/diff/14001/ui/views/widget/widget_un...
ui/views/widget/widget_unittest.cc:1377: TEST_F(WidgetTest, CaptureAutoReset) {
On 2013/09/04 23:32:15, sky wrote:
> Any time you're messing with capture your window needs to be foreground. You
> aren't guaranteed of that when running in unit test (since they may run in
> parallel). This should be moved to interactive ui tests. I believe Ananta just
> created a widget_interactiveuitest file.

how does the test above it get away with it?

[sky, 2013-09-04 23:38:56]

I suspect there are a lot of tests in that file (and others) that
could easily be flaky.

  -Scott

On Wed, Sep 4, 2013 at 4:35 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/23533030/diff/14001/ui/views/widget/widget_un...
> File ui/views/widget/widget_unittest.cc (right):
>
>
https://codereview.chromium.org/23533030/diff/14001/ui/views/widget/widget_un...
> ui/views/widget/widget_unittest.cc:1377: TEST_F(WidgetTest,
> CaptureAutoReset) {
> On 2013/09/04 23:32:15, sky wrote:
>>
>> Any time you're messing with capture your window needs to be
>
> foreground. You
>>
>> aren't guaranteed of that when running in unit test (since they may
>
> run in
>>
>> parallel). This should be moved to interactive ui tests. I believe
>
> Ananta just
>>
>> created a widget_interactiveuitest file.
>
>
> how does the test above it get away with it?
>
> https://codereview.chromium.org/23533030/

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

[Evan Stade, 2013-09-05 00:31:16]

ok, moved some tests to interactive ui tests.

[sky, 2013-09-05 15:46:05]

LGTM

[Evan Stade, 2013-09-05 22:15:03]

https://codereview.chromium.org/23533030/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/23533030/diff/5001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:381: ['exclude', '^../ui/views/'],
note this new line. Let's see if it works on the trybots.

[commit-bot: I haz the power, 2013-09-05 22:47:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/23533030/5001

[commit-bot: I haz the power, 2013-09-06 06:19:37]

Change committed as 221601