Adds an observer for the request coordinator.

Adds an observer for the request coordinator.

The request coordinator observer follows the standard chrome
observer_list pattern, and sends out notifications to registered
observers whenever the state of a request changes.  Supported
notifications are request succeeded, request failed, request became
available, or request was paused.

BUG=610521
Committed: https://crrev.com/feb1dd8c3f56254d000fa0c46a892c930a06054e
Cr-Commit-Position: refs/heads/master@{#412088}

[Dmitry Titov, 2016-08-09 20:14:58]

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

[Dmitry Titov, 2016-08-09 20:14:59]

Some initial thoughts:

https://codereview.chromium.org/2219393004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:46: virtual void
OnAvailable(RequestInfo info) = 0;
Since this is an internal, most generic interface to the collection of
SavePageRequests, it should be simply defined in terms of items it serves,
SavePageRequests. As in:

Gets the whole request because it'll almost inevitably trigger a query for that
request right away:

OnAvailable(const SavePageRequest& request);


Also, once Succeeded or Failed, there is additional data to pass, like
newly-created offline page or failure code.

OnSucceeded(int64_t request_id, int64_t offline_id);

I assume this one includes all other reasons the request may disappear,
including things like expiration or removal from internal page?

OnFailed(int64_t request_id, SomeFailureEnum failure);

There should be onUnpause probably...

OnPaused(int64_t request_id);

Another variant (probably better) would be to have 
OnAdded (this is fired first time we add request)
OnChanged (this is fired every time it's tried, paused, unpaused etc - when any
field of it changes)
OnSucceeded (fired when created offlien page and was deleted)
OnFailed (fired when did not result in offline page and was deleted)

This basically clamps all states 'during processing' into OnChanged, which is
probably better because as we invent more internal states ('paused because of
lower priority' etc) we don't need to expand the observer interface.

Open for discussion: we may pass int64_t request_id or a reference to the whole
request. I think many observers will want to know more about request right away,
so I can see why we may pass a reference.

https://codereview.chromium.org/2219393004/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:115: // observer
before the observer is deleted.
This comment should also mention what happens when the observer is added wrt the
existing collection. For example, lets say I want to have UI that shows all
requests in their current state. If I GetQueuedRequests(), I get a list
asynchronously. When should I start observing to keep my UI (which is
essentially a local cache of request information) up-to-date reliably? If I
start listening too soon (before querying the requests), I may get updates for
items I don't yet have or that do not exist anymore, but if I start listening
too late (after obtaining the result from the query), I may miss some updates.

[Pete Williamson, 2016-08-10 01:36:23]

Romax, here is a preview of what the observer will look like.

https://codereview.chromium.org/2219393004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:46: virtual void
OnAvailable(RequestInfo info) = 0;
On 2016/08/09 20:14:59, Dmitry Titov wrote:
> Since this is an internal, most generic interface to the collection of
> SavePageRequests, it should be simply defined in terms of items it serves,
> SavePageRequests. As in:
> 
> Gets the whole request because it'll almost inevitably trigger a query for
that
> request right away:
> 
> OnAvailable(const SavePageRequest& request);
> 
> 
> Also, once Succeeded or Failed, there is additional data to pass, like
> newly-created offline page or failure code.
> 
> OnSucceeded(int64_t request_id, int64_t offline_id);
> 
> I assume this one includes all other reasons the request may disappear,
> including things like expiration or removal from internal page?
> 
> OnFailed(int64_t request_id, SomeFailureEnum failure);
> 
> There should be onUnpause probably...
> 
> OnPaused(int64_t request_id);
> 
> Another variant (probably better) would be to have 
> OnAdded (this is fired first time we add request)
> OnChanged (this is fired every time it's tried, paused, unpaused etc - when
any
> field of it changes)
> OnSucceeded (fired when created offlien page and was deleted)
> OnFailed (fired when did not result in offline page and was deleted)
> 
> This basically clamps all states 'during processing' into OnChanged, which is
> probably better because as we invent more internal states ('paused because of
> lower priority' etc) we don't need to expand the observer interface.
> 
> Open for discussion: we may pass int64_t request_id or a reference to the
whole
> request. I think many observers will want to know more about request right
away,
> so I can see why we may pass a reference.

OK, changed to 
OnAdded
OnSucceeded(item, offline_id)
OnFailed(request, failure_code)
OnChanged(request) -> This one gets called for Pause and Unpause

https://codereview.chromium.org/2219393004/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:115: // observer
before the observer is deleted.
On 2016/08/09 20:14:59, Dmitry Titov wrote:
> This comment should also mention what happens when the observer is added wrt
the
> existing collection. For example, lets say I want to have UI that shows all
> requests in their current state. If I GetQueuedRequests(), I get a list
> asynchronously. When should I start observing to keep my UI (which is
> essentially a local cache of request information) up-to-date reliably? If I
> start listening too soon (before querying the requests), I may get updates for
> items I don't yet have or that do not exist anymore, but if I start listening
> too late (after obtaining the result from the query), I may miss some updates.

Done.

[fgorski, 2016-08-10 21:30:42]

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

[fgorski, 2016-08-10 21:30:45]

Initial list.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:134:
SendNotifyChangeForRequestIdList(request_ids);
I think that should be happening in the callback.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:144:
SendNotifyChangeForRequestIdList(request_ids);
same

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:189: for (int64_t
request_id : request_ids) {
given the structure of this loop, wouldn't std::unordered_set be better?

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:344:
NotifyFailed(updated_request, static_cast<int64_t>(status));
If we are returning offliner status, let's not pass it as an int64_t.

I was thinking a general cause would be good as an alternative:
EXPIRATION, RETRY_COUNT, ...

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:42: const
SavePageRequest& request, int64_t offline_id) = 0;
We should discuss making offline_id == request_id.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:47: virtual void
OnChanged(const SavePageRequest& request) = 0;
Did we consider OnUpdated?

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:112: // pointer is
unowned, and it is the callers responsibility to remove the
nit: "not owned" sounds better here.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:115: //
GetQueuedRequests yet, so before GetQueuedRequests is called, the listener
I think I disagree with Dmitry about having this part of the comment. Let's
discuss it today.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:164: void
NotifyChangedListCallback(const std::vector<int64_t> request_ids,
is copying intended here?

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:138: void
clear() {
I believe this should be Clear().

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:384:
EXPECT_EQ(true, observer().added_called());
EXPECT_TRUE and applies everywhere you do EXPECT_EQ(true

[Pete Williamson, 2016-08-11 00:08:37]

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:134:
SendNotifyChangeForRequestIdList(request_ids);
On 2016/08/10 21:30:44, fgorski wrote:
> I think that should be happening in the callback.

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:144:
SendNotifyChangeForRequestIdList(request_ids);
On 2016/08/10 21:30:44, fgorski wrote:
> same

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:189: for (int64_t
request_id : request_ids) {
On 2016/08/10 21:30:44, fgorski wrote:
> given the structure of this loop, wouldn't std::unordered_set be better?

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:344:
NotifyFailed(updated_request, static_cast<int64_t>(status));
On 2016/08/10 21:30:44, fgorski wrote:
> If we are returning offliner status, let's not pass it as an int64_t.
> 
> I was thinking a general cause would be good as an alternative:
> EXPIRATION, RETRY_COUNT, ...
> 

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:42: const
SavePageRequest& request, int64_t offline_id) = 0;
On 2016/08/10 21:30:44, fgorski wrote:
> We should discuss making offline_id == request_id.

TODO added (in the SavePageLater function)

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:47: virtual void
OnChanged(const SavePageRequest& request) = 0;
On 2016/08/10 21:30:44, fgorski wrote:
> Did we consider OnUpdated?

We discussed this in person, and nobody had a strong preference.  Code Search
shows more instances of OnChanged than OnUpdated, so I think it is more common,
sticking with OnChanged for now.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:112: // pointer is
unowned, and it is the callers responsibility to remove the
On 2016/08/10 21:30:44, fgorski wrote:
> nit: "not owned" sounds better here.

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:115: //
GetQueuedRequests yet, so before GetQueuedRequests is called, the listener
On 2016/08/10 21:30:44, fgorski wrote:
> I think I disagree with Dmitry about having this part of the comment. Let's
> discuss it today.

Comment updated per our discussion, and moved to the observer instead of the
AddObserver method.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:164: void
NotifyChangedListCallback(const std::vector<int64_t> request_ids,
On 2016/08/10 21:30:44, fgorski wrote:
> is copying intended here?

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:138: void
clear() {
On 2016/08/10 21:30:45, fgorski wrote:
> I believe this should be Clear().

Done.

https://codereview.chromium.org/2219393004/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:384:
EXPECT_EQ(true, observer().added_called());
On 2016/08/10 21:30:44, fgorski wrote:
> EXPECT_TRUE and applies everywhere you do EXPECT_EQ(true

Done everywhere.

[Pete Williamson, 2016-08-11 00:44:47]

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

[commit-bot: I haz the power, 2016-08-11 00:45:34]

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, 2016-08-11 02:29:16]

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

[commit-bot: I haz the power, 2016-08-11 02:29:17]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2016-08-11 03:41:56]

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:169: void
RequestCoordinator::UpdateMultipleRequestCallback(
UpdateMultipleRequestCallback and SendNotifyChangeForRequestIdList can be merged
into a single method.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:172: // If the
request succeeded, nothing to do.  If it failed, we can't really do
This comment seems needs updating, because "nothing to do" is not valid anymore.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:443:
queue_->GetRequests(base::Bind(&RequestCoordinator::NotifyChangedListCallback,
It is unfortunate that we need to go do the SQL query against a database just to
send notifications. In fact, there can be some requests there that are already
deleted from the database by the time we are fetching them. For example, in case
of RemoveRequests - are they first removed fmor DB, and then we try to fetch
them again to notify? Will it even work?

It would be better to keep the list of requests that were actually affected
passed through chain of callbacks and then notify based on that.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:55: int64_t
offline_id) = 0;
Once we agreed the id is inherited from request to the page, we don't seem to
need offline_id here.
I would remove it now since you will start inheriting id very soon and it'll be
nice to avoid new code written against this API to only have to change later.

[fgorski, 2016-08-11 17:03:59]

lgtm, after you resolve Dmitry's comment about reading before notifying.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:50: // after the
return callback from GetAlLRequests.
AlL -> All

nit: I believe we use |GetAllRequests| to distinguish methods, if nothing
changed.

[Pete Williamson, 2016-08-13 03:20:34]

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:169: void
RequestCoordinator::UpdateMultipleRequestCallback(
On 2016/08/11 03:41:55, Dmitry Titov wrote:
> UpdateMultipleRequestCallback and SendNotifyChangeForRequestIdList can be
merged
> into a single method.

Done.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:172: // If the
request succeeded, nothing to do.  If it failed, we can't really do
On 2016/08/11 03:41:55, Dmitry Titov wrote:
> This comment seems needs updating, because "nothing to do" is not valid
anymore.

Done.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:443:
queue_->GetRequests(base::Bind(&RequestCoordinator::NotifyChangedListCallback,
On 2016/08/11 03:41:55, Dmitry Titov wrote:
> It is unfortunate that we need to go do the SQL query against a database just
to
> send notifications. In fact, there can be some requests there that are already
> deleted from the database by the time we are fetching them. For example, in
case
> of RemoveRequests - are they first removed fmor DB, and then we try to fetch
> them again to notify? Will it even work?
> 
> It would be better to keep the list of requests that were actually affected
> passed through chain of callbacks and then notify based on that.

Done.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:50: // after the
return callback from GetAlLRequests.
On 2016/08/11 17:03:58, fgorski wrote:
> AlL -> All
> 
> nit: I believe we use |GetAllRequests| to distinguish methods, if nothing
> changed.

Done.

https://codereview.chromium.org/2219393004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:55: int64_t
offline_id) = 0;
On 2016/08/11 03:41:55, Dmitry Titov wrote:
> Once we agreed the id is inherited from request to the page, we don't seem to
> need offline_id here.
> I would remove it now since you will start inheriting id very soon and it'll
be
> nice to avoid new code written against this API to only have to change later.

Done.

[Pete Williamson, 2016-08-13 03:20:46]

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

[commit-bot: I haz the power, 2016-08-13 03:20:55]

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, 2016-08-13 04:28:26]

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

[commit-bot: I haz the power, 2016-08-13 04:28:27]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-08-15 17:51:44]

just a few nits to look at

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:159: for
(SavePageRequest request : requests) {
nit: remove {}

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:167: for
(SavePageRequest request : requests) {
nit: remove {}

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:753:
TEST_F(RequestCoordinatorTest, RemoveObserver) {
nit: this is unfortunate name.
ObservedRemoveRequest maybe?

[Pete Williamson, 2016-08-15 20:22:52]

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:159: for
(SavePageRequest request : requests) {
On 2016/08/15 17:51:44, fgorski wrote:
> nit: remove {}

Done.

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:167: for
(SavePageRequest request : requests) {
On 2016/08/15 17:51:44, fgorski wrote:
> nit: remove {}

Done.

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2219393004/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:753:
TEST_F(RequestCoordinatorTest, RemoveObserver) {
On 2016/08/15 17:51:44, fgorski wrote:
> nit: this is unfortunate name.
> ObservedRemoveRequest maybe?

Done.

[Dmitry Titov, 2016-08-15 21:36:57]

lgtm

[Pete Williamson, 2016-08-15 21:39:33]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-08-15 21:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2219393004/#ps80001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-08-15 21:39:50]

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, 2016-08-15 23:22:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-15 23:25:45]

Description was changed from

==========
Adds an observer for the request coordinator.

The request coordinator observer follows the standard chrome
observer_list pattern, and sends out notifications to registered
observers whenever the state of a request changes.  Supported
notifications are request succeeded, request failed, request became
available, or request was paused.

BUG=610521
==========

to

==========
Adds an observer for the request coordinator.

The request coordinator observer follows the standard chrome
observer_list pattern, and sends out notifications to registered
observers whenever the state of a request changes.  Supported
notifications are request succeeded, request failed, request became
available, or request was paused.

BUG=610521

Committed: https://crrev.com/feb1dd8c3f56254d000fa0c46a892c930a06054e
Cr-Commit-Position: refs/heads/master@{#412088}
==========

[commit-bot: I haz the power, 2016-08-15 23:25:46]

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