Added incident report for variations seed signature mismatch.

Added incident report for variations seed signature mismatch.

- Keep record of a bad signature in VariationsSeedStore::LoadSeed.
- Register VerifyVariationsSeedSignature in SafeBrowsingService::RegisterAllDelayedAnalysis to send an incident report, if a bad signature is encountered.

BUG=423467
Committed: https://crrev.com/37907039f034c6e39c2590fcfb49b7ce05a8b569
Cr-Commit-Position: refs/heads/master@{#299964}

[grt (UTC plus 2), 2014-10-10 03:05:29]

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

[Georges Khalil, 2014-10-10 12:57:47]

The CQ bit was checked by georgesak@chromium.org

[Georges Khalil, 2014-10-10 12:57:47]

The CQ bit was unchecked by georgesak@chromium.org

[grt (UTC plus 2), 2014-10-10 17:45:43]

this is a great start. comments below.

https://codereview.chromium.org/646733002/diff/200001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.h
(right):

https://codereview.chromium.org/646733002/diff/200001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.h:16:
// will verify if the finch config signature was valid.
"will verify the finch config signature."

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:13: #include
"chrome/browser/browser_process.h"
remove these

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:127: bad_signature_ =
std::string();
  bad_signature_.clear();

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:158: // Record the
bad signature, to be used later for sending an incident
same comment about documenting the current consumer.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:13: #include
"chrome/browser/safe_browsing/incident_reporting/add_incident_callback.h"
remove these

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:60: // Returns the bad
signature that was loaded from the config or an empty
using the empty string like this overloads these cases:
- signature was valid
- signature verification is disabled
- signature value was expected but missing (VARIATIONS_SEED_SIGNATURE_MISSING)
this last case should result in a report being sent, so i think you need an
additional piece of state to disambiguate.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:61: // string if not
the case. Used to send an incident report.
omit " Used to send an incident report." since this comment will become stale if
another consumer comes around.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:62: std::string
GetBadSignature() const { return bad_signature_; }
imo, "invalid" is better than "bad" for the function, doc, and variable name.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:147:
EXPECT_EQ(seed_store_reel.GetBadSignature(), "");
for EXPECT_EQ, the expected value should come first as per
https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:610: void
VariationsService::OnDelayedSendIncidentReport(
rather than embed a dependency on safe_browsing here in the variations service,
i think it makes more sense to add an accessor to VariationsService so that the
analyzer in safe_browsing can ask it if the signature was invalid and, if so,
ask it for the value.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:19: #include
"chrome/browser/safe_browsing/safe_browsing_service.h"
this could be just incident_reporting/add_incicent_callback.h, no?

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:22: #include
"chrome/common/safe_browsing/csd.pb.h"
remove

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.cc:20:
base::Unretained(g_browser_process->variations_service())));
make OnDelayedSendIncidentReport a free function in here that calls
g_browser_process->variations_service() to get a pointer to the service.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.cc:17:
return incident_data.finch_config_signature().signature_hash();
return some constant value here since there is only one variations seed
signature that is being validated. this could be as simple as:
  // Use a constant key since all incidents pertain to the variations seed
