Issue 73723002: [rAc OSX] Animate the dots in the "Loading ..." message.

Issue 73723002: [rAc OSX] Animate the dots in the "Loading ..." message. (Closed)

Created:
7 years, 1 month ago by Ilya Sherman

Modified:
7 years, 1 month ago

Reviewers:
groby-ooo-7-16, Evan Stade

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

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

Visibility:
Public.

More Reviews

Description

[rAc OSX] Animate the dots in the "Loading ..." message. BUG=315980, 315981 TEST=Open Autofill dialog in Wallet mode. While the Wallet data is loading, the loading screen should show the message "Loading payment details..." and the dots should animate. R=estade@chromium.org, groby@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236152

Patch Set 1 #

Patch Set 2 : git-add new files #

Patch Set 3 : Clean up new files #

Total comments: 25

Patch Set 4 : Rebase #

Patch Set 5 : Encapsulate more, use native constructs more #

Total comments: 10

Patch Set 6 : Fix nits #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase and add a missing semicolon #

Created: 7 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+264 lines, -84 lines)			Patch
M	chrome/browser/ui/autofill/loading_animation.h	View	1 2 3 4 5	2 chunks	+7 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/autofill/loading_animation.cc	View	1 2 3 4 5	4 chunks	+20 lines, -12 lines	0 comments	Download
M	chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm	View	1 2 3 4 5 6	6 chunks	+14 lines, -61 lines	0 comments	Download
A	chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h	View	1 2 3 4 5	1 chunk	+49 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm	View	1 2 3 4 5	1 chunk	+162 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/autofill/autofill_dialog_views.cc	View	1 2 3 4 5 6 7	5 chunks	+7 lines, -8 lines	0 comments	Download
M	chrome/chrome_browser_ui.gypi	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

Evan Stade

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/autofill/loading_animation.cc File chrome/browser/ui/autofill/loading_animation.cc (right): https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/autofill/loading_animation.cc#newcode57 chrome/browser/ui/autofill/loading_animation.cc:57: static double kScale = 10.0; nit: I don't really ...

7 years, 1 month ago (2013-11-15 21:58:45 UTC) #2

groby-ooo-7-16

7 years, 1 month ago (2013-11-15 23:03:37 UTC) #3

Ilya Sherman

7 years, 1 month ago (2013-11-16 00:31:13 UTC) #4

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/loading_animation.cc (right):

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/loading_animation.cc:57: static double kScale = 10.0;
On 2013/11/15 23:03:38, groby wrote:
> On 2013/11/15 21:58:46, Evan Stade wrote:
> > nit: I don't really see the benefit in doing this here --- the value depends
> on
> > the font size, so I'd prefer it be defined near where the font size is set
> (i.e.
> > in each view), even though that means defining it twice. Alternately, make
> this
> > class's ctor take a font size param then scale based on that.
> 
> +1 on the ctor doing htis.

Done.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/loading_animation.cc:67: double distance =
On 2013/11/15 21:58:46, Evan Stade wrote:
> nit: IMO, the distance and progress variable names don't really help me
> understand this code, so it doesn't come out any more readable than when it
was
> all on one line. Please either leave it as one line or come up with more
helpful
> variable names.

Done.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/loading_animation.cc:72: position =
kAnimationFrames[i - 1].position + distance * progress;
On 2013/11/15 23:03:38, groby wrote:
> I kind of like clearing up the computation, because it's not immediately
obvious
> what's happening in the 4-line formula. So maybe progress->frameRelativeTime?
> 
> Also, if you keep the split, i
thinkTween::FloatValueBetween(frameRelativeTime,
> kAnimationFrames[i-1].position, kAnimationFrames[i].position) does the
position
> computation nicely and skips distance.

Done.  Good call on using Tween::FloatValueBetween :)

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:531:
[loadingShieldController_ setMessage:newLoadingMessage];
On 2013/11/15 23:03:38, groby wrote:
> Should that just move into the setMessage: call?

Restructured this so that most of this logic is now encapsulated in the loading
shield.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm
(right):

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:9:
#include "base/strings/sys_string_conversions.h"
On 2013/11/15 23:03:38, groby wrote:
> base/mac/scoped_nsobject.h

Already included in the header, and this #include is actually necessary now that
I've switched setMessage: over to update: instead.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:33:
[controller_ performLayout];
On 2013/11/15 23:03:38, groby wrote:
> performLayoutWithAnimation:
> 
> You always need the animation, so no need for a separate accessor.

Done.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:40:
AutofillLoadingShieldController* const controller_;
On 2013/11/15 23:03:38, groby wrote:
> Probably worth pointing out it's not owned.

Done.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:45:
@interface AutofillOpaqueView : NSView
On 2013/11/15 23:03:38, groby wrote:
> Kill this, use NSBox.
> 
> Needs a few items set:
> [box setBoxType:NSBoxCustom];
> [box setBorderType:NSNoBorder];
> [box setContentViewMargins:NSZeroSize];
> [box setTitlePosition:NSNoTitle];

Done.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:111:
animationDriver_->animation()->Reset();
On 2013/11/15 23:03:38, groby wrote:
> You probably want a performLayout here. In general, it seems message of 0
length
> is supposed to stop animation - do we want to hide/disable the dots, too? 

This hides the shield, so there's not much reason to performLayout().  Hopefully
that's clearer with the current structure of the code.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:131: }
On 2013/11/15 23:03:38, groby wrote:
> We should be able to compute this up front, in setMessage. (I'm reluctant to
> compute a centering rectangle on demand - if height ever changes, the loading
> message will wobble)
> 
> Ideally, do the centering and placing of the dots in a separate routine from
> just the dot animation.

performLayout is now only ever called by the window controller's relayouting. 
Do you still think it's worth caching the width?  (My gut preference is always
to reduce caching unless there's some evidence for the need -- caching tends to
be fragile.)

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:136:
[message_ setFrame:messageFrame];
On 2013/11/15 23:03:38, groby wrote:
> [message setFrameOrigin:...]

Done.

https://codereview.chromium.org/73723002/diff/70001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.mm:137: for
(size_t i = 0; i < [dots_ count]; ++i) {
On 2013/11/15 23:03:38, groby wrote:
> Avoid manual iteration over NSArray. You need the index for GetCurrentValue,
so
> set it via setTag: on dot creation and just use [dot tag] to get it in the
loop.

Done.

Evan Stade

https://codereview.chromium.org/73723002/diff/160001/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/73723002/diff/160001/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode405 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:405: animation_.reset(new LoadingAnimation(this, font.GetHeight())) nit: the documentation for LoadingAnimation says ...

7 years, 1 month ago (2013-11-16 01:20:47 UTC) #5

groby-ooo-7-16

LGTM % fixes https://codereview.chromium.org/73723002/diff/160001/chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h File chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h (right): https://codereview.chromium.org/73723002/diff/160001/chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h#newcode21 chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h:21: @interface AutofillLoadingShieldController : NSViewController<AutofillLayout> { Please ...

7 years, 1 month ago (2013-11-16 01:34:59 UTC) #6

Ilya Sherman

https://codereview.chromium.org/73723002/diff/160001/chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h File chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h (right): https://codereview.chromium.org/73723002/diff/160001/chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h#newcode21 chrome/browser/ui/cocoa/autofill/autofill_loading_shield_controller.h:21: @interface AutofillLoadingShieldController : NSViewController<AutofillLayout> { On 2013/11/16 01:34:59, groby ...

7 years, 1 month ago (2013-11-16 01:46:53 UTC) #7