Initial Implementation of Download Notification

Initial Implementation of Download Notification

This is the first implementation of Download Notification for ChromeOS.
This feature is behind a flag and disabled by default as for now.

The major changes of the patch are:
- Add DownloadNotificationManager, which replaces
    DefaultUIControllerDelegate
- Add DownloadNotificationItem, which represents each  notification
- Add DownloadCommands, which is extracted from
    DownloadShelfContextMenu::ContextMenuCommands
- Rename DefaultUIControllerDelegate ->
    DownloadShelfUIControllerDelegate
- Rename: DefaultUIControllerDelegateAndroid ->
    AndroidUIControllerDelegate

BUG=446112
TEST=browser_test passes

Committed: https://crrev.com/f42d8fc065c006e4590476b9526c8b5bd2563d6d
Cr-Commit-Position: refs/heads/master@{#319113}

[asanka, 2015-01-23 19:43:52]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2015-01-23 19:43:53]

Sorry about the delay.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:25: class DownloadAction {
See DownloadShelfContextMenu::ContextMenuCommands. We should consolidate these.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:67: base::string16 GetTitle()
const;
DownloadItemModel is now used to back the downloads shelf, the download page
(chrome://downlaods) and download notifications. I think it makes sense to split
these into separate classes.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_service.cc (right):

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_service.cc:14: #include
"chrome/browser/download/notification/download_notification_manager.h"
Why is this necessary?

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/noti...
File chrome/browser/download/notification/download_notification_manager.cc
(right):

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/noti...
chrome/browser/download/notification/download_notification_manager.cc:237:
DownloadNotificationManager::DownloadNotificationManager(Profile* profile)
What is the behavior going to be for multi-profile and incognito profiles?

[yoshiki, 2015-01-29 20:20:46]

Asanka, thank you for reviewing the wip patch and giving me good guidances. I
updated a new patch to commit to the repository. Could you review this patch?
Thanks.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:25: class DownloadAction {
On 2015/01/23 19:43:53, asanka wrote:
> See DownloadShelfContextMenu::ContextMenuCommands. We should consolidate
these.

Thanks for guidance. I extracted DownloadCommands to use in both places.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:67: base::string16 GetTitle()
const;
On 2015/01/23 19:43:53, asanka wrote:
> DownloadItemModel is now used to back the downloads shelf, the download page
> (chrome://downlaods) and download notifications. I think it makes sense to
split
> these into separate classes.

I agree that. I'll split this class into 3 (and maybe 1 base class) in the
separate patch.

As for this patch, I moved these code which is used only in download
notification into the DownloadNotificationItem class.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_service.cc (right):

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_service.cc:14: #include
"chrome/browser/download/notification/download_notification_manager.h"
On 2015/01/23 19:43:53, asanka wrote:
> Why is this necessary?

Just a mistake. Thanks.

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/noti...
File chrome/browser/download/notification/download_notification_manager.cc
(right):

https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/noti...
chrome/browser/download/notification/download_notification_manager.cc:237:
DownloadNotificationManager::DownloadNotificationManager(Profile* profile)
On 2015/01/23 19:43:53, asanka wrote:
> What is the behavior going to be for multi-profile and incognito profiles?

I checked and it works well. Since download IDs and notification IDs are unique
among different profiles, we don't need to care the profile in the code.

[asanka, 2015-01-31 00:38:44]

On 2015/01/29 at 20:20:46, yoshiki wrote:
> Asanka, thank you for reviewing the wip patch and giving me good guidances. I
updated a new patch to commit to the repository. Could you review this patch?
Thanks.
> 
>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
> File chrome/browser/download/download_item_model.h (right):
> 
>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
> chrome/browser/download/download_item_model.h:25: class DownloadAction {
> On 2015/01/23 19:43:53, asanka wrote:
> > See DownloadShelfContextMenu::ContextMenuCommands. We should consolidate
these.
> 
> Thanks for guidance. I extracted DownloadCommands to use in both places.

You should also consider moving the logic for executing these commands.

>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
> chrome/browser/download/download_item_model.h:67: base::string16 GetTitle()
const;
> On 2015/01/23 19:43:53, asanka wrote:
> > DownloadItemModel is now used to back the downloads shelf, the download page
> > (chrome://downlaods) and download notifications. I think it makes sense to
split
> > these into separate classes.
> 
> I agree that. I'll split this class into 3 (and maybe 1 base class) in the
separate patch.
> 
> As for this patch, I moved these code which is used only in download
notification into the DownloadNotificationItem class.
> 
>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
> File chrome/browser/download/download_service.cc (right):
> 
>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/down...
> chrome/browser/download/download_service.cc:14: #include
"chrome/browser/download/notification/download_notification_manager.h"
> On 2015/01/23 19:43:53, asanka wrote:
> > Why is this necessary?
> 
> Just a mistake. Thanks.
> 
>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/noti...
> File chrome/browser/download/notification/download_notification_manager.cc
(right):
> 
>
https://codereview.chromium.org/852043002/diff/1/chrome/browser/download/noti...
> chrome/browser/download/notification/download_notification_manager.cc:237:
DownloadNotificationManager::DownloadNotificationManager(Profile* profile)
> On 2015/01/23 19:43:53, asanka wrote:
> > What is the behavior going to be for multi-profile and incognito profiles?
> 
> I checked and it works well. Since download IDs and notification IDs are
unique among different profiles, we don't need to care the profile in the code.

That was more of a UI question. I assume they all end up in the same
notification center. Are the UI folks okay with incognito and regular downloads
showing up in the same notification center?

[asanka, 2015-01-31 00:38:51]

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_commands.cc:31: default:
Don't use a 'default' clause. The compiler will warn against any unhandled
values and the build will break if someone adds a new command without also
handling it here.

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_commands.cc:52: default:
Ditto.

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_commands.h (right):

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_commands.h:10: enum DownloadCommands {
Use a suitable prefix for these enum values.

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
File chrome/browser/download/notification/download_notification_manager.h
(right):

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/notification/download_notification_manager.h:21: //
DownloadManager::Observer:
DownloadUIController::Delegate.

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/notification/download_notification_manager.h:23: 
// DownloadNotificationItem::Delegate

[yoshiki, 2015-02-04 19:03:01]

Asanka, PTAL again? Thanks.

The followings are major changes from the patch set #2.
- Created DownloadCommand class and moved the command-related code to the class.
- Made a notification use an incognito icon for incognito download (the current
icon is tentative and will be changed soon after new icon arrives from UX)

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
File chrome/browser/download/notification/download_notification_manager.h
(right):

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/notification/download_notification_manager.h:21: //
DownloadManager::Observer:
On 2015/01/31 00:38:51, asanka wrote:
> DownloadUIController::Delegate.

Done.

https://codereview.chromium.org/852043002/diff/20001/chrome/browser/download/...
chrome/browser/download/notification/download_notification_manager.h:23: 
On 2015/01/31 00:38:51, asanka wrote:
> // DownloadNotificationItem::Delegate

Done.

[yoshiki, 2015-02-09 20:21:13]

Hi Asanka,

Could you take a look at this CL when you get a chance? If you have any
questions, please let me know. Thanks.

[asanka, 2015-02-10 02:25:30]

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_command.h (right):

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_command.h:13: class DownloadCommand {
Given how it's used, I'd probably go with DownloadCommands as the class name and
Command as the enum name.

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_command.h:33: int GetCommandIconId(Commands
type);
Taken out of context, methods that return string IDs and icon IDs don't make
sense in this abstraction. The returned IDs are only valid when used in specific
circumstances. This goes for the GetCommandStringId* functions below as well.

We could keep the GetCommandStringId* type methods in the abstractions where the
name is unambigious. I.e. keep GetCommandStringId* logic in
DownloadShelfContextMenu.

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
chrome/browser/download/notification/download_notification_item.cc:54: id +=
base::UintToString(item->GetId());
Is the generated ID only expected to be valid for a single profile? What happens
if there are multiple profiles?

[yoshiki, 2015-02-10 17:49:17]

Asanka, thank you for comments. PTAL at new patchset.

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_command.h (right):

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_command.h:13: class DownloadCommand {
On 2015/02/10 02:25:30, asanka wrote:
> Given how it's used, I'd probably go with DownloadCommands as the class name
and
> Command as the enum name.

Thanks for guidance. Done.

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_command.h:33: int GetCommandIconId(Commands
type);
On 2015/02/10 02:25:30, asanka wrote:
> Taken out of context, methods that return string IDs and icon IDs don't make
> sense in this abstraction. The returned IDs are only valid when used in
specific
> circumstances. This goes for the GetCommandStringId* functions below as well.
> 
> We could keep the GetCommandStringId* type methods in the abstractions where
the
> name is unambigious. I.e. keep GetCommandStringId* logic in
> DownloadShelfContextMenu.

I made these public methods return string16/Image instead of IDs, and kept the
methods which return IDs private.

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/852043002/diff/80001/chrome/browser/download/...
chrome/browser/download/notification/download_notification_item.cc:54: id +=
base::UintToString(item->GetId());
On 2015/02/10 02:25:30, asanka wrote:
> Is the generated ID only expected to be valid for a single profile? What
happens
> if there are multiple profiles?

Download IDs are unique among profiles and notification IDs aren't conflicted
even under multi-user environment.

[yoshiki, 2015-02-13 01:40:53]

Asanka, ping? If you have any questions. Please feel free to ask me. Thanks.

[asanka, 2015-02-13 21:17:06]

My overarching comment is to make DownloadCommands be UI agnostic as possible.
Other than that, the rest are nits and I think we could consider landing and
iterating.

Can we have basic functional tests? I.e. tests that make sure that a
notification pops up when the first download starts, and that the browser
doesn't crash if the DownloadItem is destroyed etc.

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_commands.cc:255: return for_menu ?
IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE :
Rather than making each cast conditional, shall we have a switch statement in
each of GetCommandString*?

In the end it might be better to have GetCommandString() implementation sit in
the class that uses it. I.e. have DownloadShelfContextMenu::GetCommandString()
and similarly for notifications.

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
File chrome/browser/download/download_commands.h (right):

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_commands.h:36: base::string16
GetCommandString(Command command) const;
Could you explain the intended difference between these two methods?

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:89: return false;
Why this change?

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:98: base::string16
DownloadShelfContextMenu::GetStringForCommand(
Is there a need for GetStringForCommand() and a GetLabelForCommand()?

Also, once again I feel that the strings are specific to the menu and the
lookups should be here rather than in DownloadCommand.

[yoshiki, 2015-02-19 14:27:59]

Patchset #8 (id:140001) has been deleted

[yoshiki, 2015-02-19 14:28:08]

Patchset #7 (id:120001) has been deleted

[yoshiki, 2015-02-19 14:28:32]

Patchset #7 (id:160001) has been deleted

[yoshiki, 2015-02-19 14:49:57]

Patchset #7 (id:180001) has been deleted

[yoshiki, 2015-02-19 14:50:11]

Patchset #7 (id:180001) has been deleted

[yoshiki, 2015-02-19 15:07:02]

Thank you for good review comments. I added tests and modification. Could you
take a look again?

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_commands.cc:255: return for_menu ?
IDS_DOWNLOAD_MENU_OPEN_WHEN_COMPLETE :
On 2015/02/13 21:17:05, asanka wrote:
> Rather than making each cast conditional, shall we have a switch statement in
> each of GetCommandString*?
> 
> In the end it might be better to have GetCommandString() implementation sit in
> the class that uses it. I.e. have DownloadShelfContextMenu::GetCommandString()
> and similarly for notifications.

Done. I moved this to DownloadShelfContextMenu and DowndloadNotificationItem.

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
File chrome/browser/download/download_commands.h (right):

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_commands.h:36: base::string16
GetCommandString(Command command) const;
On 2015/02/13 21:17:05, asanka wrote:
> Could you explain the intended difference between these two methods?

(however I removed these methods)
We show longer messages for notification than ones for menu, hence there are 2
methods (one for menu, one for notifications) to get the strings.

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:89: return false;
On 2015/02/13 21:17:06, asanka wrote:
> Why this change?

Because we split TOGGLE_PAUSE into PAUSE and RESUME, and the menu item does no
longer dynamically change it's label. And I just found the bug about this change
and fixed it by adding GetInProgressPausedMenuModel() method.

https://codereview.chromium.org/852043002/diff/100001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:98: base::string16
DownloadShelfContextMenu::GetStringForCommand(
On 2015/02/13 21:17:05, asanka wrote:
> Is there a need for GetStringForCommand() and a GetLabelForCommand()?
> 
> Also, once again I feel that the strings are specific to the menu and the
> lookups should be here rather than in DownloadCommand.

Sorry, I misunderstood. I thought DownloadCommand => int conversion was
prohibited due to -fpermissive, but it is actually allowed.

Then I removed one and combine them.

[asanka, 2015-02-20 23:34:40]

Very close. Just a few more nits and we should be able to land this.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/download_commands.cc:140: content::PageNavigator*
navigator) {
indent.

Perhaps run this through 'git cl format'

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:112: delegate_.reset(new
DefaultUIControllerDelegate(
Shall we rename the UIControllerDelegates?

DefaultUIControllerDelegate -> DownloadShelfUIControllerDelegate

DefaultUIControllerDelegateAndroid -> AndroidUIControllerDelegate

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:107: //
Reshows the notification.
Can you explain what this logic does?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:117:
delegate_->OnDownloadRemoved(this);
Is the OnDownloadRemoved call going to destroy |this|?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:146:
DownloadCommands(item_).ExecuteCommand(command, item_->GetWebContents());
GetWebContents() can (and often will) return NULL if the WebContents that
initiated the download has gone away.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:164:
DownloadItemModel* model = new DownloadItemModel(item_);
This leaks.

In general, DownloadItemModel is a thin wrapper, and you don't need to allocate
it on the heap. You can use "DownloadItemModel model(item_);"

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:188: //
TODO(yoshiki): Shows an appreciate icon.
appreciate?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:191: default:
Don't use a 'default' label when using an enumeration with switch(). Instead
handle all the values individually.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:273:
delegate_->OnDownloadRemoved(this);
DownloadNOtificationItem being a RefCounted class makes me worried about
lifetime issues. |item| is likely going to be destroyed soon (and will also emit
an OnDownloadDestroyed() notification). If DownloadNOtificationItem outlives
DownloadItem, then the |item_| member is going to be a dangling pointer. Can you
resolve that?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:291:
actions->push_back(DownloadCommands::SHOW_IN_FOLDER);
YOu don't want to SHOW_IN_FOLDER if it's dangerous.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.h (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:58: void
OnDownloadRemoved(content::DownloadItem* item) override;
You also need to listen for OnDownloadDestroyed().

If you see this notification, then |item| is about to be deleted.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:60: void
SetNotificationData();
Reword? It's updating the notification based on the state of the item.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:61: void
SetImageToNotification(int resource_id);
Reword?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item_unittest.cc
(right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:142:
// Makes sure the DownloadItem::Close() is not called.
DownloadItem::Cancel(). Here and below.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:182:
TEST_F(DownloadNotificationItemTest, OpenDownloadTest) {
Nit: Individual tests don't need to be called *Test since that's implicit.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:210:
// Toggles open-when-complete (new value: true).
Nit: new value is false?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:239:

Extra whitespace

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_manager.cc
(right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_manager.cc:28: void
DownloadNotificationManager::UpdateDownloadingMultipleFiles() {
Might be worth leaving this out until it's ready to be implemented, or implement
it now.

Also, can you reword this?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_manager.cc:65:
item->AddRef();
Rather than manually refcounting, you can make the items_ vector contain
scoped_refptr<>s instead.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_manager.cc:65:
item->AddRef();
Rather than manually refcounting, you can make the items_ vector contain
scoped_refptr<>s instead.

[yoshiki, 2015-02-24 21:28:07]

Patchset #8 (id:220001) has been deleted

[yoshiki, 2015-02-24 21:30:57]

Asanka, PTAL. Thanks.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/download_commands.cc:140: content::PageNavigator*
navigator) {
On 2015/02/20 23:34:39, asanka wrote:
> indent.
> 
> Perhaps run this through 'git cl format'

Thanks for letting me know the command. Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:112: delegate_.reset(new
DefaultUIControllerDelegate(
On 2015/02/20 23:34:39, asanka wrote:
> Shall we rename the UIControllerDelegates?
> 
> DefaultUIControllerDelegate -> DownloadShelfUIControllerDelegate
> 
> DefaultUIControllerDelegateAndroid -> AndroidUIControllerDelegate

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:107: //
Reshows the notification.
On 2015/02/20 23:34:39, asanka wrote:
> Can you explain what this logic does?

I added some comments. Does it make sense?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:117:
delegate_->OnDownloadRemoved(this);
On 2015/02/20 23:34:40, asanka wrote:
> Is the OnDownloadRemoved call going to destroy |this|?

Yes. It removes |this|.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:146:
DownloadCommands(item_).ExecuteCommand(command, item_->GetWebContents());
On 2015/02/20 23:34:39, asanka wrote:
> GetWebContents() can (and often will) return NULL if the WebContents that
> initiated the download has gone away.

I changed it to pass a |Profile| instead of a |WebContents|.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:164:
DownloadItemModel* model = new DownloadItemModel(item_);
On 2015/02/20 23:34:40, asanka wrote:
> This leaks.
> 
> In general, DownloadItemModel is a thin wrapper, and you don't need to
allocate
> it on the heap. You can use "DownloadItemModel model(item_);"

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:188: //
TODO(yoshiki): Shows an appreciate icon.
On 2015/02/20 23:34:39, asanka wrote:
> appreciate?

I changed the code around here and the comment has been removed.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:191: default:
On 2015/02/20 23:34:39, asanka wrote:
> Don't use a 'default' label when using an enumeration with switch(). Instead
> handle all the values individually.

I changed the code around here and it has been removed.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:273:
delegate_->OnDownloadRemoved(this);
On 2015/02/20 23:34:39, asanka wrote:
> DownloadNOtificationItem being a RefCounted class makes me worried about
> lifetime issues. |item| is likely going to be destroyed soon (and will also
emit
> an OnDownloadDestroyed() notification). If DownloadNOtificationItem outlives
> DownloadItem, then the |item_| member is going to be a dangling pointer. Can
you
> resolve that?

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:291:
actions->push_back(DownloadCommands::SHOW_IN_FOLDER);
On 2015/02/20 23:34:40, asanka wrote:
> YOu don't want to SHOW_IN_FOLDER if it's dangerous.

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.h (right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:58: void
OnDownloadRemoved(content::DownloadItem* item) override;
On 2015/02/20 23:34:40, asanka wrote:
> You also need to listen for OnDownloadDestroyed().
> 
> If you see this notification, then |item| is about to be deleted. 

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:60: void
SetNotificationData();
On 2015/02/20 23:34:40, asanka wrote:
> Reword? It's updating the notification based on the state of the item.

Done. Does the new name look good for you?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:61: void
SetImageToNotification(int resource_id);
On 2015/02/20 23:34:40, asanka wrote:
> Reword?

Done. Does the new name look good for you?

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item_unittest.cc
(right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:142:
// Makes sure the DownloadItem::Close() is not called.
On 2015/02/20 23:34:40, asanka wrote:
> DownloadItem::Cancel(). Here and below.

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:182:
TEST_F(DownloadNotificationItemTest, OpenDownloadTest) {
On 2015/02/20 23:34:40, asanka wrote:
> Nit: Individual tests don't need to be called *Test since that's implicit.

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:210:
// Toggles open-when-complete (new value: true).
On 2015/02/20 23:34:40, asanka wrote:
> Nit: new value is false?

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item_unittest.cc:239:

On 2015/02/20 23:34:40, asanka wrote:
> Extra whitespace

Done.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_manager.cc
(right):

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_manager.cc:28: void
DownloadNotificationManager::UpdateDownloadingMultipleFiles() {
On 2015/02/20 23:34:40, asanka wrote:
> Might be worth leaving this out until it's ready to be implemented, or
implement
> it now.
> 
> Also, can you reword this?

Removed.

https://codereview.chromium.org/852043002/diff/200001/chrome/browser/download...
chrome/browser/download/notification/download_notification_manager.cc:65:
item->AddRef();
On 2015/02/20 23:34:40, asanka wrote:
> Rather than manually refcounting, you can make the items_ vector contain
> scoped_refptr<>s instead.

Done.

[yoshiki, 2015-02-26 18:08:06]

Asanka, ping?

[asanka, 2015-02-27 01:26:49]

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:61: if (!download_item_)
download_item_ should never be nullptr. Here and elsewhere in DownloadCommand.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:83: return
!download_item_->IsDone() &&
Use download_item_->CanResume()

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:189:
g_browser_process->profile_manager()->IsValidProfile(profile)
Something is seriously wrong if the profile isn't valid. Why do you think
DownloadCommands would outlive a Profile?

Also, you can use
Profile::FromBrowserContext(download_item_->GetBrowserContext()) to get at the
profile instead of keeping a Profile*.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:190: ?
chrome::FindBrowserWithProfile(profile,
This can return nullptr if there are no active browser windows. Shouldn't we
create a browser window if none exists?

Also, use FindTabbedBrowser() since we don't want to open the help page in a
non-tabbed browser window.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:198: if (protection_service) {
Nit: no {

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:210:
g_browser_process->profile_manager()->IsValidProfile(profile)
Same comments as above. Probably move the Browser* lookup code into a separate
function?

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:227: case RETRY:
What do you expect the RETRY command to do? And where is it used?

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_commands.h (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.h:32: explicit
DownloadCommands(content::DownloadItem* download_item);
Document that |download_item| must outlive DownloadCommands.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:31: Profile* profile)
As mentioned elsewhere, Profile* is implicit for a DownloadItem.

However, a DownloadShelfContextMenu is bound to a very specific Browser (the one
it is invoked within). We shouldn't break that link.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:33:
download_commands_(download_item),
Make download_commands_ a scoped_ptr<> and reset it when the
OnDownloadDestroyed() notification is received. That way we can make sure that
the DownloadCommands won't outlive the DownloadItem.

Also, check if download_commands_ is non-null before invoking it.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:199:
DownloadCommands command(item_);
Looks like this is unused.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:297: //
|OnDownloadRemoved()|.
why OnDownloadRemoved?

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.h (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:26: : public
base::RefCountedThreadSafe<DownloadNotificationItem>,
Why does this need to be refcounted?

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:99: Profile*
profile_;
Profile is implied.

[yoshiki, 2015-02-28 08:02:05]

Patchset #10 (id:280001) has been deleted

[yoshiki, 2015-02-28 08:23:59]

Patchset #10 (id:300001) has been deleted

[yoshiki, 2015-02-28 08:39:36]

Patchset #10 (id:320001) has been deleted

[yoshiki, 2015-02-28 10:50:54]

Asanka, Thanks you for good comments. PTAL again. Thanks

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:61: if (!download_item_)
On 2015/02/27 01:26:48, asanka wrote:
> download_item_ should never be nullptr. Here and elsewhere in DownloadCommand.

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:83: return
!download_item_->IsDone() &&
On 2015/02/27 01:26:48, asanka wrote:
> Use download_item_->CanResume()

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:189:
g_browser_process->profile_manager()->IsValidProfile(profile)
On 2015/02/27 01:26:48, asanka wrote:
> Something is seriously wrong if the profile isn't valid. Why do you think
> DownloadCommands would outlive a Profile?
> 
> Also, you can use
> Profile::FromBrowserContext(download_item_->GetBrowserContext()) to get at the
> profile instead of keeping a Profile*.
> 

Thanks for guidance. I do so. Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:190: ?
chrome::FindBrowserWithProfile(profile,
On 2015/02/27 01:26:48, asanka wrote:
> This can return nullptr if there are no active browser windows. Shouldn't we
> create a browser window if none exists?
> 
> Also, use FindTabbedBrowser() since we don't want to open the help page in a
> non-tabbed browser window.

ScopedTabbedBrowserDisplayer looks good in this case. I use it.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:198: if (protection_service) {
On 2015/02/27 01:26:48, asanka wrote:
> Nit: no {

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:210:
g_browser_process->profile_manager()->IsValidProfile(profile)
On 2015/02/27 01:26:48, asanka wrote:
> Same comments as above. Probably move the Browser* lookup code into a separate
> function?

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:227: case RETRY:
On 2015/02/27 01:26:49, asanka wrote:
> What do you expect the RETRY command to do? And where is it used?

It's used when user wants to retry a cancelled or interrupted download.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_commands.h (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.h:32: explicit
DownloadCommands(content::DownloadItem* download_item);
On 2015/02/27 01:26:49, asanka wrote:
> Document that |download_item| must outlive DownloadCommands.

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:31: Profile* profile)
On 2015/02/27 01:26:49, asanka wrote:
> As mentioned elsewhere, Profile* is implicit for a DownloadItem.
> 
> However, a DownloadShelfContextMenu is bound to a very specific Browser (the
one
> it is invoked within). We shouldn't break that link.

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_shelf_context_menu.cc:33:
download_commands_(download_item),
On 2015/02/27 01:26:49, asanka wrote:
> Make download_commands_ a scoped_ptr<> and reset it when the
> OnDownloadDestroyed() notification is received. That way we can make sure that
> the DownloadCommands won't outlive the DownloadItem.
> 
> Also, check if download_commands_ is non-null before invoking it.

Done.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:199:
DownloadCommands command(item_);
On 2015/02/27 01:26:49, asanka wrote:
> Looks like this is unused.

It's used in L274.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:297: //
|OnDownloadRemoved()|.
On 2015/02/27 01:26:49, asanka wrote:
> why OnDownloadRemoved?

The comment was wrong. Fixed.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_item.h (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:26: : public
base::RefCountedThreadSafe<DownloadNotificationItem>,
On 2015/02/27 01:26:49, asanka wrote:
> Why does this need to be refcounted?

No strong reasons. I changed back it into non-refcounted.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.h:99: Profile*
profile_;
On 2015/02/27 01:26:49, asanka wrote:
> Profile is implied.

Done.

[asanka, 2015-03-02 22:17:25]

lgtm

I'm still not onboard with the RETRY command, but I think we should consider
landing and iterating at this point.

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:227: case RETRY:
On 2015/02/28 10:50:53, yoshiki wrote:
> On 2015/02/27 01:26:49, asanka wrote:
> > What do you expect the RETRY command to do? And where is it used?
> 
> It's used when user wants to retry a cancelled or interrupted download.

What's the difference between RESUME and RETRY?

https://codereview.chromium.org/852043002/diff/340001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/340001/chrome/browser/download...
chrome/browser/download/download_commands.cc:181: (sb_service ?
sb_service->download_protection_service() : NULL);
Nit: use nullptr for new code.

[asanka, 2015-03-02 22:19:43]

Nit: Wrap CL description at 72 columns.

And thanks for putting up with my comments.

[yoshiki, 2015-03-03 04:48:38]

I really appreciate you pointing out my bad code and giving a lot of detailed
guidance. Thank you!

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/260001/chrome/browser/download...
chrome/browser/download/download_commands.cc:227: case RETRY:
On 2015/03/02 22:17:25, asanka wrote:
> On 2015/02/28 10:50:53, yoshiki wrote:
> > On 2015/02/27 01:26:49, asanka wrote:
> > > What do you expect the RETRY command to do? And where is it used?
> > 
> > It's used when user wants to retry a cancelled or interrupted download.
> 
> What's the difference between RESUME and RETRY?

I meant:
- Resume: suspended download
- Retry: cancelled or interrupted download

But I just realized we can reconsider about the difference and behavior. I'll
chat with the PM about it. Thanks!

https://codereview.chromium.org/852043002/diff/340001/chrome/browser/download...
File chrome/browser/download/download_commands.cc (right):

https://codereview.chromium.org/852043002/diff/340001/chrome/browser/download...
chrome/browser/download/download_commands.cc:181: (sb_service ?
sb_service->download_protection_service() : NULL);
On 2015/03/02 22:17:25, asanka wrote:
> Nit: use nullptr for new code.

Done.

[yoshiki, 2015-03-03 04:53:36]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2015-03-03 04:53:37]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/852043002/#ps360001
(title: "Addressed comments")

[yoshiki, 2015-03-03 04:53:42]

The CQ bit was unchecked by yoshiki@chromium.org

[yoshiki, 2015-03-03 04:53:44]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-03-03 04:54:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/852043002/360001

[commit-bot: I haz the power, 2015-03-03 04:57:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-03 04:57:37]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[yoshiki, 2015-03-03 06:19:53]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2015-03-03 06:19:53]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/852043002/#ps380001
(title: "Rebased")

[commit-bot: I haz the power, 2015-03-03 06:30:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/852043002/380001

[commit-bot: I haz the power, 2015-03-03 06:49:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-03 06:50:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yoshiki, 2015-03-03 11:26:18]

+oshima, sky, isherman: adding the rest owners.

Oshima, PTAL at changes in chrome/app/theme/*. These images are temporary, and
should be replaced after UX guys create new ones by the launch.

Sky, PTAL at changed in chrome/browser/ui/views/download/.

Ilya, PTAL at changes in tools/metrics/histograms/.

[yoshiki, 2015-03-03 11:27:00]

yoshiki@chromium.org changed reviewers:
+ isherman@chromium.org, oshima@chromium.org, sky@chromium.org

[yoshiki, 2015-03-03 11:27:01]

+oshima, sky, isherman: adding the rest owners.

Oshima, PTAL at changes in chrome/app/theme/*. These images are temporary, and
should be replaced after UX guys create new ones by the launch.

Sky, PTAL at changed in chrome/browser/ui/views/download/.

Ilya, PTAL at changes in tools/metrics/histograms/.

[sky, 2015-03-03 16:39:14]

chrome/browser/ui/views/download LGTM

[oshima, 2015-03-03 16:44:53]

c/a/theme lgtm

please make sure to request/add 200x assets as well.

[Ilya Sherman, 2015-03-03 22:56:39]

histograms.xml lgtm

[yoshiki, 2015-03-04 04:21:19]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-03-04 04:21:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/852043002/380001

[commit-bot: I haz the power, 2015-03-04 05:45:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-04 05:45:52]

Try jobs failed on following builders:
  android_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[yoshiki, 2015-03-04 18:39:59]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2015-03-04 18:40:00]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
oshima@chromium.org, isherman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/852043002/#ps400001
(title: "Fixed build & test errors on non-ChromeOS platform.")

[commit-bot: I haz the power, 2015-03-04 18:41:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/852043002/400001

[commit-bot: I haz the power, 2015-03-04 20:08:28]

Committed patchset #13 (id:400001)

[commit-bot: I haz the power, 2015-03-04 20:10:17]

Patchset 13 (id:??) landed as
https://crrev.com/f42d8fc065c006e4590476b9526c8b5bd2563d6d
Cr-Commit-Position: refs/heads/master@{#319113}