signature.
  return "value";

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.h
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.h:16:
// Returns the signature of the finch configuration.
// Returns a constant value since all incidents pertain to the same seed
signature.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.h:20:
// Returns a digest of the finch configuration signature module.
// Returns a digest of the invalid seed signature value.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers_unittest.cc:24:
// Tests that GetKey returns the dll path.
copy-n-paste? ;-)

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers_unittest.cc:25:
TEST(GetFinchConfigSignatureIncidentKey, KeyIsPath) {
make this a KeyIsConstant test that verifies that incidents with different
values have the same key.

https://codereview.chromium.org/646733002/diff/420001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:365: message FinchConfigSignatureIncident
{ optional string signature_hash = 1; }
is my understanding correct that this is the base64_seed_signature? if so,
please use variations_seed_signature, which matches the name of the preference.

also, rather than FinchConfigSignatureIncident, let's go with
VariationsSeedSignatureIncident here and throughout the CL.

[Georges Khalil, 2014-10-10 20:19:32]

Changes done, as requested.

https://codereview.chromium.org/646733002/diff/200001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.h
(right):

https://codereview.chromium.org/646733002/diff/200001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.h:16:
// will verify if the finch config signature was valid.
On 2014/10/10 17:45:42, grt wrote:
> "will verify the finch config signature."

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:13: #include
"chrome/browser/browser_process.h"
On 2014/10/10 17:45:42, grt wrote:
> remove these

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:127: bad_signature_ =
std::string();
On 2014/10/10 17:45:42, grt wrote:
>   bad_signature_.clear();

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:158: // Record the
bad signature, to be used later for sending an incident
On 2014/10/10 17:45:42, grt wrote:
> same comment about documenting the current consumer.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:13: #include
"chrome/browser/safe_browsing/incident_reporting/add_incident_callback.h"
On 2014/10/10 17:45:42, grt wrote:
> remove these

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:60: // Returns the bad
signature that was loaded from the config or an empty
On 2014/10/10 17:45:42, grt wrote:
> using the empty string like this overloads these cases:
> - signature was valid
> - signature verification is disabled
> - signature value was expected but missing (VARIATIONS_SEED_SIGNATURE_MISSING)
> this last case should result in a report being sent, so i think you need an
> additional piece of state to disambiguate.

This is by design. We only want a report if the signature is invalid for now.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:61: // string if not
the case. Used to send an incident report.
On 2014/10/10 17:45:42, grt wrote:
> omit " Used to send an incident report." since this comment will become stale
if
> another consumer comes around.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:62: std::string
GetBadSignature() const { return bad_signature_; }
On 2014/10/10 17:45:42, grt wrote:
> imo, "invalid" is better than "bad" for the function, doc, and variable name.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:147:
EXPECT_EQ(seed_store_reel.GetBadSignature(), "");
On 2014/10/10 17:45:42, grt wrote:
> for EXPECT_EQ, the expected value should come first as per
> https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:610: void
VariationsService::OnDelayedSendIncidentReport(
On 2014/10/10 17:45:42, grt wrote:
> rather than embed a dependency on safe_browsing here in the variations
service,
> i think it makes more sense to add an accessor to VariationsService so that
the
> analyzer in safe_browsing can ask it if the signature was invalid and, if so,
> ask it for the value.

Agreed, modified the code to reflect this change.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:19: #include
"chrome/browser/safe_browsing/safe_browsing_service.h"
On 2014/10/10 17:45:42, grt wrote:
> this could be just incident_reporting/add_incicent_callback.h, no?

Yes, changed.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:22: #include
"chrome/common/safe_browsing/csd.pb.h"
On 2014/10/10 17:45:42, grt wrote:
> remove

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_analyzer.cc:20:
base::Unretained(g_browser_process->variations_service())));
On 2014/10/10 17:45:42, grt wrote:
> make OnDelayedSendIncidentReport a free function in here that calls
> g_browser_process->variations_service() to get a pointer to the service.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.h
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.h:16:
// Returns the signature of the finch configuration.
On 2014/10/10 17:45:43, grt wrote:
> // Returns a constant value since all incidents pertain to the same seed
> signature.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers.h:20:
// Returns a digest of the finch configuration signature module.
On 2014/10/10 17:45:43, grt wrote:
> // Returns a digest of the invalid seed signature value.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers_unittest.cc:24:
// Tests that GetKey returns the dll path.
On 2014/10/10 17:45:43, grt wrote:
> copy-n-paste? ;-)

