[Offline pages] Empty RequestQueue and SavePageRequest

[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request. SavePageRequest comes with basic implementation and tests.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org

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

[fgorski, 2016-04-28 22:43:53]

PTAL. This is still missing the build files.

[Pete Williamson, 2016-04-28 23:16:08]

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:28: // it has reached a
quota of active requests.
Why do we need two quotas? Would one suffice?  I suspect that most requests will
be active (not too many will be "Don't load this until 5AM", for instance, so we
should count that as active, and just have one quota for active requests).

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:40: // Callback used for
|GetRequests|.
Why make these async?  I'm not averse to async per se, I just want to know what
we get for the extra complexity.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:53: // schedule purging of
the request queue.
Why not have all the methods schedule a queue purge?

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:9:
SavePageRequest::SavePageRequest() {}
We don't need a Ctor and Dtor if we make this a struct, so maybe we can drop
this file.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:17: class
SavePageRequest {
Would this be better as a struct?  All the elements are public, and we don't
have accessor functions.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:23: // gets a chance to.
SHould we also have an IN-PROGRESS state for requests that are active in the
pre-renderer?

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:24: FAILED,   // Page
request failed.
I think we need this case.  Eventually when zine hooks up, if it asks for a
page, it may need to ask us what happened to it, so we can't take it out of the
queue just because it failed.

We may want to beef up the comments to say that this is not just a request that
failed once, but failed enough times that we are giving up on it so we can avoid
the potential ambiguity.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:32: int64_t request_id;
Just curious, where does this request_id come from?  Assigned by the
coordinator?  Assigned by the API?  Assigned by the end client (Async or Zine?)

[fgorski, 2016-04-29 05:19:16]

PTAL.
Comments addressed, build files added, compilations issues resolved.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:28: // it has reached a
quota of active requests.
On 2016/04/28 23:16:07, Pete Williamson wrote:
> Why do we need two quotas? Would one suffice?  I suspect that most requests
will
> be active (not too many will be "Don't load this until 5AM", for instance, so
we
> should count that as active, and just have one quota for active requests).

Done.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:40: // Callback used for
|GetRequests|.
On 2016/04/28 23:16:07, Pete Williamson wrote:
> Why make these async?  I'm not averse to async per se, I just want to know
what
> we get for the extra complexity.

The capability to store and read it from a file.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:53: // schedule purging of
the request queue.
On 2016/04/28 23:16:07, Pete Williamson wrote:
> Why not have all the methods schedule a queue purge?

Because other methods don't know if there is anything to clean up, while this
one has a pretty good view off the queue. That way if it notices that there are
some tasks that are long expired/failed, it can simply schedule purging.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:9:
SavePageRequest::SavePageRequest() {}
On 2016/04/28 23:16:07, Pete Williamson wrote:
> We don't need a Ctor and Dtor if we make this a struct, so maybe we can drop
> this file.

I actually will want to construct it even when it is a struct, to set reasonable
values.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:17: class
SavePageRequest {
On 2016/04/28 23:16:07, Pete Williamson wrote:
> Would this be better as a struct?  All the elements are public, and we don't
> have accessor functions.

Done.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:23: // gets a chance to.
On 2016/04/28 23:16:07, Pete Williamson wrote:
> SHould we also have an IN-PROGRESS state for requests that are active in the
> pre-renderer?

Done.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:24: FAILED,   // Page
request failed.
On 2016/04/28 23:16:07, Pete Williamson wrote:
> I think we need this case.  Eventually when zine hooks up, if it asks for a
> page, it may need to ask us what happened to it, so we can't take it out of
the
> queue just because it failed.
> 
> We may want to beef up the comments to say that this is not just a request
that
> failed once, but failed enough times that we are giving up on it so we can
avoid
> the potential ambiguity.

Done.

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:32: int64_t request_id;
On 2016/04/28 23:16:08, Pete Williamson wrote:
> Just curious, where does this request_id come from?  Assigned by the
> coordinator?  Assigned by the API?  Assigned by the end client (Async or
Zine?)

We can either assign it at the coordinator level or at the queue level. Do you
have a preference?

[fgorski, 2016-04-29 05:19:24]

Description was changed from

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request.
It does not include any implementation.

TODO: add files to build files, once they are updated.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request.
It does not include any implementation.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

[dougarnett, 2016-04-29 15:39:14]

Just a bit confused by Status definitions

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.cc:6: 
I saw Pete do this too. Is there some style pref to have empty lines between
includes (even from same directory)? I've been more accustomed to them being
packed together.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:21: // time in future.
time => opportune time   ?  // yeah I know, another fancy word

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
File components/offline_pages/background/request_queue.h (right):

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/request_queue.h:56: // In case adding the
request violates policy, the result will fail with
I see the QuotaHit result defined above - could you doc a bit more about hitting
quota? Eg, will this method try to drop old entries per policy or just return
QuotaHit and when caller sees QuotaHit, should caller try Purge or do Get and
Removes?

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/request_queue.h:63: // exists. Does nothing
otherwise. Result is returned through |callback|.
Is this a simple overwrite?

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
File components/offline_pages/background/save_page_request.h (right):

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/save_page_request.h:20: WAITING,      //
Component requested a page be saved, but not until a
I'm not sure about this definition of WAITING. From design doc, I imagined
WAITING are ones that needs processing and ACTIVE are ones the Coordinator
started processing. 

Also I'm not sure we should define a Status for being out of an applicable time
window - might better be handled by query checking time constraints against
current time than update queue entry status field as they go in and out of time
window.

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/save_page_request.h:22: ACTIVE,       //
Page request is active, and coordinator can attempt it when
ACTIVE name is misleading name to me for this definition. Maybe PENDING is
better?

[Pete Williamson, 2016-04-29 16:19:03]

lgtm, once DougArnett's comments are addressed.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.cc:6: 
On 2016/04/29 15:39:14, dougarnett wrote:
> I saw Pete do this too. Is there some style pref to have empty lines between
> includes (even from same directory)? I've been more accustomed to them being
> packed together.

The style guide says that the include file that goes with this .cc file should
be first.

Other than that, it says to put blank lines between different sections, and what
order the sections should be in.  It says to use alphabetical order within a
section.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

[fgorski, 2016-04-30 18:56:14]

Updated. PTAL (Pete, there have been significant changes, you might want to take
another look).

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.cc:6: 
On 2016/04/29 16:19:03, Pete Williamson wrote:
> On 2016/04/29 15:39:14, dougarnett wrote:
> > I saw Pete do this too. Is there some style pref to have empty lines between
> > includes (even from same directory)? I've been more accustomed to them being
> > packed together.
> 
> The style guide says that the include file that goes with this .cc file should
> be first.
> 
> Other than that, it says to put blank lines between different sections, and
what
> order the sections should be in.  It says to use alphabetical order within a
> section.
> 
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

As Pete said.

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

https://codereview.chromium.org/1932953002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:21: // time in future.
On 2016/04/29 15:39:14, dougarnett wrote:
> time => opportune time   ?  // yeah I know, another fancy word

updated appropriately.

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
File components/offline_pages/background/request_queue.h (right):

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/request_queue.h:56: // In case adding the
request violates policy, the result will fail with
On 2016/04/29 15:39:14, dougarnett wrote:
> I see the QuotaHit result defined above - could you doc a bit more about
hitting
> quota? Eg, will this method try to drop old entries per policy or just return
> QuotaHit and when caller sees QuotaHit, should caller try Purge or do Get and
> Removes?

I think we should document that only once we have a policy. Also policy might be
a better place to put that information.

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/request_queue.h:63: // exists. Does nothing
otherwise. Result is returned through |callback|.
On 2016/04/29 15:39:14, dougarnett wrote:
> Is this a simple overwrite?

Yes.

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
File components/offline_pages/background/save_page_request.h (right):

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/save_page_request.h:20: WAITING,      //
Component requested a page be saved, but not until a
On 2016/04/29 15:39:14, dougarnett wrote:
> I'm not sure about this definition of WAITING. From design doc, I imagined
> WAITING are ones that needs processing and ACTIVE are ones the Coordinator
> started processing. 
> 
> Also I'm not sure we should define a Status for being out of an applicable
time
> window - might better be handled by query checking time constraints against
> current time than update queue entry status field as they go in and out of
time
> window.

not ready it is

https://codereview.chromium.org/1932953002/diff/60001/components/offline_page...
components/offline_pages/background/save_page_request.h:22: ACTIVE,       //
Page request is active, and coordinator can attempt it when
On 2016/04/29 15:39:14, dougarnett wrote:
> ACTIVE name is misleading name to me for this definition. Maybe PENDING is
> better? 

Done.

[dougarnett, 2016-05-02 15:54:20]

lgtm

https://codereview.chromium.org/1932953002/diff/100001/components/offline_pag...
File components/offline_pages/background/save_page_request.h (right):

https://codereview.chromium.org/1932953002/diff/100001/components/offline_pag...
components/offline_pages/background/save_page_request.h:43: void
MarkAttempStarted(const base::Time& start_time);
Nice

[fgorski, 2016-05-02 21:12:08]

Pete, PTAL now. Tests added. (I don't think this needs rebasing, but if it fails
to compile I'll try that as well.)

[fgorski, 2016-05-02 21:30:56]

Description was changed from

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request.
It does not include any implementation.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request. SavePageRequest
comes with basic implementation and tests.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

[Pete Williamson, 2016-05-02 21:45:33]

lgtm with nits

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
components/offline_pages/background/request_coordinator.cc:29: void
RequestCoordinator::StopProcessing() {}
Nit - you can revert moving the close curly brace.  (it will cause me a small
merge conflict, and is not important to this patch)

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
File components/offline_pages/background/save_page_request.h (right):

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
components/offline_pages/background/save_page_request.h:47: // Marks attempt as
completed and clears |last_attempt_time_|.
You are not clearing it, the comment needs updating.

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
File components/offline_pages/background/save_page_request_unittest.cc (right):

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
components/offline_pages/background/save_page_request_unittest.cc:78: // Attempt
time, attempt count and status will thou.
nit - thou -> though

[fgorski, 2016-05-02 22:11:01]

Addressed and re-testing.

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
components/offline_pages/background/request_coordinator.cc:29: void
RequestCoordinator::StopProcessing() {}
On 2016/05/02 21:45:33, Pete Williamson wrote:
> Nit - you can revert moving the close curly brace.  (it will cause me a small
> merge conflict, and is not important to this patch)

Done.

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
File components/offline_pages/background/save_page_request.h (right):

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
components/offline_pages/background/save_page_request.h:47: // Marks attempt as
completed and clears |last_attempt_time_|.
On 2016/05/02 21:45:33, Pete Williamson wrote:
> You are not clearing it, the comment needs updating.

Per our discussion I am (setting base::Time() which is epoch time).

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
File components/offline_pages/background/save_page_request_unittest.cc (right):

https://codereview.chromium.org/1932953002/diff/120001/components/offline_pag...
components/offline_pages/background/save_page_request_unittest.cc:78: // Attempt
time, attempt count and status will thou.
On 2016/05/02 21:45:33, Pete Williamson wrote:
> nit - thou -> though

Done.

[fgorski, 2016-05-02 22:44:36]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-02 22:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/1932953002/#ps140001
(title: "Addressing nits from Pete")

[commit-bot: I haz the power, 2016-05-02 22:46:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932953002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932953002/140001

[commit-bot: I haz the power, 2016-05-02 22:56:11]

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

[commit-bot: I haz the power, 2016-05-02 22:56:12]

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

[fgorski, 2016-05-02 23:23:35]

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

[fgorski, 2016-05-02 23:23:36]

jam@ please review the gn/gyp changes.

Thanks.
Filip

[jam, 2016-05-02 23:58:25]

lgtm

[fgorski, 2016-05-03 04:34:25]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-05-03 04:34:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932953002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932953002/140001

[commit-bot: I haz the power, 2016-05-03 04:41:46]

Description was changed from

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request. SavePageRequest
comes with basic implementation and tests.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request. SavePageRequest
comes with basic implementation and tests.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

[commit-bot: I haz the power, 2016-05-03 04:41:47]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-03 04:43:04]

Description was changed from

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request. SavePageRequest
comes with basic implementation and tests.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Empty RequestQueue and SavePageRequest

This is initial patch for request queue and save page request. SavePageRequest
comes with basic implementation and tests.

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org

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

[commit-bot: I haz the power, 2016-05-03 04:43:05]

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