Change database scheme - add state and start tracking

Change database scheme - add state and start tracking

To be able to individually pause requests, we need a "PAUSED" state.
It needs to be persisted, so we need to store it in our RequestQueue,
which is the only persistent storage the BackgroundOfflining system has.

While we are changing the schema, we are also adding a new field to
keep track of how often a request has been started.  If a request
casues the pre-reneder to crash, we don't have code to detect the crash,
and we will just retry later.  To avoid getting stuck in an infinite
crashing loop, we only allow requests to start so many times.

Currently, we can't tell the difference between a crash and chromium
getting swapped out of memory, so this will have some false positives,
which is why we chose the limit to be higher than the completed limit.

We split the existing attempt count field into started_attempt_count
and completed_attempt_count.

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

[Pete Williamson, 2016-08-07 19:21:22]

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

[commit-bot: I haz the power, 2016-08-07 19:21:29]

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-07 20:12:44]

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

[commit-bot: I haz the power, 2016-08-07 20:12:45]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-08-07 20:35:24]

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

[commit-bot: I haz the power, 2016-08-07 20:35:42]

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-07 21:12:43]

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

[commit-bot: I haz the power, 2016-08-07 21:12:45]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-08-08 02:14:36]

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

[fgorski, 2016-08-08 17:48:06]

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy.h:59: // The max number of
times we will *start* a request.
Drop the **.

I think you could add something like:
Starting an attempt does not need it has to successfully finished. It could be
cause by the browser being evicted from memory, or in rare cases prerenderer
issues.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:284:
request.completed_attempt_count() + 1 >=
What is going on with the +1. Please explain it.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:285:
policy_->GetMaxCompletedTries()) {
Do you plan to check for GetMaxStartedTries() here?

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:13: const bool
was_user_requested)
nit: rename to user_requested, please.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:72: // We intentinally 
do not increment the  completed_attempt_count_, since this
typo in: intentinally 
extra space after "intentionally" and "the" in this line, and after "to" in the
next.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:73: // was killed
before it had a chance to  run to completion so we could use the
how about: chance to complete. Phone was used for other memory/CPU intensive
task?

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:80:
DCHECK_GT(started_attempt_count_, 0);
Would it not be possible to pause a task before it gets a chance to run? ... I
think notification appears earlier than we actually run the background loading.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:81: state_ =
RequestState::PAUSED;
can we mark last_attempt_time_ = base::Time(), since we are coming out of
pending when paused in cases this happens.

Alternative solution would be to never clear last_attempt_pending_ and use it
for sorting the requests in the picker.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:105: // number of
attempts finished because we could crash.
s/finished/completed/

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:108: // Number of
attempts we actually finished to get the page.
s/finished/completed/

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:35:
ASSERT_EQ(0, request.completed_attempt_count());
check for state and started count here as well, please.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:70:
ASSERT_EQ(base::Time(), request.last_attempt_time()); // TODO: flaky?
what is happening? Could you explain it?

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:106:
ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());
This is unrelated to this line, but affects this file:
Can we have a scenario when we MarkAttemptPaused(), please?
* From AVAILABLE (with no attempts), if that is possible
* From AVAILABLE, which has been attempted in the past
* From PRERENDERING, (make sure to check last_attempt_time_).

[chili, 2016-08-08 18:17:00]

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

[chili, 2016-08-08 18:17:01]

