Mountain Lion: use the system download progress.

Unify and clean up system download notifications, add Mountain Lion support.

BUG=138962
TEST=as in bug


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151195

[Avi (use Gerrit), 2012-08-08 19:34:27]

Nico: all the Mac stuff
Randy: all the download stuff

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:120: usingBlock:(void
(^)(BOOL success))block;
Yes, this is > 80. I can't think how to format it cleanly to not be so.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:178:
ProgressString(kNSProgressFileLocationCanChangeKey) : @true,
@YES is broken for the SDK that we compile against.

[Nico, 2012-08-08 20:00:58]

nice!!

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater.h (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater.h:54: void
UpdateDownloadProgressForItemRemoved(content::DownloadItem* download);
Maybe add a small test to check these get called as expected.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:13: // --- Private 10.8
API for showing progress ---
Can you file a rdar about turning this into public api and then link to that
here? There's clearly a use case for it :-)

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:17: NSString*
kNSProgressAppBundleIdentifierKey =
NSString* const

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:64:
CFBundleGetDataPointerForName(foundation, (CFStringRef)string));
we have a type-safe casting function somewhere in base (foundation_util.h I
think?)

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:66: [cache setObject:*ref
1 line (or braces)

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:70: return [cache
objectForKey:string];
You can avoid the two cache lookups by writing it like this:

  NSString* result = [cache objectForKey:string];
  if (!result) {
    NSString** ref = ...
    if (ref) {
      result = *ref;
      [cache setObject:result forKey:string];
    }
  }
  return result;
}

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:78: @property(copy)
NSString *kind;
space after *

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:120: usingBlock:(void
(^)(BOOL success))block;
On 2012/08/08 19:34:28, Avi wrote:
> Yes, this is > 80. I can't think how to format it cleanly to not be so.

Just indent this line by 4 spaces instead. We do this in a few other places
already. (And/or add a typedef for the block, but I don't think that's
necessary)

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:143: class
NSProgressUserData : public base::SupportsUserData::Data {
CrNSProgessUserData?

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:166:
base::SysUTF8ToNSString(download->GetURL().spec())];
Huh, amazing that we don't have something nicer for nsurl <-> gurl conversions!
Oh well :-)

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:169: [NSString
stringWithUTF8String:destination_path.value().c_str()]];
This gets a tiny bit shorter with base::mac::FilePathToNSString()

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:178:
ProgressString(kNSProgressFileLocationCanChangeKey) : @true,
On 2012/08/08 19:34:28, Avi wrote:
> @YES is broken for the SDK that we compile against.

@(YES) might work

(why "YES" though? Do you know what this key means?)

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:194:
download->Cancel(true);
rdsmith: What are the guarantees for DownloadItem lifetime?

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:225: [NSString
stringWithUTF8String:download_path.value().c_str()]];
same as above

[Randy Smith (Not in Mondays), 2012-08-08 20:39:40]

LGTM for this as written, but I'm finding myself again musing on alternative
implementations.  Maybe we should update the UpdateAppIconDownloadProgress
routine to take the download that caused the update (maybe with a marker for
created, progressing, destroyed) and make that the main OS interface?  (Though
pulling into the status updater from download_util:: would be a mitvah.)

Part of the reason I'm thinking in this direction despite having ok'd this basic
approach yesterday is that I wasn't connecting that very similar work is going
on for Metro (see http://codereview.chromium.org/10832174 ) which went in a
different direction, mostly because I was asleep at the overview switch.  But
that CL has me trying to come up with a good general approach to integrate with
all the OS notification handlers, even if we don't do that now.

http://codereview.chromium.org/10827207/diff/6001/chrome/browser/download/dow...
File chrome/browser/download/download_status_updater_mac.mm (right):

http://codereview.chromium.org/10827207/diff/6001/chrome/browser/download/dow...
chrome/browser/download/download_status_updater_mac.mm:194:
download->Cancel(true);
On 2012/08/08 20:00:59, Nico wrote:
> rdsmith: What are the guarantees for DownloadItem lifetime?

If you're an observer of the DownloadItem, you'll get a notification when it's
dying.  Currently you have to catch passage through the REMOVING state, but
there's a CL in process ( http://codereview.chromium.org/10704026, but don't be
fooled by the apparent commit at the end of it--it's been landed and reverted,
and will probably be re-landed soon) to change that over to an
OnDownloadDestroyed notification to the observer.  REMOVING != IN_PROGRESS, so
this CL is good (and in general the updater should be updated as we change
that).

