Created uber tray UI for monitoring ongoing WebDrive sync operations

Created uber tray UI for monitoring ongoing WebDrive sync operations


BUG=chromium-os:30075
TEST=none

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

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

[sadrul, 2012-05-01 14:28:58]

Changes in ui/, ash/ and chromeos/system/ LGTM with the nits.

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/drive_obs...
File ash/system/drive/drive_observer.h (right):

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/drive_obs...
ash/system/drive/drive_observer.h:9: #include <vector>
nit: You probably don't need this here.

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
File ash/system/drive/tray_drive.cc (right):

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:32: string16 GetTrayLabel(const
ash::DriveOperationStatusList& list) {
These two functions should either be static or in anonymous namespace. (I think
the preference is the latter)

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:87: in_progress_img_(NULL), done_img_(NULL),
failed_img_(NULL) {
Each in its own line.

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:109: //    AppendSettings();
Left out intentionally?

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:124: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:134: progress_bar_(NULL), cancel_button_(NULL),
file_path_(file_path) {
each in its own line

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:235: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:290: operations_->GetLayoutManager());
It doesn't look like layout is used after this. So move the declaration from
line 281 into line 284?

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.cc:373: scoped_ptr<DriveOperationStatusList>
list(GetCurrentOperationList());
Is there a chance the tray can show up when logged in incognito (i.e.
LoginStatus != USER || OWNER)?

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_drive.h
File ash/system/drive/tray_drive.h (right):

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.h:26: };
I think we avoid ;s after namespace

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.h:28: class TrayItemView;
- this

http://codereview.chromium.org/10214013/diff/21003/ash/system/drive/tray_driv...
ash/system/drive/tray_drive.h:56: scoped_ptr<tray::DriveDetailedView> detailed_;
stevenjb@ has a CL (http://codereview.chromium.org/10269017/) that removes the
scoped_ptr's like these and replace them with a normal pointer. Let's do that
here too.

http://codereview.chromium.org/10214013/diff/21003/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10214013/diff/21003/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:1006:
GetProgressStatusList());
4-space indent

[xiyuan, 2012-05-01 16:47:52]

LGTM with nits

You might want to check in binary files first so that you can run try jobs and
use CQ.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/d...
File ash/system/drive/drive_observer.h (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/d...
ash/system/drive/drive_observer.h:10: #include
"ash/system/tray/system_tray_delegate.h"
nit: seems this one is not used either.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
File ash/system/drive/tray_drive.cc (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:59: explicit DriveDefaultView(SystemTrayItem*
owner,
nit: no need to have "explicit" when having more than one arg

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:119: 
nit: empty line before "private:"?

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:137: status_img_->set_focusable(false);
nit: probably don't need this as ImageView is not focusable by default.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:154: cancel_button_->set_focusable(false);
nit: probably don't need this as ImageButton/CustomButton is not focusable by
default.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:163: void UpdateStatus(DriveDetailedView* parent,
nit: since we have member parent_ now, probably don't need to pass it here.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:174: 
nit: nuke this empty line?

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:186: 
nit: nuke this empty line?

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:203: pos_x +=
status_img_->GetPreferredSize().width() + kHorizontalPadding;
nit: status_img_->GetPreferredSize().width() -> status_img_->bounds().width()

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:205: gfx::Rect bounds_label(pos_x, pos_y,
nit: pos_y on its own line

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:209: cancel_button_->GetPreferredSize().width(),
nit: indent above 3 lines to make it clear that they are part of one expression
to get width.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:237: struct OperationItem {
nit: Is this still in use?

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:354: }
nit: insert an empty line

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:377: TrayDrive::~TrayDrive() {
nit: move this up to keep the same order as in header.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
File ash/system/drive/tray_drive.h (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.h:13: 
nit: nuke one empty line

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/tray/sy...
File ash/system/tray/system_tray.cc (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/tray/sy...
ash/system/tray/system_tray.cc:463: : progress(0.0), type(OPERATION_OTHER),
state(OPERATION_NOT_STARTED) {
nit: each member on its own line

https://chromiumcodereview.appspot.com/10214013/diff/21003/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:988: 
nit: nuke empty line.

https://chromiumcodereview.appspot.com/10214013/diff/21003/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:1094:
scoped_ptr<base::WeakPtrFactory<SystemTrayDelegate> > ui_weak_ptr_factory_;
nit: Any reason to put base::WeakPtrFactory in a scoped_ptr?
"base::WeakPtrFactory<SystemTrayDelegate> ui_weak_ptr_factory_" should work.

[zel, 2012-05-01 16:56:52]

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/d...
File ash/system/drive/drive_observer.h (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/d...
ash/system/drive/drive_observer.h:9: #include <vector>
On 2012/05/01 14:28:58, sadrul wrote:
> nit: You probably don't need this here.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
File ash/system/drive/tray_drive.cc (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:32: string16 GetTrayLabel(const
ash::DriveOperationStatusList& list) {
On 2012/05/01 14:28:58, sadrul wrote:
> These two functions should either be static or in anonymous namespace. (I
think
> the preference is the latter)

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:59: explicit DriveDefaultView(SystemTrayItem*
owner,
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: no need to have "explicit" when having more than one arg

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:87: in_progress_img_(NULL), done_img_(NULL),
failed_img_(NULL) {
On 2012/05/01 14:28:58, sadrul wrote:
> Each in its own line.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:109: //    AppendSettings();
On 2012/05/01 14:28:58, sadrul wrote:
> Left out intentionally?

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:119: 
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: empty line before "private:"?

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:124: };
On 2012/05/01 14:28:58, sadrul wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:134: progress_bar_(NULL), cancel_button_(NULL),
file_path_(file_path) {
On 2012/05/01 14:28:58, sadrul wrote:
> each in its own line

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:137: status_img_->set_focusable(false);
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: probably don't need this as ImageView is not focusable by default.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:154: cancel_button_->set_focusable(false);
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: probably don't need this as ImageButton/CustomButton is not focusable by
> default.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:163: void UpdateStatus(DriveDetailedView* parent,
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: since we have member parent_ now, probably don't need to pass it here.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:163: void UpdateStatus(DriveDetailedView* parent,
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: since we have member parent_ now, probably don't need to pass it here.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:186: 
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: nuke this empty line?

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:203: pos_x +=
status_img_->GetPreferredSize().width() + kHorizontalPadding;
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: status_img_->GetPreferredSize().width() -> status_img_->bounds().width()

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:209: cancel_button_->GetPreferredSize().width(),
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: indent above 3 lines to make it clear that they are part of one
expression
> to get width.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:235: };
On 2012/05/01 14:28:58, sadrul wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:237: struct OperationItem {
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: Is this still in use?

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:290: operations_->GetLayoutManager());
On 2012/05/01 14:28:58, sadrul wrote:
> It doesn't look like layout is used after this. So move the declaration from
> line 281 into line 284?

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:354: }
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: insert an empty line

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:373: scoped_ptr<DriveOperationStatusList>
list(GetCurrentOperationList());
On 2012/05/01 14:28:58, sadrul wrote:
> Is there a chance the tray can show up when logged in incognito (i.e.
> LoginStatus != USER || OWNER)?

Nope. The user needs to be fully authenticated and have legit OAuth1+2 tokens
for drive to work.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.cc:377: TrayDrive::~TrayDrive() {
On 2012/05/01 16:47:53, xiyuan wrote:
> nit: move this up to keep the same order as in header.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
File ash/system/drive/tray_drive.h (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.h:26: };
On 2012/05/01 14:28:58, sadrul wrote:
> I think we avoid ;s after namespace

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.h:28: class TrayItemView;
On 2012/05/01 14:28:58, sadrul wrote:
> - this

Removed.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.h:56: scoped_ptr<tray::DriveDetailedView> detailed_;
On 2012/05/01 14:28:58, sadrul wrote:
> stevenjb@ has a CL (http://codereview.chromium.org/10269017/) that removes the
> scoped_ptr's like these and replace them with a normal pointer. Let's do that
> here too.

Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/ash/system/drive/t...
ash/system/drive/tray_drive.h:56: scoped_ptr<tray::DriveDetailedView> detailed_;
On 2012/05/01 14:28:58, sadrul wrote:
> stevenjb@ has a CL (http://codereview.chromium.org/10269017/) that removes the
> scoped_ptr's like these and replace them with a normal pointer. Let's do that
> here too.

Yes, good point. Done.

https://chromiumcodereview.appspot.com/10214013/diff/21003/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/10214013/diff/21003/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:1006:
GetProgressStatusList());
On 2012/05/01 14:28:58, sadrul wrote:
> 4-space indent

Done.

[sky, 2012-05-01 17:17:11]

LGTM with the following fixes.

http://codereview.chromium.org/10214013/diff/14023/ash/system/drive/drive_obs...
File ash/system/drive/drive_observer.h (right):

http://codereview.chromium.org/10214013/diff/14023/ash/system/drive/drive_obs...
ash/system/drive/drive_observer.h:15: virtual ~DriveObserver() {}
As the consumer of this doesn't own the observer make the destructor protected.

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:462: DriveOperationStatus::DriveOperationStatus()
these (and similar structs) belong in system_tray_delegate.cc .

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
File ash/system/tray/system_tray_delegate.h (right):

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
ash/system/tray/system_tray_delegate.h:52: enum OperationType {
nit: enums before constructor.

[zel, 2012-05-01 17:45:40]

http://codereview.chromium.org/10214013/diff/14023/ash/system/drive/drive_obs...
File ash/system/drive/drive_observer.h (right):

http://codereview.chromium.org/10214013/diff/14023/ash/system/drive/drive_obs...
ash/system/drive/drive_observer.h:15: virtual ~DriveObserver() {}
On 2012/05/01 17:17:12, sky wrote:
> As the consumer of this doesn't own the observer make the destructor
protected.

Done.

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:462: DriveOperationStatus::DriveOperationStatus()
On 2012/05/01 17:17:12, sky wrote:
> these (and similar structs) belong in system_tray_delegate.cc .

Created system_tray_delegate.cc, moved those structs there.

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
File ash/system/tray/system_tray_delegate.h (right):

http://codereview.chromium.org/10214013/diff/14023/ash/system/tray/system_tra...
ash/system/tray/system_tray_delegate.h:52: enum OperationType {
On 2012/05/01 17:17:12, sky wrote:
> nit: enums before constructor.

Done.

[commit-bot: I haz the power, 2012-05-01 20:15:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/10214013/17028

[commit-bot: I haz the power, 2012-05-01 20:41:36]

Try job failure for 10214013-17028 (retry) on win_rel for step "update" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.