Issue 2870753003: Add a Model to the download component.

David Trainor- moved to gerrit

dtrainor@chromium.org changed reviewers: + shaktisahu@chromium.org, xingliu@chromium.org

3 years, 7 months ago (2017-05-08 22:47:09 UTC) #1

David Trainor- moved to gerrit

ptal! Peter as fyi, feel free to review as well if you have cycles. I'll ...

3 years, 7 months ago (2017-05-08 22:47:10 UTC) #2

xingliu

In general looks good. https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.h File components/download/internal/entry.h (right): https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.h#newcode5 components/download/internal/entry.h:5: #ifndef COMPONENTS_DOWNLOAD_INTERNAL_DOWNLOAD_ENTRY_H_ INTERNAL_ENTRY_H_. https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.h#newcode10 components/download/internal/entry.h:10: ...

3 years, 7 months ago (2017-05-09 06:11:42 UTC) #3

David Trainor- moved to gerrit

https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.h File components/download/internal/entry.h (right): https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.h#newcode40 components/download/internal/entry.h:40: DUMP_STATS = 4, On 2017/05/09 06:11:41, xingliu wrote: > ...

3 years, 7 months ago (2017-05-09 15:59:27 UTC) #4

David Trainor- moved to gerrit

dtrainor@chromium.org changed reviewers: + peter@chromium.org

3 years, 7 months ago (2017-05-09 15:59:57 UTC) #5

Peter Beverloo

lgtm I like how only meta data will be stored, avoiding duplication with the downloads ...

3 years, 7 months ago (2017-05-10 12:44:46 UTC) #7

David Trainor- moved to gerrit

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

3 years, 7 months ago (2017-05-13 00:32:21 UTC) #9

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-13 00:33:20 UTC) #10

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-13 00:36:47 UTC) #11

commit-bot: I haz the power

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/210324) ios-device-xcode-clang on ...

3 years, 7 months ago (2017-05-13 00:36:48 UTC) #12

David Trainor- moved to gerrit

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

3 years, 7 months ago (2017-05-13 00:47:08 UTC) #13

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-13 00:47:55 UTC) #14

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-13 00:52:53 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/268778) android_n5x_swarming_rel on ...

3 years, 7 months ago (2017-05-13 00:52:54 UTC) #16

David Trainor- moved to gerrit

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

3 years, 7 months ago (2017-05-13 01:18:46 UTC) #17

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-13 01:19:07 UTC) #18

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-13 03:07:00 UTC) #19

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/177590)

3 years, 7 months ago (2017-05-13 03:07:01 UTC) #20

David Trainor- moved to gerrit

https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.cc File components/download/internal/entry.cc (right): https://codereview.chromium.org/2870753003/diff/1/components/download/internal/entry.cc#newcode9 components/download/internal/entry.cc:9: Entry::Entry() : client(DownloadClient::INVALID), state(State::NEW) {} On 2017/05/10 12:44:44, Peter ...