kind of drive-by =)

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.cc:92: // reqeust.
tiny nit - (since this file's touched anyway....) can we fix spelling on
request?

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.cc:118: // eligible to try
again.
nit: this comment is the same as the one above.  Perhaps update to "the tries
completed max number of times"?

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:48: // migrate
it.
do we need a todo to add transaction once this feature is ready to be released?

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:73: // was killed
before it had a chance to  run to completion so we could use the
On 2016/08/08 17:48:05, fgorski wrote:
> how about: chance to complete. Phone was used for other memory/CPU intensive
> task? 

even shorter: "since this was killed before it completed"?

[Pete Williamson, 2016-08-08 18:44:29]

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy.h:59: // The max number of
times we will *start* a request.
On 2016/08/08 17:48:05, fgorski wrote:
> Drop the **.
> 
> I think you could add something like:
> Starting an attempt does not need it has to successfully finished. It could be
> cause by the browser being evicted from memory, or in rare cases prerenderer
> issues.

Done.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:284:
request.completed_attempt_count() + 1 >=
On 2016/08/08 17:48:05, fgorski wrote:
> What is going on with the +1. Please explain it.

comment added.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:285:
policy_->GetMaxCompletedTries()) {
On 2016/08/08 17:48:05, fgorski wrote:
> Do you plan to check for GetMaxStartedTries() here?

Checking GetMaxStartedTries is coming in a future changelist once this one
clears.  Also, this is not the right place to check it, we check it before
starting a request, not after.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.cc:92: // reqeust.
On 2016/08/08 18:17:01, chili wrote:
> tiny nit - (since this file's touched anyway....) can we fix spelling on
> request?

Done.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.cc:118: // eligible to try
again.
On 2016/08/08 18:17:01, chili wrote:
> nit: this comment is the same as the one above.  Perhaps update to "the tries
> completed max number of times"?

comment updated.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:48: // migrate
it.
On 2016/08/08 18:17:01, chili wrote:
> do we need a todo to add transaction once this feature is ready to be
released?

We only need a transaction the next time we modify the schema after this feature
is released, so I chose not to add the TODO, but left a verbose comment.  Let me
know if you feel otherwise, and I'll be happy to add it back in.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:13: const bool
was_user_requested)
On 2016/08/08 17:48:05, fgorski wrote:
> nit: rename to user_requested, please.

Is it OK to do that later?  was_user_requested is in a lot of places.  I'll
start a separate changelist for it.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:72: // We intentinally 
do not increment the  completed_attempt_count_, since this
On 2016/08/08 17:48:05, fgorski wrote:
> typo in: intentinally 
> extra space after "intentionally" and "the" in this line, and after "to" in
the
> next.

Done.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:73: // was killed
before it had a chance to  run to completion so we could use the
On 2016/08/08 17:48:05, fgorski wrote:
> how about: chance to complete. Phone was used for other memory/CPU intensive
> task? 

Done.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:73: // was killed
before it had a chance to  run to completion so we could use the
On 2016/08/08 18:17:01, chili wrote:
> On 2016/08/08 17:48:05, fgorski wrote:
> > how about: chance to complete. Phone was used for other memory/CPU intensive
> > task? 
> 
> even shorter: "since this was killed before it completed"?

Done.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:80:
DCHECK_GT(started_attempt_count_, 0);
On 2016/08/08 17:48:05, fgorski wrote:
> Would it not be possible to pause a task before it gets a chance to run? ... I
> think notification appears earlier than we actually run the background
loading.

Good catch, removed DCHECK

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:81: state_ =
RequestState::PAUSED;
On 2016/08/08 17:48:05, fgorski wrote:
> can we mark last_attempt_time_ = base::Time(), since we are coming out of
> pending when paused in cases this happens.
> 
> Alternative solution would be to never clear last_attempt_pending_ and use it
> for sorting the requests in the picker.

In the normal case the request has not been attempted, so I left
last_attempt_time alone.  If it has been attempted, the last_attempt_time was
set when we attempted it, so we also leave it alone.

Now that we have state, we don't need to rely on the last attempt time being
cleared to show that the request is not in progress.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:105: // number of
attempts finished because we could crash.
On 2016/08/08 17:48:05, fgorski wrote:
> s/finished/completed/

Done.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.h:108: // Number of
attempts we actually finished to get the page.
On 2016/08/08 17:48:05, fgorski wrote:
> s/finished/completed/

Done.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:35:
ASSERT_EQ(0, request.completed_attempt_count());
On 2016/08/08 17:48:05, fgorski wrote:
> check for state and started count here as well, please.

Done.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:70:
ASSERT_EQ(base::Time(), request.last_attempt_time()); // TODO: flaky?
On 2016/08/08 17:48:06, fgorski wrote:
> what is happening? Could you explain it?

Sure - Sorry, I meant to investigate this before uploading the change, and
missed the TODO that I left for myself.

My concern here was that if time passes between when we set last_attempt_time
and when we verify that the time might not be what we expect.  It was related to
not being sure if we were using the real clock here or a test clock.  If we used
a real clock, this would be flaky if any of the test takes more than 0 time to
complete.  I wanted to make sure I investigated and that we were using a test
clock.

Looking at it, I don't see a test clock in use, so I'm not sure why this test
always passes unless it always happens very fast and never loses the time slice.

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:106:
ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());
On 2016/08/08 17:48:06, fgorski wrote:
> This is unrelated to this line, but affects this file:
> Can we have a scenario when we MarkAttemptPaused(), please?
> * From AVAILABLE (with no attempts), if that is possible
> * From AVAILABLE, which has been attempted in the past
> * From PRERENDERING, (make sure to check last_attempt_time_).

