Add gn args and BUILDFLAGS for OfflinePages, instead on relying on is_android and is_official_build.

Add gn args and BUILDFLAGS for OfflinePages, instead on relying on is_android and is_official_build.
This will reliably remove the manual test harness code from the builds tested on waterfall.
Also it's in preparation for potential move of OfflinePages to other platforms (like ChromeOS).


BUG=NONE

Review-Url: https://codereview.chromium.org/2875853003
Cr-Commit-Position: refs/heads/master@{#471023}
Committed: https://chromium.googlesource.com/chromium/src/+/7cda0c17761fa409ebaf4907dc042d7c9fb691f7

[Dmitry Titov, 2017-05-11 03:24:58]

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

[commit-bot: I haz the power, 2017-05-11 03:26:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2017-05-11 04:06:01]

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

[commit-bot: I haz the power, 2017-05-11 04:53:45]

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

[commit-bot: I haz the power, 2017-05-11 04:53:46]

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-05-11 17:03:40]

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2107:
"android/offline_pages/background_loader_offliner.cc",
I wonder if it makes sense to just have BUILD.gn in android/offline_pages/ that
defines a source_set.

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2168:
"android/offline_pages/evaluation/evaluation_test_scheduler.cc",
Are there no tests for these files? If there are tests then they won't build
without the flag.  It makes sense to have tests for these files, at least to
ensure they can compile.

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:185: #include
"components/offline_pages/features/features.h"
nit: why add this now?

https://codereview.chromium.org/2875853003/diff/20001/components/offline_page...
File components/offline_pages/features/features.gni (right):

https://codereview.chromium.org/2875853003/diff/20001/components/offline_page...
components/offline_pages/features/features.gni:12: enable_offline_pages_harness
= false
Probably want to +cc romax and petewil

[Dmitry Titov, 2017-05-11 17:35:03]

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

[Dmitry Titov, 2017-05-11 17:35:04]

+romax@ for harness flag and tests Q from Justin.

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2107:
"android/offline_pages/background_loader_offliner.cc",
On 2017/05/11 17:03:40, dewittj wrote:
> I wonder if it makes sense to just have BUILD.gn in android/offline_pages/
that
> defines a source_set.

Perhaps. However most of the sources in c/b are included directly into this
target, so it is more consistent with the rest of this file to do this...

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2168:
"android/offline_pages/evaluation/evaluation_test_scheduler.cc",
On 2017/05/11 17:03:40, dewittj wrote:
> Are there no tests for these files? If there are tests then they won't build
> without the flag.  It makes sense to have tests for these files, at least to
> ensure they can compile.

I don't know if there are tests for these, and adding tests if they are not
there would be out of scope for this CL. Note the offline_pages unittests are
still unconditionally included into unittest build on all platforms, and pull in
the core via deps. We might deal with this later as it seems orthogonal.

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:185: #include
"components/offline_pages/features/features.h"
On 2017/05/11 17:03:40, dewittj wrote:
> nit: why add this now?

It defines the BUILDFLAG(ENABLE_OFFLINE_PAGES_HARNESS) used below. This seems to
be a regular practice for features.h files to build those and include
unconditionally - see printing/features in this file for example.

https://codereview.chromium.org/2875853003/diff/20001/components/offline_page...
File components/offline_pages/features/features.gni (right):

https://codereview.chromium.org/2875853003/diff/20001/components/offline_page...
components/offline_pages/features/features.gni:12: enable_offline_pages_harness
= false
On 2017/05/11 17:03:40, dewittj wrote:
> Probably want to +cc romax and petewil

Discussed it with romax, adding him as R

[romax, 2017-05-11 18:09:21]

lgtm!

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2875853003/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2168:
"android/offline_pages/evaluation/evaluation_test_scheduler.cc",
On 2017/05/11 17:35:04, Dmitry Titov wrote:
> On 2017/05/11 17:03:40, dewittj wrote:
> > Are there no tests for these files? If there are tests then they won't build
> > without the flag.  It makes sense to have tests for these files, at least to
> > ensure they can compile.
> 
> I don't know if there are tests for these, and adding tests if they are not
> there would be out of scope for this CL. Note the offline_pages unittests are
> still unconditionally included into unittest build on all platforms, and pull
in
> the core via deps. We might deal with this later as it seems orthogonal.

Unfortunately there's no test for these since they're not supposed to be part of
production (i think it's a bad excuse). But on a big picture I guess there is a
chance to rewrite the whole test harness since we're  targeting downloaded pages
when using it.
TBH i don't even care if it will compile, since it won't compile without setting
the flag manually, and we are the only people using it (at least for now).

https://codereview.chromium.org/2875853003/diff/20001/components/offline_page...
File components/offline_pages/features/features.gni (right):

https://codereview.chromium.org/2875853003/diff/20001/components/offline_page...
components/offline_pages/features/features.gni:12: enable_offline_pages_harness
= false
On 2017/05/11 17:35:04, Dmitry Titov wrote:
> On 2017/05/11 17:03:40, dewittj wrote:
> > Probably want to +cc romax and petewil
> 
> Discussed it with romax, adding him as R

this flag LGTM and I think it's better getting rid off that 'is_official_build'
flag :)

[dewittj, 2017-05-11 18:13:14]

lgtm

[Dmitry Titov, 2017-05-11 18:19:50]

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

[Dmitry Titov, 2017-05-11 18:19:51]

+ dtrainor@ for JNI registrar OWNERS look.

[David Trainor- moved to gerrit, 2017-05-11 18:23:13]

jni registration lgtm

[Dmitry Titov, 2017-05-11 18:36:02]

The CQ bit was checked by dimich@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:37:21]

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-05-11 18:49:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494527762349830,
"parent_rev": "5c011426b2ca0721f632cab58f55a2e7ca6781b2", "commit_rev":
"7cda0c17761fa409ebaf4907dc042d7c9fb691f7"}

[commit-bot: I haz the power, 2017-05-11 18:49:25]

Description was changed from

==========
Add gn args and BUILDFLAGS for OfflinePages, instead on relying on is_android
and is_official_build.
This will reliably remove the manual test harness code from the builds tested on
waterfall.
Also it's in preparation for potential move of OfflinePages to other platforms
(like ChromeOS).


BUG=NONE
==========

to

==========
Add gn args and BUILDFLAGS for OfflinePages, instead on relying on is_android
and is_official_build.
This will reliably remove the manual test harness code from the builds tested on
waterfall.
Also it's in preparation for potential move of OfflinePages to other platforms
(like ChromeOS).


BUG=NONE

Review-Url: https://codereview.chromium.org/2875853003
Cr-Commit-Position: refs/heads/master@{#471023}
Committed:
https://chromium.googlesource.com/chromium/src/+/7cda0c17761fa409ebaf4907dc04...
==========

[commit-bot: I haz the power, 2017-05-11 18:49:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7cda0c17761fa409ebaf4907dc04...