Issue 23512003: new animation for rAc card generation

Issue 23512003: new animation for rAc card generation (Closed)

Created:
7 years, 3 months ago by Evan Stade

Modified:
7 years, 3 months ago

Reviewers:
groby-ooo-7-16

CC:
chromium-reviews, benquan, tfarina, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman, rouslan+autofillwatch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

new animation for rAc card generation this matches Android somewhat. A good first pass, which can be followed up if need be. BUG=260951 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221148

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : remove second string on final interstitial #

Total comments: 14

Patch Set 4 : fix unit tests #

Created: 7 years, 3 months ago

Download [raw] [tar.bz2]

Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+125 lines, -124 lines)			Patch
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h	View	1 2 3	3 chunks	+5 lines, -3 lines	0 comments	Download
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc	View	1 2 3	6 chunks	+55 lines, -47 lines	0 comments	Download
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc	View	1 2 3	8 chunks	+18 lines, -22 lines	0 comments	Download
chrome/browser/ui/autofill/autofill_dialog_types.h	View	1 2 3	3 chunks	+8 lines, -3 lines	0 comments	Download
chrome/browser/ui/autofill/autofill_dialog_view_delegate.h	View	1 2 3	1 chunk	+0 lines, -3 lines	0 comments	Download
chrome/browser/ui/views/autofill/autofill_dialog_views.h	View	1 2 3	5 chunks	+13 lines, -12 lines	0 comments	Download
chrome/browser/ui/views/autofill/autofill_dialog_views.cc	View	1 2 3	9 chunks	+26 lines, -34 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

groby-ooo-7-16

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode846 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:846: state.expiry = base::TimeDelta::FromMilliseconds(100); Constant? https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode3187 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3187: // TODO(estade): implement ...

7 years, 3 months ago (2013-09-03 18:50:03 UTC) #3

Evan Stade

7 years, 3 months ago (2013-09-03 19:05:37 UTC) #4

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:846: state.expiry
= base::TimeDelta::FromMilliseconds(100);
On 2013/09/03 18:50:04, groby wrote:
> Constant?

I don't see the purpose of a constant here because the code is already very
self-documenting and the value is not reused anywhere.

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3187: //
TODO(estade): implement overlays on other platforms.
On 2013/09/03 18:50:04, groby wrote:
> Is there a particular reason to not implement this for OSX right now? I'm
> working on overlays, so that should land soon. Meanwhile, UpdateButtonStrip
will
> do exactly nothing, so there's not much harm either :)
> 

I'd rather that enabling this on mac be a separate CL where you make sure it
works as expected.

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3197:
base::TimeDelta::FromMilliseconds(2000));
On 2013/09/03 18:50:04, groby wrote:
> Declare as constant?

ditto

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.h:218: // TODO(estade): should
be able to remove this; text is always black.
On 2013/09/03 18:50:04, groby wrote:
> Remove away. Or will that bite Android, too?

Would rather keep this change focused; also verification of "should" in above
statement will require some small effort.

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.h:239: base::TimeDelta expiry;
On 2013/09/03 18:50:04, groby wrote:
> Heh. I learned a new word :)
> 
> Should the controller push the status updates instead? We do call UpdateXXX
for
> lots of other things, so it feels odd to have this pull based.

The reason I did it like this is because the controller doesn't really track
when something starts or stops showing. The controller never says "start
animation", it just knows that for the given state, this is what the dialog
should look like.

The controller/view relationship is pull-based with only "invalidate" messages
(i.e. UpdateXXX) being pushed. The view should be able to pull the current state
at any time, regardless of whether it got an invalidate message.

> Amongst other things, it'd hide the entire timer logic from the view and make
it
> slightly simpler.

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:580:
refresh_timer_(false, false) {
On 2013/09/03 18:50:04, groby wrote:
> If we keep the timer in OverlayView - why not a repeating timer?

I don't see how that would reduce the code complexity.

groby-ooo-7-16

lgtm https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofill/autofill_dialog_types.h File chrome/browser/ui/autofill/autofill_dialog_types.h (right): https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofill/autofill_dialog_types.h#newcode239 chrome/browser/ui/autofill/autofill_dialog_types.h:239: base::TimeDelta expiry; Well, it sort of does track ...

7 years, 3 months ago (2013-09-03 22:21:41 UTC) #5

lgtm

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.h:239: base::TimeDelta expiry;
Well, it sort of does track when it stops showing - DoFinishSubmit is posted
from the ControllerImpl.

I'd have preferred for all the timer stuff to be in the controller (amongst
other things because it's shared across platforms then), but I'm not going to be
too attached to that.

On 2013/09/03 19:05:37, Evan Stade wrote:
> On 2013/09/03 18:50:04, groby wrote:
> > Heh. I learned a new word :)
> > 
> > Should the controller push the status updates instead? We do call UpdateXXX
> for
> > lots of other things, so it feels odd to have this pull based.
> 
> The reason I did it like this is because the controller doesn't really track
> when something starts or stops showing. The controller never says "start
> animation", it just knows that for the given state, this is what the dialog
> should look like.
> 
> The controller/view relationship is pull-based with only "invalidate" messages
> (i.e. UpdateXXX) being pushed. The view should be able to pull the current
state
> at any time, regardless of whether it got an invalidate message.
> 
> > Amongst other things, it'd hide the entire timer logic from the view and
make
> it
> > slightly simpler.
>

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/23512003/diff/20001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:580:
refresh_timer_(false, false) {
My above statement missed a don't, I think. A repeating timer would make sense
for the controller. Since we re-update the timer every time in the view, you're
right, it's not really a gain.

On 2013/09/03 19:05:37, Evan Stade wrote:
> On 2013/09/03 18:50:04, groby wrote:
> > If we keep the timer in OverlayView - why not a repeating timer?
> 
> I don't see how that would reduce the code complexity.

commit-bot: I haz the power

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

7 years, 3 months ago (2013-09-04 00:14:05 UTC) #6

Message was sent while issue was closed.

Change committed as 221148

Expand Messages | Collapse Messages