[Offline Pages] Add Android version check for timeout in offliner policy.

[Offline Pages] Add Android version check for timeout in offliner policy.

Since the doze mode only exists after Android 6.0, adding a check in order
to change to a longer timeout when the system has no doze mode.

BUG=701035
Review-Url: https://codereview.chromium.org/2755753005
Cr-Commit-Position: refs/heads/master@{#459274}
Committed: https://chromium.googlesource.com/chromium/src/+/15a0b4f782f55474f51b073f583a974b5f2bf63f

[romax, 2017-03-17 02:58:34]

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

[romax, 2017-03-17 02:58:34]

PTAL, thanks!

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/offliner_policy.h (right):

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
components/offline_pages/core/background/offliner_policy.h:68: has_doze_mode_ =
false;
not sure if it's worth to have a member variable here, or i can change it to
check it everytime a getter is called.

[Pete Williamson, 2017-03-17 21:05:21]

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

[Pete Williamson, 2017-03-17 21:05:55]

Mostly looks good, one suggestion.


Also, I started a CQ dry run for you to make sure we've run all the right unit
tests.

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/offliner_policy.h (right):

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
components/offline_pages/core/background/offliner_policy.h:68: has_doze_mode_ =
false;
On 2017/03/17 02:58:34, romax wrote:
> not sure if it's worth to have a member variable here, or i can change it to
> check it everytime a getter is called.

I think I'd prefer checking this the first time that a getter is called instead
of in the constructor.  I don't mind if you use a member variable to remember
the state, but I'm concerned that we shouldn't do things that might possibly
fail in the constructor, since the constructor can't really return an error.  
Is there any chance the call to get version numbers could fail? 

Also, did you run unit tests?  I'm not sure that all the unit test frameworks
use a sane number for the android version.

[commit-bot: I haz the power, 2017-03-17 21:06: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-17 21:27:11]

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

[commit-bot: I haz the power, 2017-03-17 21:27:12]

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

[romax, 2017-03-23 02:16:17]

Patchset #2 (id:20001) has been deleted

[romax, 2017-03-23 18:58:50]

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

[commit-bot: I haz the power, 2017-03-23 18:59:24]

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-23 19:09:31]

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

[commit-bot: I haz the power, 2017-03-23 19:09:31]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[romax, 2017-03-23 19:11:48]

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

[commit-bot: I haz the power, 2017-03-23 19:12:19]

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-23 19:44:37]

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

[commit-bot: I haz the power, 2017-03-23 19:44:38]

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

[romax, 2017-03-23 20:53:50]

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

[commit-bot: I haz the power, 2017-03-23 20:54:26]

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

[romax, 2017-03-23 21:56:37]

replied to comments. PTAnL, thanks!

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/offliner_policy.h (right):

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
components/offline_pages/core/background/offliner_policy.h:68: has_doze_mode_ =
false;
On 2017/03/17 21:05:55, Pete Williamson wrote:
> On 2017/03/17 02:58:34, romax wrote:
> > not sure if it's worth to have a member variable here, or i can change it to
> > check it everytime a getter is called.
> 
> I think I'd prefer checking this the first time that a getter is called
instead
> of in the constructor.  I don't mind if you use a member variable to remember
> the state, but I'm concerned that we shouldn't do things that might possibly
> fail in the constructor, since the constructor can't really return an error.  
> Is there any chance the call to get version numbers could fail? 
> 
> Also, did you run unit tests?  I'm not sure that all the unit test frameworks
> use a sane number for the android version.

Generally i wouldn't think this would fail. Even if it fails we'll just use the
longer timeout in the current case. And putting it in constructor seems okay
since it's trying to initialize a member variable?
Also instead of putting it in constructor, it requires another boolean to track
if it's the first time that a getter is called, or the has_doze_mode_ needs to
be a tri-state variable (like an enum?)...
Please let me know if it's still not considered safe to do so or you're strongly
against it, i'll make the change again :)
I've also removed a piece of code for setting the default processing time added
for test harness but end up not being used.

[commit-bot: I haz the power, 2017-03-23 22:17:56]

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

[commit-bot: I haz the power, 2017-03-23 22:17:57]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-03-23 22:52:53]

lgtm

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/offliner_policy.h (right):

https://codereview.chromium.org/2755753005/diff/1/components/offline_pages/co...
components/offline_pages/core/background/offliner_policy.h:68: has_doze_mode_ =
false;
On 2017/03/23 21:56:37, romax wrote:
> On 2017/03/17 21:05:55, Pete Williamson wrote:
> > On 2017/03/17 02:58:34, romax wrote:
> > > not sure if it's worth to have a member variable here, or i can change it
to
> > > check it everytime a getter is called.
> > 
> > I think I'd prefer checking this the first time that a getter is called
> instead
> > of in the constructor.  I don't mind if you use a member variable to
remember
> > the state, but I'm concerned that we shouldn't do things that might possibly
> > fail in the constructor, since the constructor can't really return an error.
 
> > Is there any chance the call to get version numbers could fail? 
> > 
> > Also, did you run unit tests?  I'm not sure that all the unit test
frameworks
> > use a sane number for the android version.
> 
> Generally i wouldn't think this would fail. Even if it fails we'll just use
the
> longer timeout in the current case. And putting it in constructor seems okay
> since it's trying to initialize a member variable?
> Also instead of putting it in constructor, it requires another boolean to
track
> if it's the first time that a getter is called, or the has_doze_mode_ needs to
> be a tri-state variable (like an enum?)...
> Please let me know if it's still not considered safe to do so or you're
strongly
> against it, i'll make the change again :)
> I've also removed a piece of code for setting the default processing time
added
> for test harness but end up not being used.

OK, I took a look at the function, it doesn't seem like it can fail on android
(if it does, it just guesses we are on the default version of android).  So,
this should be OK in a constructor.

[romax, 2017-03-23 23:15:37]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2017-03-23 23:16:27]

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-23 23:23:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490310937430720,
"parent_rev": "6438aa5580fc47cabbdefaa7def934dce2624877", "commit_rev":
"15a0b4f782f55474f51b073f583a974b5f2bf63f"}

[commit-bot: I haz the power, 2017-03-23 23:23:59]

Description was changed from

==========
[Offline Pages] Add Android version check for timeout in offliner policy.

Since the doze mode only exists after Android 6.0, adding a check in order
to change to a longer timeout when the system has no doze mode.

BUG=701035
==========

to

==========
[Offline Pages] Add Android version check for timeout in offliner policy.

Since the doze mode only exists after Android 6.0, adding a check in order
to change to a longer timeout when the system has no doze mode.

BUG=701035

Review-Url: https://codereview.chromium.org/2755753005
Cr-Commit-Position: refs/heads/master@{#459274}
Committed:
https://chromium.googlesource.com/chromium/src/+/15a0b4f782f55474f51b073f583a...
==========

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/15a0b4f782f55474f51b073f583a...

[fgorski, 2017-03-26 05:37:12]

On 2017/03/23 23:24:01, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/15a0b4f782f55474f51b073f583a...

This thing:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[romax, 2017-03-27 17:33:45]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2776203003/ by romax@chromium.org.

The reason for reverting is: As described in the bug by fgorski@, we shouldn't
go longer than 3 minutes since that's the timeout on GCMNetworkManager..