Avi, you should be aware of possible merge issues if the CL mentioned above goes
in before this one--it won't be a hard merge, it would just be good to make sure
it happens properly.

[Avi (use Gerrit), 2012-08-08 21:16:20]

Nico: addressed all your issues except the one that asked for a test. I'll get
on that once we figure out...
Randy: damn, I see. The issue that I see with that CL is that their
DownloadCompletionObserver duplicates a ton of the work that the
DownloadStatusUpdater already does in tracking current downloads. Porting their
code to hang off of DownloadStatusUpdater a la this CL would be trivial, and
then we have a single class keeping track of the downloads for the purposes of
notifying the OS.

What do you think?

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:13: // --- Private 10.8
API for showing progress ---
On 2012/08/08 20:00:59, Nico wrote:
> Can you file a rdar about turning this into public api and then link to that
> here? There's clearly a use case for it :-)

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:17: NSString*
kNSProgressAppBundleIdentifierKey =
On 2012/08/08 20:00:59, Nico wrote:
> NSString* const

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:64:
CFBundleGetDataPointerForName(foundation, (CFStringRef)string));
On 2012/08/08 20:00:59, Nico wrote:
> we have a type-safe casting function somewhere in base (foundation_util.h I
> think?)

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:66: [cache setObject:*ref
On 2012/08/08 20:00:59, Nico wrote:
> 1 line (or braces)

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:70: return [cache
objectForKey:string];
On 2012/08/08 20:00:59, Nico wrote:
> You can avoid the two cache lookups by writing it like this:
> 
>   NSString* result = [cache objectForKey:string];
>   if (!result) {
>     NSString** ref = ...
>     if (ref) {
>       result = *ref;
>       [cache setObject:result forKey:string];
>     }
>   }
>   return result;
> }

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:78: @property(copy)
NSString *kind;
On 2012/08/08 20:00:59, Nico wrote:
> space after *

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:120: usingBlock:(void
(^)(BOOL success))block;
On 2012/08/08 20:00:59, Nico wrote:
> On 2012/08/08 19:34:28, Avi wrote:
> > Yes, this is > 80. I can't think how to format it cleanly to not be so.
> 
> Just indent this line by 4 spaces instead. We do this in a few other places
> already. (And/or add a typedef for the block, but I don't think that's
> necessary)

Done.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:143: class
NSProgressUserData : public base::SupportsUserData::Data {
On 2012/08/08 20:00:59, Nico wrote:
> CrNSProgessUserData?

Moved to anon namespace.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:169: [NSString
stringWithUTF8String:destination_path.value().c_str()]];
Ooh!

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:178:
ProgressString(kNSProgressFileLocationCanChangeKey) : @true,
On 2012/08/08 20:00:59, Nico wrote:
> @(YES) might work

Doesn't that determine at runtime what the type of the expression between the @(
and ) are? In any case, Safari sets that key. I'm setting it too.

Since @YES and @true are equivalent, I'm sticking to this.

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:194:
download->Cancel(true);
As you noted, the DSU code would need to be updated too, and this piggybacks on
it. What should I be watching out for?

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:225: [NSString
stringWithUTF8String:download_path.value().c_str()]];
On 2012/08/08 20:00:59, Nico wrote:
> same as above

Done.

[Avi (use Gerrit), 2012-08-08 21:19:30]