Tests for the state changes are coming in next changelist (I've already coded
it, but I'm waiting for this to land before sending it out for review so we
don't have to review code twice)

[Pete Williamson, 2016-08-08 18:53:12]

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:70:
ASSERT_EQ(base::Time(), request.last_attempt_time()); // TODO: flaky?
On 2016/08/08 18:44:29, Pete Williamson wrote:
> On 2016/08/08 17:48:06, fgorski wrote:
> > what is happening? Could you explain it?
> 
> Sure - Sorry, I meant to investigate this before uploading the change, and
> missed the TODO that I left for myself.
> 
> My concern here was that if time passes between when we set last_attempt_time
> and when we verify that the time might not be what we expect.  It was related
to
> not being sure if we were using the real clock here or a test clock.  If we
used
> a real clock, this would be flaky if any of the test takes more than 0 time to
> complete.  I wanted to make sure I investigated and that we were using a test
> clock.
> 
> Looking at it, I don't see a test clock in use, so I'm not sure why this test
> always passes unless it always happens very fast and never loses the time
slice.

Ah, I see it is comparing to base::Time(), not base::Time::Now().  In that case,
the code is safe, comment removed.

[fgorski, 2016-08-08 22:35:34]

lgtm after you sort out which way to go with last_attempt_time_.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:81: state_ =
RequestState::PAUSED;
On 2016/08/08 18:44:28, Pete Williamson wrote:
> On 2016/08/08 17:48:05, fgorski wrote:
> > can we mark last_attempt_time_ = base::Time(), since we are coming out of
> > pending when paused in cases this happens.
> > 
> > Alternative solution would be to never clear last_attempt_pending_ and use
it
> > for sorting the requests in the picker.
> 
> In the normal case the request has not been attempted, so I left
> last_attempt_time alone.  If it has been attempted, the last_attempt_time was
> set when we attempted it, so we also leave it alone.
> 
> Now that we have state, we don't need to rely on the last attempt time being
> cleared to show that the request is not in progress.

Then why clearing it in cases where the attempt was completed or aborted?
In my mind either way is good (clean it or not), as long as code paths are
consistent.

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request_unittest.cc:106:
ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());
On 2016/08/08 18:44:29, Pete Williamson wrote:
> On 2016/08/08 17:48:06, fgorski wrote:
> > This is unrelated to this line, but affects this file:
> > Can we have a scenario when we MarkAttemptPaused(), please?
> > * From AVAILABLE (with no attempts), if that is possible
> > * From AVAILABLE, which has been attempted in the past
> > * From PRERENDERING, (make sure to check last_attempt_time_).
> 
> Tests for the state changes are coming in next changelist (I've already coded
> it, but I'm waiting for this to land before sending it out for review so we
> don't have to review code twice)

Acknowledged.

[Pete Williamson, 2016-08-08 22:56:56]

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

https://codereview.chromium.org/2218403002/diff/20001/components/offline_page...
components/offline_pages/background/save_page_request.cc:81: state_ =
RequestState::PAUSED;
On 2016/08/08 22:35:33, fgorski wrote:
> On 2016/08/08 18:44:28, Pete Williamson wrote:
> > On 2016/08/08 17:48:05, fgorski wrote:
> > > can we mark last_attempt_time_ = base::Time(), since we are coming out of
> > > pending when paused in cases this happens.
> > > 
> > > Alternative solution would be to never clear last_attempt_pending_ and use
> it
> > > for sorting the requests in the picker.
> > 
> > In the normal case the request has not been attempted, so I left
> > last_attempt_time alone.  If it has been attempted, the last_attempt_time
was
> > set when we attempted it, so we also leave it alone.
> > 
> > Now that we have state, we don't need to rely on the last attempt time being
> > cleared to show that the request is not in progress.
> 
> Then why clearing it in cases where the attempt was completed or aborted?
> In my mind either way is good (clean it or not), as long as code paths are
> consistent.

OK, I chose to stop clearing it and rely solely on state.

[Pete Williamson, 2016-08-08 22:57:04]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-08-08 22:57:05]

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/2218403002/#ps80001
(title: "Stop clearing last request time")

