Initial checkin of OfflineContentProvider.

Initial checkin of OfflineContentProvider.

This patch adds the initial OfflineContentProvider interface and
aggregator.  This will be the framework that exposes different types of
downloads to the front end UI.  The aggregator groups multiple OfflineContentProviders and
exposes them as a single list of OfflineItems.

BUG=691805
Review-Url: https://codereview.chromium.org/2690333002
Cr-Commit-Position: refs/heads/master@{#455011}
Committed: https://chromium.googlesource.com/chromium/src/+/b4cc24562bca56bd56c7e461e39324eef85a19aa

[David Trainor- moved to gerrit, 2017-02-14 01:21:14]

dtrainor@chromium.org changed reviewers:
+ dimich@chromium.org, fgorski@chromium.org, qinmin@chromium.org

[David Trainor- moved to gerrit, 2017-02-14 01:21:15]

ptal.

qinmin@: All of it (sorry!).
dimich/fgorski: Specifically the public interfaces but happy to have you take a
look at all of it as well.

Starting this while we iron out some of the comments on the doc.  I can tweak
any of this as we go, but I want to get started early.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/OWNERS (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/OWNERS:1: dtrainor@chromium.org
Happy to add more people if appropriate.  Let me know.

[Dmitry Titov, 2017-02-14 05:41:31]

Looks pretty cool, thanks for getting it so fast to a good shape!

A few comments:

https://codereview.chromium.org/2690333002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:1443: "//components/offline_content",
Naming-wise, "offline_content" gives me a pause. This is an aggregator for UI
purposes, while the name may suggest analogy with WebContents or some layer that
actually deals with content itself.

Maybe offline_items_collection or download_home?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_content_provider.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_provider.h:84: // on this
observer.
Maybe makes sense to say that in case of already initialized provider the
OnItemsAvailable should be scheduled via a task rather that directly called.
Less trouble this way :)

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:26: std::string guid;
This can be any id scoped to the name space provider specified... Can we just
call it 'id' w/o expectation that it is a guid-like-formatted string?

Also, 2 different namespaces may have overlapping ids, right?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:69: std::string icon_url;
What about offline scenarios, is this supposed to be a data: url or some other
one?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:73: OfflineItemFilter filter;
This feels related to "file extension" field below. And also like a mime-type.
Maybe only one value (not sure which type, the mime type would be better
perhaps) is needed on the Item.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:82: OfflineItemFileExtension
file_type;
Is this for the same reason as mime-type would be used? Many mime types have
multiple extensions, would be good to either have mime-type if possible or
illustrate how this is going to be used so providers may supply correct values.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:102: std::string referrer_url;
Might not exist... Does UI need to know?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:105: bool is_user_gesture;
What is this used for? This may be not an easy bit to set (sometimes ambiguous,
as in async loading of a page), so the more explicit purpose may help providers
set it.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:120: int64_t received_size_bytes;
Comment could also say what happens when item becomes fully received. Same about
next two.

[qinmin, 2017-02-14 07:37:20]

https://codereview.chromium.org/2690333002/diff/20001/components/offline_cont...
File
components/offline_content/core/test_support/mock_offline_content_provider.cc
(right):

https://codereview.chromium.org/2690333002/diff/20001/components/offline_cont...
components/offline_content/core/test_support/mock_offline_content_provider.cc:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/2690333002/diff/20001/components/offline_cont...
File
components/offline_content/core/test_support/scoped_mock_offline_content_provider.h
(right):

https://codereview.chromium.org/2690333002/diff/20001/components/offline_cont...
components/offline_content/core/test_support/scoped_mock_offline_content_provider.h:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_content_aggregator.cc (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:189: }
can we just early return here?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:82: OfflineItemFileExtension
file_type;
why extension, not mime type?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item_file_extension.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item_file_extension.h:13:
FILE_EXTENSION_OTHER = 0,
are these extensions for FILTER_DOCUMENT? Why not using mime type?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item_file_extension.h:14:
FILE_EXTENSION_APK = 1,
you can remove all the numbers here and below

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item_filter.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item_filter.h:14: FILTER_PAGE = 1,
remove all numbers here and below

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File
components/offline_content/core/test_support/mock_offline_content_provider.cc
(right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/test_support/mock_offline_content_provider.cc:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File
components/offline_content/core/test_support/mock_offline_content_provider.h
(right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/test_support/mock_offline_content_provider.h:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
remove (c)

[fgorski, 2017-02-14 22:17:55]

Great patch. A few comments to consider added.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/OWNERS (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/OWNERS:1: dtrainor@chromium.org
On 2017/02/14 01:21:15, David Trainor wrote:
> Happy to add more people if appropriate.  Let me know.

feel free to add me.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_content_aggregator.cc (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:47:
DCHECK(pending_actions_.find(name_space) == pending_actions_.end());
did you consider adding: DCHECK(provider); ?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:75:
&OfflineContentProvider::OpenItem, base::Unretained(it->second), id));
I feel a little uncomfortable about this one.
Is there a way to do it through a weak_ptr?

I understand the obvious piece (we unregister and remove pending actions).
But what if the provider gets destroy as we process the pending actions, or
something like that?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:76:
FlushPendingActionsIfReady(it->second);
Did you consider alternative approach:
* run if provider ready
* add to pending actions otherwise
which could be wrapped in RunIfReady

It makes little sense to try and flush from almost every action, as chances that
nothing changed the state in between (otherwise the pending activities queue
would be flushed already).

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:126: OfflineItem*
OfflineContentAggregator::GetItemById(const ContentId& id) {
should this be a const OfflineItem*?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:178:
FlushPendingActionsIfReady(provider);
Why did you choose to flush first and then inform observers?
Probably worth documenting with a comment.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_content_provider.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_provider.h:76: // of the
function call.
should the caller be able to modify it?
once again, perhaps a const OfflineItem*?

[David Trainor- moved to gerrit, 2017-02-15 00:23:07]

thanks everyone for the comments!  sorry i wasn't able to reply to them yet
today.  will try to do so tonight or tomorrow.

[David Trainor- moved to gerrit, 2017-02-22 01:23:13]

took longer than I thought but finally had some cycles to get back to this. 
Will upload another patch moving these to the other directory but I wanted to
make sure that the diff tracked properly first.

https://codereview.chromium.org/2690333002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:1443: "//components/offline_content",
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> Naming-wise, "offline_content" gives me a pause. This is an aggregator for UI
> purposes, while the name may suggest analogy with WebContents or some layer
that
> actually deals with content itself.
> 
> Maybe offline_items_collection or download_home?

offline_items_collection works.  I'm hesitant to name it the same thing as the
actual UI element.

I'll move it after I address the other comments so I don't mess up the diffs.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/OWNERS (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/OWNERS:1: dtrainor@chromium.org
On 2017/02/14 22:17:55, fgorski wrote:
> On 2017/02/14 01:21:15, David Trainor wrote:
> > Happy to add more people if appropriate.  Let me know.
> 
> feel free to add me.

Done.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_content_aggregator.cc (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:47:
DCHECK(pending_actions_.find(name_space) == pending_actions_.end());
On 2017/02/14 22:17:55, fgorski wrote:
> did you consider adding: DCHECK(provider); ?

Ah good call done!

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:75:
&OfflineContentProvider::OpenItem, base::Unretained(it->second), id));
On 2017/02/14 22:17:55, fgorski wrote:
> I feel a little uncomfortable about this one.
> Is there a way to do it through a weak_ptr?
> 
> I understand the obvious piece (we unregister and remove pending actions).
> But what if the provider gets destroy as we process the pending actions, or
> something like that?

Ended up checking if the entry was removed from the pending_actions_ map in the
loop.  Felt cleaner than adding a list of WeakPtrFactories, and I was hoping not
to bleed the need for a weak pointer into the interface; If we need them I'd
rather this class maintain the factories since it fully understands the
lifecycle of when the provider is registered.

Let me know if you think this is ok.  If not I can add the list of
WeakPtrFactories.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:76:
FlushPendingActionsIfReady(it->second);
On 2017/02/14 22:17:55, fgorski wrote:
> Did you consider alternative approach:
> * run if provider ready
> * add to pending actions otherwise
> which could be wrapped in RunIfReady
> 
> It makes little sense to try and flush from almost every action, as chances
that
> nothing changed the state in between (otherwise the pending activities queue
> would be flushed already).

Yeah that makes sense.  Will change.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:126: OfflineItem*
OfflineContentAggregator::GetItemById(const ContentId& id) {
On 2017/02/14 22:17:55, fgorski wrote:
> should this be a const OfflineItem*?

Done.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:178:
FlushPendingActionsIfReady(provider);
On 2017/02/14 22:17:55, fgorski wrote:
> Why did you choose to flush first and then inform observers?
> Probably worth documenting with a comment.

Will document in the header.  Let me know if you want me to add something here
as well.  I wanted the data set to reflect the actions take on the data before
consumers tried to query it.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.cc:189: }
On 2017/02/14 07:37:20, qinmin wrote:
> can we just early return here? 

No I don't think so :(.  We still need to notify the other observers that this
provider is initialized.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_content_provider.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_provider.h:76: // of the
function call.
On 2017/02/14 22:17:55, fgorski wrote:
> should the caller be able to modify it?
> once again, perhaps a const OfflineItem*?

Done.  I think this will change as this thing gets built out.  The GetAllItems
call might move to an std::vector<> of pointers to prevent running the copy
constructor on everything.  This is just fleshing out the main interfaces and
kicking things off.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_content_provider.h:84: // on this
observer.
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> Maybe makes sense to say that in case of already initialized provider the
> OnItemsAvailable should be scheduled via a task rather that directly called.
> Less trouble this way :)

Agreed.  I like that you did that in the DownloadUIAdapter and I did the same
thing in the aggregator.  Will comment!

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:26: std::string guid;
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> This can be any id scoped to the name space provider specified... Can we just
> call it 'id' w/o expectation that it is a guid-like-formatted string?
Good idea!  I think that's ok.  Updating it.
> 
> Also, 2 different namespaces may have overlapping ids, right?

I'd say no for now only because some of the UI depends on the guid being unique,
but I can relax the restriction for now until it looks like we need it (and then
maybe I can concat the namespace and the id or something).

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:44: struct OfflineItem {
Quick note on all of these fields:  I pulled most of these from the existing
DownloadHistoryItemWrapper.  I fully expect this list to flux a bit as the UI is
migrated over to this code.  To that end, you all have good questions and I
think removing most of these fields that are in question for now makes the most
sense.  As I need to add them back for the UI we can have targeted discussions
as necessary.  Sorry for the churn!

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:69: std::string icon_url;
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> What about offline scenarios, is this supposed to be a data: url or some other
> one?

Will remove in the short term.  Good point.  It might make sense to expose a
RequestIconForItem(Callback, const ContentId& id) to each
OfflineContentProvider.  I don't want us to have to shoot a bunch of data to
Java and we should only pull icons as the items become (or nearly become)
visible.  Will remove now with a TODO to investigate icons.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:73: OfflineItemFilter filter;
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> This feels related to "file extension" field below. And also like a mime-type.
> Maybe only one value (not sure which type, the mime type would be better
> perhaps) is needed on the Item.

Can we detect offline pages via the mime type?  Currently it looks like for
typical downloads we just use the mime type to generate the filter (video,
audio, image, document, other).  But pages manually sets the entry to page.

We could agree on a custom mime type for offline pages I guess?  wdyt?

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:82: OfflineItemFileExtension
file_type;
On 2017/02/14 07:37:20, qinmin wrote:
> why extension, not mime type?

Yeah you guys have a good point.  See my comment on
offline_item_file_extension.h and at the top of this struct for why I added
this.  Can remove for now :).

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:102: std::string referrer_url;
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> Might not exist... Does UI need to know? 

We need to know to add files to the Android download manager.  I can remove it
for now though.  I would prefer that code went through another path and not
through the UI provider anyway :).

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:105: bool is_user_gesture;
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> What is this used for? This may be not an easy bit to set (sometimes
ambiguous,
> as in async loading of a page), so the more explicit purpose may help
providers
> set it.

Used to determine whether or not to automatically open the download when it's
done downloading.  I think this will eventually be moved to another flow as
well.  Removing.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item.h:120: int64_t received_size_bytes;
On 2017/02/14 05:41:31, Dmitry Titov wrote:
> Comment could also say what happens when item becomes fully received. Same
about
> next two.

Marked them as unused if |state| is COMPLETE.  Let me know if that's a good
approach or if you'd rather I specify specific values for these in that
situation.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item_file_extension.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item_file_extension.h:13:
FILE_EXTENSION_OTHER = 0,
On 2017/02/14 07:37:20, qinmin wrote:
> are these extensions for FILTER_DOCUMENT? Why not using mime type?

I pulled this (practically all) of the fields from the current Java UI.  This is
used to store a UMA histogram on the Java side.  I can remove this for now and
add it back if/when we need it.  I do think we want to keep the histograms but
we may be able to move it to C++ instead (if we migrate the share option back
through the provider).

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item_file_extension.h:14:
FILE_EXTENSION_APK = 1,
On 2017/02/14 07:37:20, qinmin wrote:
> you can remove all the numbers here and below

Done.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File components/offline_content/core/offline_item_filter.h (right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/offline_item_filter.h:14: FILTER_PAGE = 1,
On 2017/02/14 07:37:20, qinmin wrote:
> remove all numbers here and below

Done.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File
components/offline_content/core/test_support/mock_offline_content_provider.cc
(right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/test_support/mock_offline_content_provider.cc:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
On 2017/02/14 07:37:20, qinmin wrote:
> remove (c)

Done.

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
File
components/offline_content/core/test_support/mock_offline_content_provider.h
(right):

https://codereview.chromium.org/2690333002/diff/60001/components/offline_cont...
components/offline_content/core/test_support/mock_offline_content_provider.h:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
On 2017/02/14 07:37:20, qinmin wrote:
> remove (c)

Done.

https://codereview.chromium.org/2690333002/diff/80001/components/offline_cont...
File components/offline_content/core/offline_content_aggregator.h (right):

https://codereview.chromium.org/2690333002/diff/80001/components/offline_cont...
components/offline_content/core/offline_content_aggregator.h:101: // TODO(me):
Add a WeakPtrFactory to this map?
TODO(me): Remove comment!

[qinmin, 2017-02-22 07:39:17]

lgtm % comments

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:45:
DCHECK(provider);
nit: not needed, if provider is null, we will crash anyway

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:56:
DCHECK(it != providers_.end());
ditto

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:68: auto
it = providers_.find(id.name_space);
lots of redundant code here, maybe a separate function:

OfflineContentProvider* FindOfflineContentProvider(const std::string& namespace)
{
}


and change all the code here and below to
if (FindOfflineContentProvider(id.name_space)) 
  .....;

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:248: if
(provider->AreItemsAvailable()) {
nit: {} not needed

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_item.h (right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:19: public:
nit: not needed

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:44: public:
ditto

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:49: ~OfflineItem();
nit: ContentId doesn't declare a dtor, but this one has. Need to make the class
declarations consistent.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:108: int64_t
received_size_bytes;
nit: received_bytes seems a more proper name

[fgorski, 2017-02-23 22:18:44]

lgtm mod comments.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:177: for
(auto& observer : signaled_observers_) {
nit: you can drop the {}

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:241: if
(pending_actions_.find(provider) == pending_actions_.end())
did you mean to check in providers_ and not in pending_actions_?

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.h
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.h:56: //
routed to |provider|.  |provider| is expected to only expose OfflineItems
will we be able to register the same provider multiple times (we have 2
namespaces for offline pages visible in downloads home: async_loading and
downloads)?

[Dmitry Titov, 2017-02-24 05:12:09]

lgtm, nice patch!

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/OWNERS (right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/OWNERS:2: fgorski@chromium.org
Could you add me as well?

[David Trainor- moved to gerrit, 2017-03-03 00:34:12]

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/OWNERS (right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/OWNERS:2: fgorski@chromium.org
On 2017/02/24 05:12:09, Dmitry Titov wrote:
> Could you add me as well?

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:45:
DCHECK(provider);
On 2017/02/22 07:39:16, qinmin wrote:
> nit: not needed, if provider is null, we will crash anyway

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:56:
DCHECK(it != providers_.end());
On 2017/02/22 07:39:16, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:68: auto
it = providers_.find(id.name_space);
On 2017/02/22 07:39:16, qinmin wrote:
> lots of redundant code here, maybe a separate function:
> 
> OfflineContentProvider* FindOfflineContentProvider(const std::string&
namespace)
> {
> }
> 
> 
> and change all the code here and below to
> if (FindOfflineContentProvider(id.name_space)) 
>   .....;

Yeah I thought about it, but it ended up looking the same :(

Provider* p = FindOfflineContentProvider(id.name_space));
if (p == nullptr)
  return;

RunIfReady(...)

Ends up being the same number of lines :(.  If I could lazily bind the provider
to the callback, I could do something like:

RunIfReady(const std::string& name_space, base::Closure closure);

then in that method I could check if the provider existed, and bind the closure
there.  I'll take a look and see if I can clean it up though :).

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:177: for
(auto& observer : signaled_observers_) {
On 2017/02/23 22:18:43, fgorski wrote:
> nit: you can drop the {}

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:241: if
(pending_actions_.find(provider) == pending_actions_.end())
On 2017/02/23 22:18:43, fgorski wrote:
> did you mean to check in providers_ and not in pending_actions_?

providers_ is keyed off of the namespace not the OfflineContentProvider. 
pending_actions_ will get removed if the provider is unregistered as well.

This behavior is a little interesting though:
pending_actions_[provider] allocates an empty std::vector, so the call before
the loop will add the vector to the list if it isn't there.  If we get a remove
call we remove the key and value, so checking this here is ok.  If we somehow
called this method with a provider that wasn't registered, it wouldn't be ideal.
 I can add a DCHECK for that though.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:248: if
(provider->AreItemsAvailable()) {
On 2017/02/22 07:39:16, qinmin wrote:
> nit: {} not needed

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.h
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.h:56: //
routed to |provider|.  |provider| is expected to only expose OfflineItems
On 2017/02/23 22:18:44, fgorski wrote:
> will we be able to register the same provider multiple times (we have 2
> namespaces for offline pages visible in downloads home: async_loading and
> downloads)?

No not as it currently stands :(.  You'd have to register two providers,
otherwise items will get duplicated.  If we end up needing to handle that here
we can add support for it.  An alternate approach would be to write a thin
wrapper layer around your provider that filters the items based on an internal
namespace.  Then you could wrap your provider twice and hook in two separate
providers here.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_item.h (right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:19: public:
On 2017/02/22 07:39:17, qinmin wrote:
> nit: not needed

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:44: public:
On 2017/02/22 07:39:17, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:49: ~OfflineItem();
On 2017/02/22 07:39:17, qinmin wrote:
> nit: ContentId doesn't declare a dtor, but this one has. Need to make the
class
> declarations consistent.

Done.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_item.h:108: int64_t
received_size_bytes;
On 2017/02/22 07:39:17, qinmin wrote:
> nit: received_bytes seems a more proper name

Done.

[David Trainor- moved to gerrit, 2017-03-03 00:36:52]

dtrainor@chromium.org changed reviewers:
+ brettw@chromium.org, dcheng@chromium.org, dpranke@chromium.org

[David Trainor- moved to gerrit, 2017-03-03 00:36:53]

ptal for DEPS owners review:

decheng@: included base/ in DEPS
dpranke@: included testing/ in DEPS
brettw@: included url/ in DEPS

thanks!

[Dirk Pranke, 2017-03-03 00:56:17]

lgtm

[David Trainor- moved to gerrit, 2017-03-03 01:40:16]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 01:41:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 01:50:19]

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

[commit-bot: I haz the power, 2017-03-03 01:50:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dcheng, 2017-03-03 08:59:46]

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:20: bool
VectorContains(const std::vector<T>& items, T item) {
ContainsValue from base/stl_util.h should work here.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:168:
RemoveFromVector(signaled_observers_, observer);
Why not just use a set?

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:225: if
(!VectorContains(signaled_observers_, &observer)) {
Ditto: why not just use a set? A vector is probably more efficient, but it's
easy to break the "set" invariant on it here as well.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:242:
CallbackList actions = std::move(pending_actions_[provider]);
Nit: include <utility> for std::move

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.h
(right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.h:104: //
TODO(me): Add a WeakPtrFactory to this map?
Nit: username or crbug link please

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/core/offline_item.cc (right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:11:
ContentId::ContentId(const ContentId& other)
Or just = default this too.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:24: if (name_space ==
content_id.name_space)
return std::tie(name_space, id) < std::tie(content_id.name_space,
content_id.id);

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:40:
OfflineItem::OfflineItem(const OfflineItem& other)
Ditto: = default will save a lot of typing here.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File
components/offline_items_collection/core/test_support/scoped_mock_offline_content_provider.h
(right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/test_support/scoped_mock_offline_content_provider.h:19:
ScopedMockObserver(OfflineContentProvider* provider);
Nit: explicit

[David Trainor- moved to gerrit, 2017-03-03 09:42:01]

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:20: bool
VectorContains(const std::vector<T>& items, T item) {
On 2017/03/03 08:59:45, dcheng wrote:
> ContainsValue from base/stl_util.h should work here.

Ah nice! :)

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:168:
RemoveFromVector(signaled_observers_, observer);
On 2017/03/03 08:59:45, dcheng wrote:
> Why not just use a set?

Done.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:225: if
(!VectorContains(signaled_observers_, &observer)) {
On 2017/03/03 08:59:45, dcheng wrote:
> Ditto: why not just use a set? A vector is probably more efficient, but it's
> easy to break the "set" invariant on it here as well.

Done.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:242:
CallbackList actions = std::move(pending_actions_[provider]);
On 2017/03/03 08:59:45, dcheng wrote:
> Nit: include <utility> for std::move

Done.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.h
(right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.h:104: //
TODO(me): Add a WeakPtrFactory to this map?
On 2017/03/03 08:59:45, dcheng wrote:
> Nit: username or crbug link please

Ah nice catch.  The comment is no longer valid.  Removed.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/core/offline_item.cc (right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:11:
ContentId::ContentId(const ContentId& other)
On 2017/03/03 08:59:45, dcheng wrote:
> Or just = default this too.

Done.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:24: if (name_space ==
content_id.name_space)
On 2017/03/03 08:59:45, dcheng wrote:
> return std::tie(name_space, id) < std::tie(content_id.name_space,
> content_id.id);

Done.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:40:
OfflineItem::OfflineItem(const OfflineItem& other)
On 2017/03/03 08:59:45, dcheng wrote:
> Ditto: = default will save a lot of typing here.

Done.

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File
components/offline_items_collection/core/test_support/scoped_mock_offline_content_provider.h
(right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/core/test_support/scoped_mock_offline_content_provider.h:19:
ScopedMockObserver(OfflineContentProvider* provider);
On 2017/03/03 08:59:45, dcheng wrote:
> Nit: explicit

Done.

[dcheng, 2017-03-03 09:43:42]

lgtm

[fgorski, 2017-03-03 17:57:47]

Just circling back. Things still look great.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.cc
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.cc:241: if
(pending_actions_.find(provider) == pending_actions_.end())
On 2017/03/03 00:34:12, David Trainor wrote:
> On 2017/02/23 22:18:43, fgorski wrote:
> > did you mean to check in providers_ and not in pending_actions_?
> 
> providers_ is keyed off of the namespace not the OfflineContentProvider. 
> pending_actions_ will get removed if the provider is unregistered as well.
> 
> This behavior is a little interesting though:
> pending_actions_[provider] allocates an empty std::vector, so the call before
> the loop will add the vector to the list if it isn't there.  If we get a
remove
> call we remove the key and value, so checking this here is ok.  If we somehow
> called this method with a provider that wasn't registered, it wouldn't be
ideal.
>  I can add a DCHECK for that though.

Acknowledged.

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
File components/offline_items_collection/core/offline_content_aggregator.h
(right):

https://codereview.chromium.org/2690333002/diff/100001/components/offline_ite...
components/offline_items_collection/core/offline_content_aggregator.h:56: //
routed to |provider|.  |provider| is expected to only expose OfflineItems
On 2017/03/03 00:34:12, David Trainor wrote:
> On 2017/02/23 22:18:44, fgorski wrote:
> > will we be able to register the same provider multiple times (we have 2
> > namespaces for offline pages visible in downloads home: async_loading and
> > downloads)?
> 
> No not as it currently stands :(.  You'd have to register two providers,
> otherwise items will get duplicated.  If we end up needing to handle that here
> we can add support for it.  An alternate approach would be to write a thin
> wrapper layer around your provider that filters the items based on an internal
> namespace.  Then you could wrap your provider twice and hook in two separate
> providers here.

OK.  This is something to consider in the integration moving forward. 
Alternative I see would be to translate all ClientIds on our side to uniform
(namespace-wise) content Ids on OCP side. I'll ping Dmitry about that.

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
File components/offline_items_collection/core/offline_item.cc (right):

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:23: return
std::tie(name_space, id) <
wow!

[David Trainor- moved to gerrit, 2017-03-04 06:43:04]

brettw@ friendly ping for adding a dependency on url/.  thanks!

[brettw, 2017-03-06 18:35:50]

lgtm

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
File components/offline_items_collection/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/BUILD.gn:5:
static_library("offline_items_collection") {
Some systems will complain about static libraries with no sources (this is
causing some of the mac failures in the previous try runs). This should probably
be a group.

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
File components/offline_items_collection/core/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/core/BUILD.gn:5: source_set("core") {
I'd make this a static library. It will build slightly faster in some
configurations with things being static libraries so use those unless
contraindicated (linked into a component or test code).

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/core/BUILD.gn:25: source_set("unit_tests") {
Keep this as-is.

[brettw, 2017-03-06 18:36:42]

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
File components/offline_items_collection/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
components/offline_items_collection/BUILD.gn:5:
static_library("offline_items_collection") {
I would consider just deleting this file, I don't think it's doing much for you.

[David Trainor- moved to gerrit, 2017-03-06 19:03:12]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[David Trainor- moved to gerrit, 2017-03-06 19:03:23]

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
File components/offline_items_collection/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/BUILD.gn:5:
static_library("offline_items_collection") {
On 2017/03/06 18:35:49, brettw (plz ping after 24h) wrote:
> Some systems will complain about static libraries with no sources (this is
> causing some of the mac failures in the previous try runs). This should
probably
> be a group.

Done.

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
File components/offline_items_collection/core/BUILD.gn (right):

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/core/BUILD.gn:5: source_set("core") {
On 2017/03/06 18:35:50, brettw (plz ping after 24h) wrote:
> I'd make this a static library. It will build slightly faster in some
> configurations with things being static libraries so use those unless
> contraindicated (linked into a component or test code).

Done.

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/core/BUILD.gn:25: source_set("unit_tests") {
On 2017/03/06 18:35:50, brettw (plz ping after 24h) wrote:
> Keep this as-is.

Acknowledged.

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
File components/offline_items_collection/core/offline_item.cc (right):

https://codereview.chromium.org/2690333002/diff/140001/components/offline_ite...
components/offline_items_collection/core/offline_item.cc:23: return
std::tie(name_space, id) <
On 2017/03/03 17:57:47, fgorski wrote:
> wow!

+1!

[commit-bot: I haz the power, 2017-03-06 19:03:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 20:09:46]

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

[commit-bot: I haz the power, 2017-03-06 20:09:47]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-06 20:29:26]

On 2017/03/06 18:36:42, brettw (plz ping after 24h) wrote:
>
https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
> File components/offline_items_collection/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2690333002/diff/120001/components/offline_ite...
> components/offline_items_collection/BUILD.gn:5:
> static_library("offline_items_collection") {
> I would consider just deleting this file, I don't think it's doing much for
you.

Yeah I had it there for cleanliness for build dependencies to not have to worry
about the internal structure, but I have a few patches queued up after this and
none really update this build file to make it more useful.  I'll remove it for
now.  Thanks!

[David Trainor- moved to gerrit, 2017-03-06 20:32:03]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 20:32:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 23:15:45]

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

[commit-bot: I haz the power, 2017-03-06 23:15:46]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-07 00:58:43]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-03-07 00:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
dimich@chromium.org, fgorski@chromium.org, dpranke@chromium.org,
dcheng@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2690333002/#ps180001
(title: "Removed forwarding build file and targets for now")

[commit-bot: I haz the power, 2017-03-07 00:59:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 01:07:21]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488848323466610,
"parent_rev": "f4013e330a877ad9fed8f9b39404551cff181217", "commit_rev":
"b4cc24562bca56bd56c7e461e39324eef85a19aa"}

[commit-bot: I haz the power, 2017-03-07 01:08:24]

Description was changed from

==========
Initial checkin of OfflineContentProvider.

This patch adds the initial OfflineContentProvider interface and
aggregator.  This will be the framework that exposes different types of
downloads to the front end UI.  The aggregator groups multiple
OfflineContentProviders and
exposes them as a single list of OfflineItems.

BUG=691805
==========

to

==========
Initial checkin of OfflineContentProvider.

This patch adds the initial OfflineContentProvider interface and
aggregator.  This will be the framework that exposes different types of
downloads to the front end UI.  The aggregator groups multiple
OfflineContentProviders and
exposes them as a single list of OfflineItems.

BUG=691805

Review-Url: https://codereview.chromium.org/2690333002
Cr-Commit-Position: refs/heads/master@{#455011}
Committed:
https://chromium.googlesource.com/chromium/src/+/b4cc24562bca56bd56c7e461e393...
==========

[commit-bot: I haz the power, 2017-03-07 01:08:26]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/b4cc24562bca56bd56c7e461e393...