Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Update: The server has started sending updates to Google Chrome builds for all supported lists. See: http://b/33182208

BUG=543161
Committed: https://crrev.com/f9409669639beed7cb2859a9e443c3cfad3ea14b
Cr-Commit-Position: refs/heads/master@{#435673}

[vakh (use Gerrit instead), 2016-11-28 23:01:25]

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

[vakh (use Gerrit instead), 2016-11-28 23:01:55]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org

[vakh (use Gerrit instead), 2016-11-28 23:02:26]

Description was changed from

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

to

==========
DO NOT SUBMIT
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

[Jialiu Lin, 2016-11-28 23:02:56]

Description was changed from

==========
DO NOT SUBMIT
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

to

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

[commit-bot: I haz the power, 2016-11-28 23:03:46]

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

[vakh (use Gerrit instead), 2016-11-28 23:03:56]

Description was changed from

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

to

==========
DO NOT SUBMIT yet since the server isn't responding to Google Chrome builds
either. Remove this line and submit when the server starts responding.

Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

[commit-bot: I haz the power, 2016-11-29 01:36:09]

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

[commit-bot: I haz the power, 2016-11-29 01:36:12]

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

[vakh (use Gerrit instead), 2016-11-29 02:50:32]

Description was changed from

==========
DO NOT SUBMIT yet since the server isn't responding to Google Chrome builds
either. Remove this line and submit when the server starts responding.

Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

to

==========
DO NOT SUBMIT yet since the server isn't responding to Google Chrome builds
either. Remove this line and submit when the server starts responding.
See: http://b/33182208

Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

[Nathan Parker, 2016-11-29 18:45:33]

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:43:
ListInfo(EnableInternalLists(), "CertCsdDownloadWhitelist.store",
By setting fetch_updates=false for these, we're still attempting to read from
disk (probably fine), and then returning "false" for the Check*() calls that
access them. I _think_ that return value is the best behavior for most lists,
but I'm not sure about ChromeFileNameClientIncident.  You should ping caitkp@ to
see if this will cause trouble for Incident Reporting.

Please make it more clear what the first argument really does. Something like:

bool kSyncOnlyOnChromeBuilds = EnableInternalLists();  // or set via #ifdef and
scrap this func.
bool kSyncAlways = true;
bool kSyncNever = false
...
ListInfos(kSyncOnlyOnChromeBuilds, ".."..)


An alternate idea: For non-Chrome builds we could just not add the unsupported
ListInfos at all.  Also, then the Check*() functions would need to explicitly
define their non-chrome behavior by checking EnableInternalLists() and then
returning something (true/false). Or, we could add an enum/option here which is
"fake-list-that-always-returns-true" (or false).

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:143: bool
EnableInternalLists();
Add a comment explaining why some lists are internal, and what internal means.
Or drop this per my comment on the other file.

[vakh (use Gerrit instead), 2016-11-29 20:24:49]

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:43:
ListInfo(EnableInternalLists(), "CertCsdDownloadWhitelist.store",
> I _think_ that return value is the best behavior for most lists,
but I'm not sure about ChromeFileNameClientIncident.  You should ping caitkp@ to
see if this will cause trouble for Incident Reporting.

Not sure why that list is different. Can you please explain so that I can
provide better context to caitkp? Thanks.

[Nathan Parker, 2016-11-29 20:29:57]

Details on the usage of that list:
https://cs.chromium.org/chromium/src/components/safe_browsing_db/database_man...

The comments suggest it should default to true.

The usage:
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/incident_re...

Basically if a DLL is injected into Chrome that's not on the (small, I think )
whitelist, it may generate an incident report to get sent.

[vakh (use Gerrit instead), 2016-11-29 20:54:14]

Address nparker's comments

[vakh (use Gerrit instead), 2016-11-29 20:54:16]

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

[commit-bot: I haz the power, 2016-11-29 20:54:56]

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

[vakh (use Gerrit instead), 2016-11-29 20:56:34]

Nit: Change negative-negative comment to positive

[vakh (use Gerrit instead), 2016-11-29 20:56:36]

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

[vakh (use Gerrit instead), 2016-11-29 20:56:53]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-29 20:57:39]

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

[vakh (use Gerrit instead), 2016-11-29 21:00:13]

nparker@ -- I've changed the code to explicitly return 'true' for Match* methods
that expect true on error. PTAL and let me know if we still need to give a heads
up to caitkp@

Please note that MatchModuleWhitelistString would only be called under V4Only
which means that the build has got to be a GoogleChrome build.

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:43:
ListInfo(EnableInternalLists(), "CertCsdDownloadWhitelist.store",
On 2016/11/29 20:24:49, vakh (Varun Khaneja) wrote:
> > I _think_ that return value is the best behavior for most lists,
> but I'm not sure about ChromeFileNameClientIncident.  You should ping caitkp@
to
> see if this will cause trouble for Incident Reporting.
> 
> Not sure why that list is different. Can you please explain so that I can
> provide better context to caitkp? Thanks.

Done.

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2536913002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:143: bool
EnableInternalLists();
On 2016/11/29 18:45:33, Nathan Parker wrote:
> Add a comment explaining why some lists are internal, and what internal means.
> Or drop this per my comment on the other file.

Done.

[commit-bot: I haz the power, 2016-11-30 00:04:26]

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

[commit-bot: I haz the power, 2016-11-30 00:04:26]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Nathan Parker, 2016-11-30 21:34:59]

lgtm

[vakh (use Gerrit instead), 2016-12-01 18:35:35]

Description was changed from

==========
DO NOT SUBMIT yet since the server isn't responding to Google Chrome builds
either. Remove this line and submit when the server starts responding.
See: http://b/33182208

Small: PVer4: Enable syncing internal lists for Google Chrome builds.

BUG=543161
==========

to

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Update: The server has started sending updates to Google Chrome builds for all
supported lists. See: http://b/33182208

BUG=543161
==========

[vakh (use Gerrit instead), 2016-12-01 18:35:41]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-12-01 18:35:59]

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-12-01 19:23:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480617341624930,
"parent_rev": "6d6d0f9d469ba6a05d516662664e16a51e8e3e87", "commit_rev":
"9cecc6504b0d5a29370477dc9343a9ea83704666"}

[commit-bot: I haz the power, 2016-12-01 19:24:14]

Description was changed from

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Update: The server has started sending updates to Google Chrome builds for all
supported lists. See: http://b/33182208

BUG=543161
==========

to

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Update: The server has started sending updates to Google Chrome builds for all
supported lists. See: http://b/33182208

BUG=543161
==========

[commit-bot: I haz the power, 2016-12-01 19:24:16]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-12-01 19:26:47]

Description was changed from

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Update: The server has started sending updates to Google Chrome builds for all
supported lists. See: http://b/33182208

BUG=543161
==========

to

==========
Small: PVer4: Enable syncing internal lists for Google Chrome builds.

Update: The server has started sending updates to Google Chrome builds for all
supported lists. See: http://b/33182208

BUG=543161

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

[commit-bot: I haz the power, 2016-12-01 19:26:48]

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