[commit-bot: I haz the power, 2016-08-08 22:57:42]

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-09 00:17:37]

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

[commit-bot: I haz the power, 2016-08-09 00:17:38]

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

[Pete Williamson, 2016-08-09 00:18:24]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-08-09 00:19:02]

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-09 02:15:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-09 02:18:03]

Description was changed from

==========
Change database scheme - add state and start tracking

To be able to individually pause requests, we need a "PAUSED" state.
It needs to be persisted, so we need to store it in our RequestQueue,
which is the only persistent storage the BackgroundOfflining system has.

While we are changing the schema, we are also adding a new field to
keep track of how often a request has been started.  If a request
casues the pre-reneder to crash, we don't have code to detect the crash,
and we will just retry later.  To avoid getting stuck in an infinite
crashing loop, we only allow requests to start so many times.

Currently, we can't tell the difference between a crash and chromium
getting swapped out of memory, so this will have some false positives,
which is why we chose the limit to be higher than the completed limit.

We split the existing attempt count field into started_attempt_count
and completed_attempt_count.

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

to

==========
Change database scheme - add state and start tracking

To be able to individually pause requests, we need a "PAUSED" state.
It needs to be persisted, so we need to store it in our RequestQueue,
which is the only persistent storage the BackgroundOfflining system has.

While we are changing the schema, we are also adding a new field to
keep track of how often a request has been started.  If a request
casues the pre-reneder to crash, we don't have code to detect the crash,
and we will just retry later.  To avoid getting stuck in an infinite
crashing loop, we only allow requests to start so many times.

Currently, we can't tell the difference between a crash and chromium
getting swapped out of memory, so this will have some false positives,
which is why we chose the limit to be higher than the completed limit.

We split the existing attempt count field into started_attempt_count
and completed_attempt_count.

BUG=610521

Committed: https://crrev.com/47cd270d10b49ebdaea567566f8044ae5a89ef9e
Cr-Commit-Position: refs/heads/master@{#410521}
==========

[commit-bot: I haz the power, 2016-08-09 02:18:06]

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

[dougarnett, 2016-08-19 20:21:25]

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

[dougarnett, 2016-08-19 20:21:26]

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

https://codereview.chromium.org/2218403002/diff/80001/components/offline_page...
components/offline_pages/background/save_page_request.cc:61: state_ =
RequestState::PRERENDERING;
I don't understand why this internally sets a specifically named PRERENDERING
state (vs. some more generic IN_PROGRESS or LOADING). This method doesn't know
the implementation for loading so it is guessing. I think it should either be a
generally named state or else the specific state would need to be passed in.

[Pete Williamson, 2016-08-22 16:33:29]

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

https://codereview.chromium.org/2218403002/diff/80001/components/offline_page...
components/offline_pages/background/save_page_request.cc:61: state_ =
RequestState::PRERENDERING;
On 2016/08/19 20:21:26, dougarnett wrote:
> I don't understand why this internally sets a specifically named PRERENDERING
> state (vs. some more generic IN_PROGRESS or LOADING). This method doesn't know
> the implementation for loading so it is guessing. I think it should either be
a
> generally named state or else the specific state would need to be passed in. 

The thinking was that we might someday have other states such as SAVING_PAGE, or
other types of pre-renderer.  So, we can pass in the state to the
MarkAttemptStarted method in a future change, or we could wait until we
introduce more states to make the change.