3 years, 7 months ago (2017-05-15 15:59:52 UTC) #21

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/entry.cc (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/entry.cc:9: Entry::Entry() :
client(DownloadClient::INVALID), state(State::NEW) {}
On 2017/05/10 12:44:44, Peter Beverloo wrote:
> something to consider: you can initialize scalars in the header file these
days.
> Adds more explicit initial-state documentation to the header, and allows
another
> fancy `= default;` in this file.

Done.  Sometimes I keep it in the cc file so I can initialize everything in one
place.  But so far these are the only things I have to initialize, so that works
:).

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/entry.h (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/entry.h:5: #ifndef
COMPONENTS_DOWNLOAD_INTERNAL_DOWNLOAD_ENTRY_H_
On 2017/05/09 06:11:41, xingliu wrote:
> INTERNAL_ENTRY_H_.

Done.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/entry.h:10: #include "base/time/time.h"
On 2017/05/09 06:11:41, xingliu wrote:
> nit: Maybe we can remove the above 3 headers.

Done.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/entry.h:40: DUMP_STATS = 4,
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> On 2017/05/09 15:59:26, David Trainor-ping if over 24h wrote:
> > On 2017/05/09 06:11:41, xingliu wrote:
> > > optional nit: not sure, but do we really like to make metric logging a
> > separate
> > > state?
> > 
> > Probably not.  I do want all logging for all possible exit points to go
> through
> > a single flow though (too easy to miss added points).
> 
> If you want to validate state transitions, maybe just think of this as
COMPLETE?
> 
> I guess all previous states can transition to this one (either through
> completion, timeout or cancellation). You'd record UMA, which can be
> synchronous, and then transition to WATCHDOG, requiring that for deleting the
> data, even if it's immediate.

Yeah the plan was to pretty much immediately transition out of this state.  I'll
make it COMPLETE because that abstracts away exactly what the state is doing and
represents the state of the download at that point.  Thanks!

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/model.h (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model.h:69: // Will call
Client::OnStoreUpdateFailed() if the write fails.
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> There's cases when a client might want to wait for the update to finish. A
> cleaner transactional design would be to create an OnItemUpdated analogous to
> OnItemAdded, having the same arguments, allowing both success and failure to
be
> acknowledged.
> 
> This also goes for a theoretical OnItemRemoved. You have a much better
> understanding here -- how'd you answer the "what do I do" question when
> OnStoreUpdateFailed, or either of the OnItem{Updated,Removed} methods, gets
> called with `false`?

Yeah I went back and forth on exposing those concepts for a while... there were
some cases where it ended up causing potential pain and complexity if you have
observers pushing new data to the model while we're notifying them.  Then
pushing two updates at once would cause some headaches as well.

I think your idea of only sending the DownloadClient and guid (and not the
old/new Entry state) might simplify things a bit.  And now that we only have one
Client for the Model we avoid a lot of the complexity.  Will add.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model.h:83: virtual EntryList PeekEntries() = 0;
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> Here and for Get() - document that the Entry*(s) should be used immediately
> (i.e. a stronger warning). Since the Entries are owned by the model, any sort
of
> caching could result in UAF.

Done.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/model_impl.cc (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model_impl.cc:35:
entries_.insert(OwnedEntryPair(entry.guid, base::MakeUnique<Entry>(entry)));
On 2017/05/09 06:11:41, xingliu wrote:
> nit: maybe consider std::map::emplace.

Done.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model_impl.cc:43: // that is currently in the
process of being added?
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> While the Client definitely should wait for the acknowledgements, in practice
> this does little harm. The ProtoDB will run on a sequenced task runner so
> there's a guarantee that the update will run after the add.

Yeah I can probably remove this now that it's simpler.  Thanks!

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model_impl.cc:100: // this map?
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> I'd argue so. Current behaviour is:
> 
> auto model = base::MakeUnique<ModelImpl>(..);
> model->Add(entry) --> false
> model->Get(entry.guid) --> it was added??

I think the hard part is:
model->Add(entry) --> async write starts
model->Get(entry.guid) --> Has the value before the async returns
model->OnAddFinished(false) --> We remove it

I guess as long as I'm 100% clear on the semantics of the operation it should be
ok.  Either that or notifying OnEntryRemoved() on the client to clear it up.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/model_impl.h (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model_impl.h:10: #include <vector>
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> IWYU: #include <utility>

Ah ty!

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/model_impl_unittest.cc (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model_impl_unittest.cc:76: return true;
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> I think you could *significantly* simplify this (and the OIC copy) by using
> std::is_permuation()?
> 
> http://en.cppreference.com/w/cpp/algorithm/is_permutation

O_o!  Yeah way better :).

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/model_impl_unittest.cc:95: : task_runner_(new
base::TestMockTimeTaskRunner),
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> qq: would TestSimpleTaskRunner suffice? why is time significant?

Good call.  Done.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/noop_store.h (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/noop_store.h:17: // requirements.
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> Once an actual ProtoDB store is in place, will this class be removed in favor
of
> store_impl? Might be worth adding a TODO for that.

I was going to leave it for tests.  Will add a TODO to track what should happen
to it though.

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
File components/download/internal/store.h (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/interna...
components/download/internal/store.h:26: using StoreCallback =
base::Callback<void(bool success)>;
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> nit: these can be base::OnceCallback, allowing you to drop const& in the usage
> below and use move semantics.

Done.

https://codereview.chromium.org/2870753003/diff/1/components/download/public/...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2870753003/diff/1/components/download/public/...
components/download/public/clients.h:26: TEST_3 = 3,
On 2017/05/10 12:44:45, Peter Beverloo wrote:
> On 2017/05/09 15:59:26, David Trainor-ping if over 24h wrote:
> > On 2017/05/09 06:11:41, xingliu wrote:
> > > optional nit: this is more like something in tests files. But I don't have
> > > better solution. 
> > 
> > Me neither... I don't want to rely on a client always being there to test
this
> > out.  I could probably just drop it to one test client though...
> 
> We'll have to reflect these in histograms.xml, so if we could reduce this to
two
> (or one!) clients that'd be grand.

Good point r.e. UMA.  I'll leave it at 1.  If we ever need more than 1 we can
revisit.

David Trainor- moved to gerrit

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

3 years, 7 months ago (2017-05-15 16:00:00 UTC) #22

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 16:00:25 UTC) #23

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 17:46:36 UTC) #24

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/178266)

3 years, 7 months ago (2017-05-15 17:46:37 UTC) #25

David Trainor- moved to gerrit

The CQ bit was checked by dtrainor@chromium.org

3 years, 7 months ago (2017-05-15 18:10:34 UTC) #26

David Trainor- moved to gerrit

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, xingliu@chromium.org Link to ...

3 years, 7 months ago (2017-05-15 18:10:35 UTC) #27

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 18:11:34 UTC) #28

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494871834834270, "parent_rev": "27d4a2f28593194c2642514f9f28457d14fab406", "commit_rev": "61cce8bddecd5de53945f16fc2093340eb60375a"}

3 years, 7 months ago (2017-05-15 19:34:32 UTC) #29

commit-bot: I haz the power

Description was changed from ========== Add a Model to the download component. Add a basic ...

3 years, 7 months ago (2017-05-15 19:34:48 UTC) #30

commit-bot: I haz the power

3 years, 7 months ago (2017-05-15 19:34:49 UTC) #31

Message was sent while issue was closed.

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/61cce8bddecd5de53945f16fc209...

Issue 2870753003: Add a Model to the download component. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed comments and restructured tests because of OnceCallback #

Patch Set 3 : Fixed small issues #

Patch Set 4 : Rebased #

Patch Set 5 : Fix analyze error #

Patch Set 6 : Renamed test base class #

Messages