Oops! :)

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/finch_config_signature_incident_handlers_unittest.cc:25:
TEST(GetFinchConfigSignatureIncidentKey, KeyIsPath) {
On 2014/10/10 17:45:43, grt wrote:
> make this a KeyIsConstant test that verifies that incidents with different
> values have the same key.

Done.

https://codereview.chromium.org/646733002/diff/420001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:365: message FinchConfigSignatureIncident
{ optional string signature_hash = 1; }
On 2014/10/10 17:45:43, grt wrote:
> is my understanding correct that this is the base64_seed_signature? if so,
> please use variations_seed_signature, which matches the name of the
preference.
> 
> also, rather than FinchConfigSignatureIncident, let's go with
> VariationsSeedSignatureIncident here and throughout the CL.

Done.

[grt (UTC plus 2), 2014-10-14 01:28:30]

lookin' good

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/420001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:60: // Returns the bad
signature that was loaded from the config or an empty
On 2014/10/10 20:19:31, georgesak wrote:
> On 2014/10/10 17:45:42, grt wrote:
> > using the empty string like this overloads these cases:
> > - signature was valid
> > - signature verification is disabled
> > - signature value was expected but missing
(VARIATIONS_SEED_SIGNATURE_MISSING)
> > this last case should result in a report being sent, so i think you need an
> > additional piece of state to disambiguate.
> 
> This is by design. We only want a report if the signature is invalid for now.

Acknowledged.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:13: #include
"chrome/browser/browser_process.h"
unused

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:117: :
local_state_(local_state) {
the new bool must be initialized:
    : is_invalid_signature(), local_state_(local_state) {

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:13: #include
"chrome/browser/safe_browsing/incident_reporting/add_incident_callback.h"
unused

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:59: // Returns the bad
signature that was loaded from the config or an empty
please document return value as well as whether or not |invalid_signature| is
modified when false is returned.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:62: //  std::string
GetInvalidSignature() const { return invalid_signature_; }
remove

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:124: // The
below seed and signature pair were generated using the server's
how about moving this new test into its own "GetInvalidSignature" test function?
fine-grained tests make it easier to diagnose failures. this also allows you to
use |seed_store| for the name of the store under test since it won't collide
with the TestVariationsSeedStore used throughout this function.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:147: //
Valid signature, so we should get an empty string.
update comment

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:148:
EXPECT_EQ(false, seed_store_reel.GetInvalidSignature(&invalid_signature));
EXPECT_FALSE

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:154:
EXPECT_EQ(true, seed_store_reel.GetInvalidSignature(&invalid_signature));
EXPECT_FALSE

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:160:
EXPECT_EQ(false, seed_store_reel.GetInvalidSignature(&invalid_signature));
EXPECT_TRUE

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:19: #include
"chrome/browser/safe_browsing/safe_browsing_service.h"
unused

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:9:
#include
"chrome/browser/safe_browsing/incident_reporting/add_incident_callback.h"
already included in .h

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.h
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.h:8:
#include <vector>
unused

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.cc
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.cc:16:
return "VariationsSeedSignatureIncidentKey";
since this names a value in a dict specific to the variations_seed_signature
incident type, the "IncidentKey" suffix is superfluous. i propose either
"VariationsSeedSignature" or "variations_seed_signature". i lean toward the
latter since it's more consistent with the way preferences are named in
Chromium.

https://codereview.chromium.org/646733002/diff/910001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:2604:
'browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.cc',
add
'browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.h',
to this list

[Georges Khalil, 2014-10-14 13:51:10]

Responded to latest comments.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:13: #include
"chrome/browser/browser_process.h"
On 2014/10/14 01:28:30, grt wrote:
> unused

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:117: :
local_state_(local_state) {
On 2014/10/14 01:28:30, grt wrote:
> the new bool must be initialized:
>     : is_invalid_signature(), local_state_(local_state) {

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:13: #include
"chrome/browser/safe_browsing/incident_reporting/add_incident_callback.h"
On 2014/10/14 01:28:30, grt wrote:
> unused

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:59: // Returns the bad
signature that was loaded from the config or an empty
On 2014/10/14 01:28:30, grt wrote:
> please document return value as well as whether or not |invalid_signature| is
> modified when false is returned.

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:62: //  std::string
GetInvalidSignature() const { return invalid_signature_; }
On 2014/10/14 01:28:30, grt wrote:
> remove

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:124: // The
below seed and signature pair were generated using the server's
On 2014/10/14 01:28:30, grt wrote:
> how about moving this new test into its own "GetInvalidSignature" test
function?
> fine-grained tests make it easier to diagnose failures. this also allows you
to
> use |seed_store| for the name of the store under test since it won't collide
> with the TestVariationsSeedStore used throughout this function.

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:147: //
Valid signature, so we should get an empty string.
On 2014/10/14 01:28:30, grt wrote:
> update comment

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:148:
EXPECT_EQ(false, seed_store_reel.GetInvalidSignature(&invalid_signature));
On 2014/10/14 01:28:30, grt wrote:
> EXPECT_FALSE

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:154:
EXPECT_EQ(true, seed_store_reel.GetInvalidSignature(&invalid_signature));
On 2014/10/14 01:28:30, grt wrote:
> EXPECT_FALSE

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:160:
EXPECT_EQ(false, seed_store_reel.GetInvalidSignature(&invalid_signature));
On 2014/10/14 01:28:30, grt wrote:
> EXPECT_TRUE

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:19: #include
"chrome/browser/safe_browsing/safe_browsing_service.h"
On 2014/10/14 01:28:30, grt wrote:
> unused

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:9:
#include
"chrome/browser/safe_browsing/incident_reporting/add_incident_callback.h"
On 2014/10/14 01:28:30, grt wrote:
> already included in .h

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.h
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.h:8:
#include <vector>
On 2014/10/14 01:28:30, grt wrote:
> unused

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.cc
(right):

https://codereview.chromium.org/646733002/diff/910001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.cc:16:
return "VariationsSeedSignatureIncidentKey";
On 2014/10/14 01:28:30, grt wrote:
> since this names a value in a dict specific to the variations_seed_signature
> incident type, the "IncidentKey" suffix is superfluous. i propose either
> "VariationsSeedSignature" or "variations_seed_signature". i lean toward the
> latter since it's more consistent with the way preferences are named in
> Chromium.

Done.

https://codereview.chromium.org/646733002/diff/910001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/646733002/diff/910001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:2604:
'browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.cc',
On 2014/10/14 01:28:30, grt wrote:
> add
>
'browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.h',
> to this list

Done.

[grt (UTC plus 2), 2014-10-14 16:54:23]

grt@chromium.org changed reviewers:
+ asvitkine@chromium.org

[grt (UTC plus 2), 2014-10-14 16:54:24]

code lgtm. please update the CL description to use the same terminology as the
code. also worth "git cl try" to get a full set of tryjobs going.

+asvitkine for variations and histograms.xml OWNERS review.

[Alexei Svitkine (slow), 2014-10-14 17:30:02]

Please make the first line of the CL description the same as the CL title (else
it won't show up in the commit message).

Also, please associate this CL with a corresponding crbug.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:282: if
(!is_invalid_signature_) {
Nit: No need for {}'s if body is a single line.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:58: // If the
signature is invalid, returns true and sets |invalid_signature|.
Sets it to what?

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:90: bool
is_invalid_signature_;
Do we need to have both this bool and the field below, or can we conclude that
there was an invalid signature if the field below is not empty?

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:91: std::string
invalid_signature_;
Please document what this field stores.

Also, please move these members further below - since local_state_ is
initialized during construction, it makes sense for it to be first.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:138: bool
IsVariationsSeedSignatureInvalid(std::string* invalid_signature) const {
This should be implemented in the .cc file, since it's not a trivial method
(else it should use hacker_style).

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:26:
safe_browsing::ClientIncidentReport_IncidentData incident;
This file should be in the safe_browsing namespace, so you don't have to prefix
these.

[Georges Khalil, 2014-10-14 18:00:32]

Responded to comments by Alexei.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.cc:282: if
(!is_invalid_signature_) {
On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> Nit: No need for {}'s if body is a single line.

Done.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:58: // If the
signature is invalid, returns true and sets |invalid_signature|.
On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> Sets it to what?

Rewrote the comment to make it clearer.

It sets |invalid_signature| to the value of the invalid loaded signature from
the local state.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:90: bool
is_invalid_signature_;
On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> Do we need to have both this bool and the field below, or can we conclude that
> there was an invalid signature if the field below is not empty?

Reasoning behind this is if we decide later on to consider empty signatures as
invalid, we won't need to modify this file.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:91: std::string
invalid_signature_;
On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> Please document what this field stores.
> 
> Also, please move these members further below - since local_state_ is
> initialized during construction, it makes sense for it to be first.

Done.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:138: bool
IsVariationsSeedSignatureInvalid(std::string* invalid_signature) const {
On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> This should be implemented in the .cc file, since it's not a trivial method
> (else it should use hacker_style).

Done.

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:26:
safe_browsing::ClientIncidentReport_IncidentData incident;
On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> This file should be in the safe_browsing namespace, so you don't have to
prefix
> these.

Done.

[Alexei Svitkine (slow), 2014-10-14 18:15:07]

I think you also missed my top-level comments from my last reply (about the CL
description).

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/960001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:90: bool
is_invalid_signature_;
On 2014/10/14 18:00:32, georgesak wrote:
> On 2014/10/14 17:30:01, Alexei Svitkine wrote:
> > Do we need to have both this bool and the field below, or can we conclude
that
> > there was an invalid signature if the field below is not empty?
> 
> Reasoning behind this is if we decide later on to consider empty signatures as
> invalid, we won't need to modify this file.

Are you planning to do that immediately after? If not, I think its best to make
the code simpler in this CL and the bool can always be added later if necessary
by the CL that needs it.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:59: //
|invalid_signature| to the loaded signature. Otherwise returns false and
I would mention that the content of |invalid_signature| is actually the
base64-encoded serialized seed data. (Otherwise it's not clear if it's just the
serialized proto bytes, which are also commonly stored in std::string). Maybe
even rename the param to make that clear - e.g. |invalid_baset64_signature| -
and do it throughout.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:138: bool
IsVariationsSeedSignatureInvalid(std::string* invalid_signature) const;
Would it also make sense to log the seed signature, in addition to the seed?

That way, we could maybe detect which cases are from e.g. just a bit-flip in the
signature, vs. tampering.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.h
(right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.h:17:
// signature.
I don't understand this comment. I suggest explaining what the key is that's
reported - i.e. what it's for, etc.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:34:
ASSERT_EQ(std::string("variations_seed_signature"),
Nit: Do you need the std::string() wrapper? I believe it shouldn't be necessary.
Same above.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:59:
ASSERT_NE(GetVariationsSeedSignatureIncidentDigest(*incident),
Nit: You should use EXPECT_NE. ASSERT_* macros should only be used if an early
return is needed in the case of failure - e.g. if the code below it would crash
if it fails. I believe EXPECT_* macros make sense above as well.

https://codereview.chromium.org/646733002/diff/980001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/646733002/diff/980001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:366: optional string
variations_seed_signature = 1;
You should probably comment and mention that this is base64 encoded.

[Georges Khalil, 2014-10-15 14:41:47]

Responded to comments.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:59: //
|invalid_signature| to the loaded signature. Otherwise returns false and
On 2014/10/14 18:15:06, Alexei Svitkine wrote:
> I would mention that the content of |invalid_signature| is actually the
> base64-encoded serialized seed data. (Otherwise it's not clear if it's just
the
> serialized proto bytes, which are also commonly stored in std::string). Maybe
> even rename the param to make that clear - e.g. |invalid_baset64_signature| -
> and do it throughout.

Done.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:138: bool
IsVariationsSeedSignatureInvalid(std::string* invalid_signature) const;
On 2014/10/14 18:15:06, Alexei Svitkine wrote:
> Would it also make sense to log the seed signature, in addition to the seed?
> 
> That way, we could maybe detect which cases are from e.g. just a bit-flip in
the
> signature, vs. tampering.

It could be interesting eventually. For now, we'll only log the signature.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.h
(right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.h:17:
// signature.
On 2014/10/14 18:15:06, Alexei Svitkine wrote:
> I don't understand this comment. I suggest explaining what the key is that's
> reported - i.e. what it's for, etc.

Done.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:34:
ASSERT_EQ(std::string("variations_seed_signature"),
On 2014/10/14 18:15:06, Alexei Svitkine wrote:
> Nit: Do you need the std::string() wrapper? I believe it shouldn't be
necessary.
> Same above.

Done.

https://codereview.chromium.org/646733002/diff/980001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:59:
ASSERT_NE(GetVariationsSeedSignatureIncidentDigest(*incident),
On 2014/10/14 18:15:06, Alexei Svitkine wrote:
> Nit: You should use EXPECT_NE. ASSERT_* macros should only be used if an early
> return is needed in the case of failure - e.g. if the code below it would
crash
> if it fails. I believe EXPECT_* macros make sense above as well.

Done.

https://codereview.chromium.org/646733002/diff/980001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/646733002/diff/980001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:366: optional string
variations_seed_signature = 1;
On 2014/10/14 18:15:06, Alexei Svitkine wrote:
> You should probably comment and mention that this is base64 encoded.

Discussed with owners offline.

[Alexei Svitkine (slow), 2014-10-15 14:53:07]

LGTM % remaining nits, thanks!

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_seed_store_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:138:
variations::VariationsSeed loaded_seed;
Nit: Move this closer to where it's used - i.e. above the LoadSeed() call.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:176:
EXPECT_EQ("", invalid_signature);
Nit: std::string() instead of "". Same thing above.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_service.h:137: // string if the
signature was not invalid.
Nit: "was not invalid" -> "was valid"

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:25:
std::string invalid_signature = g_browser_process->variations_service()
Nit: Wrap after =

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:30:
safe_browsing::
Nit: No need for namespace here.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc:13:
scoped_ptr<ClientIncidentReport_IncidentData> MakeIncident() {
Nit: Put this function in an anon namespace inside the safe_browsing namespace.

[Alexei Svitkine (slow), 2014-10-15 14:55:57]

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:26:
->GetInvalidVariationsSeedSignature();
Actually, one other thing - g_browser_process->variations_service() may be NULL
- for example if it's a Chromium build. You should add an early return in that
case.

[grt (UTC plus 2), 2014-10-15 15:08:07]

comment question

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store.h:59: // signature was
not invalid.
shouldn't this be: "..., or an empty string if the signature was valid, missing,
or if signature verification is disabled."? same q for the comment in
variations_service.h. what do you think, alexei?

[Alexei Svitkine (slow), 2014-10-15 15:16:31]

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store.h:59: // signature was
not invalid.
On 2014/10/15 15:08:07, grt wrote:
> shouldn't this be: "..., or an empty string if the signature was valid,
missing,
> or if signature verification is disabled."? same q for the comment in
> variations_service.h. what do you think, alexei?

Yeah, that sounds even better!

[Georges Khalil, 2014-10-15 16:53:20]

Responded to comments.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store.h:59: // signature was
not invalid.
On 2014/10/15 15:08:07, grt wrote:
> shouldn't this be: "..., or an empty string if the signature was valid,
missing,
> or if signature verification is disabled."? same q for the comment in
> variations_service.h. what do you think, alexei?

Done.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_seed_store_unittest.cc
(right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:138:
variations::VariationsSeed loaded_seed;
On 2014/10/15 14:53:07, Alexei Svitkine wrote:
> Nit: Move this closer to where it's used - i.e. above the LoadSeed() call.

Done.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_seed_store_unittest.cc:176:
EXPECT_EQ("", invalid_signature);
On 2014/10/15 14:53:07, Alexei Svitkine wrote:
> Nit: std::string() instead of "". Same thing above.

Done.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_service.h:137: // string if the
signature was not invalid.
On 2014/10/15 14:53:07, Alexei Svitkine wrote:
> Nit: "was not invalid" -> "was valid"

Changed to the same description as in variations_seed_store.h.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:25:
std::string invalid_signature = g_browser_process->variations_service()
On 2014/10/15 14:53:07, Alexei Svitkine wrote:
> Nit: Wrap after =

I tried, it won't fit. This is the output of git cl format.

https://codereview.chromium.org/646733002/diff/1000001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:26:
->GetInvalidVariationsSeedSignature();
On 2014/10/15 14:55:56, Alexei Svitkine wrote:
> Actually, one other thing - g_browser_process->variations_service() may be
NULL
> - for example if it's a Chromium build. You should add an early return in that
> case.

Done.

[grt (UTC plus 2), 2014-10-15 17:29:42]

lgtm with one final nit. thanks.

https://codereview.chromium.org/646733002/diff/1020001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/1020001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:25:
if (g_browser_process->variations_service() == nullptr) {
nit: omit braces when the body is a single line.

[grt (UTC plus 2), 2014-10-16 01:07:39]

grt@chromium.org changed reviewers:
+ mattm@chromium.org

[grt (UTC plus 2), 2014-10-16 01:08:07]

+mattm for safe_browsing OWNERS review

[mattm, 2014-10-16 02:38:12]

lgtm with nits

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2606:
'browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.h',
sorting

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:670:
'browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc',
sorting

[Georges Khalil, 2014-10-16 12:43:32]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-10-16 12:44:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646733002/1060001

[Georges Khalil, 2014-10-16 13:21:27]

Fixed last nits.

https://codereview.chromium.org/646733002/diff/1020001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc
(right):

https://codereview.chromium.org/646733002/diff/1020001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/variations_seed_signature_analyzer.cc:25:
if (g_browser_process->variations_service() == nullptr) {
On 2014/10/15 17:29:42, grt wrote:
> nit: omit braces when the body is a single line.

Done.

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2606:
'browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers.h',
On 2014/10/16 02:38:12, mattm wrote:
> sorting

Done.

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/646733002/diff/1040001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:670:
'browser/safe_browsing/incident_reporting/variations_seed_signature_incident_handlers_unittest.cc',
On 2014/10/16 02:38:12, mattm wrote:
> sorting

Done.

[commit-bot: I haz the power, 2014-10-16 14:31:32]

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

[commit-bot: I haz the power, 2014-10-16 14:31:34]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Georges Khalil, 2014-10-16 19:55:35]

Patchset #14 (id:1080001) has been deleted

[grt (UTC plus 2), 2014-10-16 19:58:12]

lgtm

[Georges Khalil, 2014-10-16 19:58:57]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-10-16 19:59:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646733002/1100001

[commit-bot: I haz the power, 2014-10-16 20:15:28]

Committed patchset #14 (id:1100001)

[commit-bot: I haz the power, 2014-10-16 20:16:41]

Patchset 14 (id:??) landed as
https://crrev.com/37907039f034c6e39c2590fcfb49b7ce05a8b569
Cr-Commit-Position: refs/heads/master@{#299964}