Oh, and if I port their CL to be atop DownloadStatusUpdater, and I write the
test Nico wanted, then they at least get some test coverage, of which they have
none now.

[Nico, 2012-08-08 21:21:38]

LGTM!

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:143: class
NSProgressUserData : public base::SupportsUserData::Data {
On 2012/08/08 21:16:20, Avi wrote:
> On 2012/08/08 20:00:59, Nico wrote:
> > CrNSProgessUserData?
> 
> Moved to anon namespace.

I'd still call it CrNSProgressUserData, I consider all NSFoo to be in apple's
namespace. But I don't feel strongly, so keep it as is if you prefer that.

[Avi (use Gerrit), 2012-08-08 21:22:46]

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:143: class
NSProgressUserData : public base::SupportsUserData::Data {
On 2012/08/08 21:21:38, Nico wrote:
> I consider all NSFoo to be in apple's
> namespace.

Even for C++ classes?

[Nico, 2012-08-08 21:27:23]

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:143: class
NSProgressUserData : public base::SupportsUserData::Data {
On 2012/08/08 21:22:46, Avi wrote:
> On 2012/08/08 21:21:38, Nico wrote:
> > I consider all NSFoo to be in apple's
> > namespace.
> 
> Even for C++ classes?

Even for poetry.

[Randy Smith (Not in Mondays), 2012-08-08 21:27:42]

I definitely think we should be doing the same thing for both situations.  If
you want to volunteer to do the port, I'd be happy and guilty (as I consider the
responsibility for the split to be mine).

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
File chrome/browser/download/download_status_updater_mac.mm (right):

https://chromiumcodereview.appspot.com/10827207/diff/6001/chrome/browser/down...
chrome/browser/download/download_status_updater_mac.mm:194:
download->Cancel(true);
On 2012/08/08 21:16:20, Avi wrote:
> As you noted, the DSU code would need to be updated too, and this piggybacks
on
> it. What should I be watching out for?

A glitch in the DSU code update :-}.  The DSU code update may substantially move
where the removal from the tracked list occurs (since it's under a different
entry point), and that'll probably get a delete/change merge conflict for
you--I'm mostly just giving you a heads up about it.

(Just checked the other CL and it hadn't modified DSU at all.  Which is ok,
since we always move from IN_PROGRESS->CANCELLED/INTERRUPTED before destruction,
but I've still requested hooking OnDownloadDestroyed.)

[Avi (use Gerrit), 2012-08-09 15:06:15]

Nico: Mac stuff; since you last saw it: moved download finished code here.
Rando: Download stuff; since you last saw it: unit tested!
Asanka: Win stuff; new for you, but just shuffling code around.

[Nico, 2012-08-09 15:11:01]

mac still looks fine

[asanka, 2012-08-09 15:23:24]

https://chromiumcodereview.appspot.com/10827207/diff/14001/chrome/browser/dow...
File chrome/browser/download/download_status_updater.cc (right):

https://chromiumcodereview.appspot.com/10827207/diff/14001/chrome/browser/dow...
chrome/browser/download/download_status_updater.cc:109:
UpdateDownloadProgressForItemCompleted(download);
This is going to be called for states other than COMPLETE. (I.e. CANCELLED and
INTERRUPTED also end up here.) The handler for Windows expects that it is only
called on successful completion.

[Avi (use Gerrit), 2012-08-09 19:04:23]

https://chromiumcodereview.appspot.com/10827207/diff/14001/chrome/browser/dow...
File chrome/browser/download/download_status_updater.cc (right):

https://chromiumcodereview.appspot.com/10827207/diff/14001/chrome/browser/dow...
chrome/browser/download/download_status_updater.cc:109:
UpdateDownloadProgressForItemCompleted(download);
Oops. Fixed.

[asanka, 2012-08-09 19:35:21]

download_status_updater_win.* LGTM

[Randy Smith (Not in Mondays), 2012-08-09 20:12:25]

http://codereview.chromium.org/10827207/diff/13018/chrome/browser/download/do...
File chrome/browser/download/download_status_updater_unittest.cc (right):

http://codereview.chromium.org/10827207/diff/13018/chrome/browser/download/do...
chrome/browser/download/download_status_updater_unittest.cc:264:
EXPECT_GE(updater_->GetStartedCount(), orig_started);
Why are these GE rather than GT (throughout this function)?  It seems like
you're really only testing "We *don't* update progressed/completed on add, We
*don't* set completed on progress" rather than testing that you actually do get
progress info.

[Avi (use Gerrit), 2012-08-09 20:29:30]

You are right, those should be GT.

As discussed, this test is moot :(

[Avi (use Gerrit), 2012-08-10 17:43:04]

New version, new connection to the status updater.

Elliot, you get gtk. Everyone else, you know what you're looking at. The
functionality is the same, just a little different wiring.

[Elliot Glaysher, 2012-08-10 18:17:27]

gtk lgtm

[Randy Smith (Not in Mondays), 2012-08-10 18:38:44]

Could you do me a favor and either:
a) Add a note at DownloadItem::Observer::OnDownloadDestroyed() saying that that
function will never be called while the DownloadItem is in the IN_PROGRESS
state, or
b) Add a note in download_status_updater.h before UpdateAppIconDownloadProgress
saying that no pointers to the DownloadItem should be held by platform specific
code outside of UserData?

Thanks very much, by the way, for doing this--I'm excited to see all this
related code converging on DownloadStatusUpdater.

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
chrome/browser/download/download_status_updater.cc:99: #if defined(USE_AURA) ||
defined(ANDROID)
Avi: Happy to go with your opinion on this, but wanted to raise the question:
Should we mix inline ifdefs with platform specific files as solutions for the
same problem?  (Platform specific definitions of routines.)

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
File chrome/browser/download/download_status_updater_unittest.cc (right):

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
chrome/browser/download/download_status_updater_unittest.cc:27:
content::DownloadItem* download) OVERRIDE {
Shouldn't you add a test to make sure the right download item is passed in some
variety of circumstances?

[Randy Smith (Not in Mondays), 2012-08-10 18:40:44]

Sorry, should also have said: Reviewed
download_status_updater{.h,.cc._unittest.cc}, download_util.*,
supports_user_data.* (though not OWNER).  Left platform specific files to
platform people and Asanka.

On 2012/08/10 18:38:44, rdsmith wrote:
> Could you do me a favor and either:
> a) Add a note at DownloadItem::Observer::OnDownloadDestroyed() saying that
that
> function will never be called while the DownloadItem is in the IN_PROGRESS
> state, or
> b) Add a note in download_status_updater.h before
UpdateAppIconDownloadProgress
> saying that no pointers to the DownloadItem should be held by platform
specific
> code outside of UserData?
> 
> Thanks very much, by the way, for doing this--I'm excited to see all this
> related code converging on DownloadStatusUpdater.
> 
>
http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
> File chrome/browser/download/download_status_updater.cc (right):
> 
>
http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
> chrome/browser/download/download_status_updater.cc:99: #if defined(USE_AURA)
||
> defined(ANDROID)
> Avi: Happy to go with your opinion on this, but wanted to raise the question:
> Should we mix inline ifdefs with platform specific files as solutions for the
> same problem?  (Platform specific definitions of routines.)
> 
>
http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
> File chrome/browser/download/download_status_updater_unittest.cc (right):
> 
>
http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
> chrome/browser/download/download_status_updater_unittest.cc:27:
> content::DownloadItem* download) OVERRIDE {
> Shouldn't you add a test to make sure the right download item is passed in
some
> variety of circumstances?

[Avi (use Gerrit), 2012-08-10 19:25:43]

(Pondering how to test this; would like to land this.)

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
chrome/browser/download/download_status_updater.cc:99: #if defined(USE_AURA) ||
defined(ANDROID)
There are two other options that I can think of:

- download_status_updater_android and download_status_updater_aura that only
have this empty definition
- #ifdefing all the callers

Neither seems better to me.

[Randy Smith (Not in Mondays), 2012-08-10 19:35:14]

In terms of testing, you could create a variant of OneManagerManyItems that
confirmed at each point that you provoked a notification (ModelChanged(),
CompleteItem--bit sad that that's it) that the overridden function received the
correct DownloadItem(s)?

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10827207/diff/14004/chrome/browser/download/do...
chrome/browser/download/download_status_updater.cc:99: #if defined(USE_AURA) ||
defined(ANDROID)
On 2012/08/10 19:25:43, Avi wrote:
> There are two other options that I can think of:
> 
> - download_status_updater_android and download_status_updater_aura that only
> have this empty definition
> - #ifdefing all the callers
> 
> Neither seems better to me.

SG; just wanted to ask the question.

[Avi (use Gerrit), 2012-08-10 20:20:10]

Added a test.

Randy: is the download code and the test satisfactory?
Nico: is the Mac code satisfactory?
Asanka: is the Windows code satisfactory?

[Nico, 2012-08-10 20:24:31]

mac still lgtm

[Randy Smith (Not in Mondays), 2012-08-10 20:35:52]

All files I was mentioning as reviewing earlier LGTM.

Avi, should have said earlier: Asanka's not in today.  I don't know if he'll be
online over the weekend (though he often is); how do you feel about waiting for
Monday for a final LGTM?  (Having said that, I'll scan the files I imagine he'd
be reviewing and see if I feel comfortable reviewing them.)

[Randy Smith (Not in Mondays), 2012-08-10 20:42:08]

download_status_updater_win.* LGTM.  I'm not a windows expert, but I think
combined with Asanka's earlier LGTM you're probably ok.  But you might want to
wait for Asanka if you changed the guts of the Windows code since his LGTM.

[Avi (use Gerrit), 2012-08-10 21:00:37]

On 2012/08/10 20:42:08, rdsmith wrote:
> download_status_updater_win.* LGTM.  I'm not a windows expert, but I think
> combined with Asanka's earlier LGTM you're probably ok.  But you might want to
> wait for Asanka if you changed the guts of the Windows code since his LGTM.

There is no change to the Windows code since Asanka's last LG, only to the
plumbing that leads up to it. The wiring is different enough that I can wait.

[asanka, 2012-08-11 05:29:20]

download_status_updater_win.* LGTM. Thanks!

https://chromiumcodereview.appspot.com/10827207/diff/1012/chrome/browser/down...
File chrome/browser/download/download_status_updater.cc (right):

https://chromiumcodereview.appspot.com/10827207/diff/1012/chrome/browser/down...
chrome/browser/download/download_status_updater.cc:99: #if defined(USE_AURA) ||
defined(ANDROID)
defined(OS_ANDROID)?

[Avi (use Gerrit), 2012-08-11 16:29:03]

http://codereview.chromium.org/10827207/diff/1012/chrome/browser/download/dow...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10827207/diff/1012/chrome/browser/download/dow...
chrome/browser/download/download_status_updater.cc:99: #if defined(USE_AURA) ||
defined(ANDROID)
On 2012/08/11 05:29:20, asanka wrote:
> defined(OS_ANDROID)?

Done.

[commit-bot: I haz the power, 2012-08-11 16:29:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10827207/5040

[commit-bot: I haz the power, 2012-08-11 16:29:21]

Presubmit check for 10827207-5040 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

Presubmit checks took 1.8s to calculate.

[Avi (use Gerrit), 2012-08-11 16:33:34]

Brett: OWNER stamp for base?

[brettw, 2012-08-11 17:12:16]

base lgtm

[commit-bot: I haz the power, 2012-08-11 17:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10827207/5040

[commit-bot: I haz the power, 2012-08-12 02:25:15]

Change committed as 151195