Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558913003:
Restrict transmission of external exp ids to signed in users. (Closed)
|
Created:
4 years ago by Alexei Svitkine (slow) Modified:
3 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, ntp-dev+reviews_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, Randy Smith (Not in Mondays), jdonnelly+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, mathp+autofillwatch_chromium.org, asvitkine+watch_chromium.org, loading-reviews_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestrict transmission of external exp ids to signed in users.
Since external experiment ids are not based on Chrome's low
entropy source, they do not have the same guarantees about
not identifying a user as Chrome's variations. As such, we
should only transmit them for signed in users, whose identity
is already known by Google so there's no risk of identifying
them through these headers.
Note: The signed-in state checking in this CL is only done for
web content area requests and not other internal requests,
like to the suggestion service, where it treats the state as
"not signed in". This is fine to do because variations service
ids are still sent, which is what the other call sites are
interested in.
BUG=672532
TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chromium.org
Committed: https://crrev.com/9ed7b5611a61505c3dba734fe55b92211df3c2f6
Cr-Commit-Position: refs/heads/master@{#437959}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix. #Patch Set 3 : Fix windows compile. #Patch Set 4 : google_services_account_id is not initialized in incognito #
Total comments: 2
Patch Set 5 : Add comments to callsites and API headers. #
Total comments: 4
Patch Set 6 : Address nit. #Messages
Total messages: 68 (43 generated)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. BUG= ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. BUG=672532 ==========
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. BUG=672532 ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". BUG=672532 ==========
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". BUG=672532 ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is OK to do, because "not signed in" is a conservative state that results in fewer things sent. BUG=672532 ==========
asvitkine@chromium.org changed reviewers: + jwd@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
mathp@chromium.org changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_download_manager.cc (right): https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_download_manager.cc:250: variations::AppendVariationHeaders(fetcher->GetOriginalURL(), Could we check whether the user is signed in, here? If we don't and pass |false|, this line is basically a no-op (this class is used to communicate with a Google server, where an experiment could be activated based on ClientDataHeader)? Sorry if I'm not understanding correctly.
https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_download_manager.cc (right): https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_download_manager.cc:250: variations::AppendVariationHeaders(fetcher->GetOriginalURL(), On 2016/12/08 18:04:59, Mathieu Perreault wrote: > Could we check whether the user is signed in, here? If we don't and pass > |false|, this line is basically a no-op (this class is used to communicate with > a Google server, where an experiment could be activated based on > ClientDataHeader)? > > Sorry if I'm not understanding correctly. It shouldn't affect the experiments coming from the variations server, so it will work as before.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with comment https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_download_manager.cc (right): https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_download_manager.cc:250: variations::AppendVariationHeaders(fetcher->GetOriginalURL(), On 2016/12/08 20:27:33, Alexei Svitkine (slow) wrote: > On 2016/12/08 18:04:59, Mathieu Perreault wrote: > > Could we check whether the user is signed in, here? If we don't and pass > > |false|, this line is basically a no-op (this class is used to communicate > with > > a Google server, where an experiment could be activated based on > > ClientDataHeader)? > > > > Sorry if I'm not understanding correctly. > > It shouldn't affect the experiments coming from the variations server, so it > will work as before. Thanks for the clarification! Could we add a comment on line 248 about how we are passing |is_signed_in| as false but that Variations server experiment state will still be sent?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is OK to do, because "not signed in" is a conservative state that results in fewer things sent. BUG=672532 ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2558913003/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2558913003/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:684: false, // is_signed_in Essentially the same comment that mathp had: Can you please add a comment explaining why it's okay to simply set this to false here? I was also concerned that this would stop variations from working.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done - added comments to all the call sites as well as expanded comments in API headers. https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_download_manager.cc (right): https://codereview.chromium.org/2558913003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_download_manager.cc:250: variations::AppendVariationHeaders(fetcher->GetOriginalURL(), On 2016/12/08 20:37:26, Mathieu Perreault wrote: > On 2016/12/08 20:27:33, Alexei Svitkine (slow) wrote: > > On 2016/12/08 18:04:59, Mathieu Perreault wrote: > > > Could we check whether the user is signed in, here? If we don't and pass > > > |false|, this line is basically a no-op (this class is used to communicate > > with > > > a Google server, where an experiment could be activated based on > > > ClientDataHeader)? > > > > > > Sorry if I'm not understanding correctly. > > > > It shouldn't affect the experiments coming from the variations server, so it > > will work as before. > > Thanks for the clarification! Could we add a comment on line 248 about how we > are passing |is_signed_in| as false but that Variations server experiment state > will still be sent? Done. https://codereview.chromium.org/2558913003/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2558913003/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:684: false, // is_signed_in On 2016/12/09 12:34:31, Marc Treib wrote: > Essentially the same comment that mathp had: Can you please add a comment > explaining why it's okay to simply set this to false here? I was also concerned > that this would stop variations from working. Done.
Thanks! ntp_snippets lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
asvitkine@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for chrome/browser/loader OWNERS jwd: friendly ping (Planning to TBR other call site owners as it's just changing the function signature and passing false for the new param.)
https://codereview.chromium.org/2558913003/diff/120001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2558913003/diff/120001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:489: !io_data->google_services_account_id()->GetValue().empty(); I think you just need the last check? google_services_user_account_id() should be empty for off the record ProfileIODatas.
https://codereview.chromium.org/2558913003/diff/120001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2558913003/diff/120001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:489: !io_data->google_services_account_id()->GetValue().empty(); On 2016/12/09 19:47:44, mmenke wrote: > I think you just need the last check? google_services_user_account_id() should > be empty for off the record ProfileIODatas. We don't initialize google_services_account_id for OTR, see: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_io_data.... Not having this would hit a DCHECK that it's not initialized.
On 2016/12/09 20:10:44, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/2558913003/diff/120001/chrome/browser/loader/... > File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): > > https://codereview.chromium.org/2558913003/diff/120001/chrome/browser/loader/... > chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:489: > !io_data->google_services_account_id()->GetValue().empty(); > On 2016/12/09 19:47:44, mmenke wrote: > > I think you just need the last check? google_services_user_account_id() > should > > be empty for off the record ProfileIODatas. > > We don't initialize google_services_account_id for OTR, see: > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_io_data.... > > Not having this would hit a DCHECK that it's not initialized. LGTM
jwd: friendly ping We want to get this merged to 56 per discussion with Darin and Tyler - so before that we need to get it into canary for a day asap. On Dec 9, 2016 4:32 PM, <mmenke@chromium.org> wrote: > On 2016/12/09 20:10:44, Alexei Svitkine (slow) wrote: > > > https://codereview.chromium.org/2558913003/diff/120001/ > chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc > > File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc > (right): > > > > > https://codereview.chromium.org/2558913003/diff/120001/ > chrome/browser/loader/chrome_resource_dispatcher_host_ > delegate.cc#newcode489 > > chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:489: > > !io_data->google_services_account_id()->GetValue().empty(); > > On 2016/12/09 19:47:44, mmenke wrote: > > > I think you just need the last check? google_services_user_account_ > id() > > should > > > be empty for off the record ProfileIODatas. > > > > We don't initialize google_services_account_id for OTR, see: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ > profiles/profile_io_data.cc?rcl=1481300494&l=494 > > > > Not having this would hit a DCHECK that it's not initialized. > > LGTM > > https://codereview.chromium.org/2558913003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM with nit. https://codereview.chromium.org/2558913003/diff/120001/components/variations/... File components/variations/variations_associated_data.h (right): https://codereview.chromium.org/2558913003/diff/120001/components/variations/... components/variations/variations_associated_data.h:61: // This collected is used by Google web properties for signed in users only, nit: collection
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org ==========
asvitkine@chromium.org changed reviewers: + donnd@chromium.org, mattm@chromium.org, mpearson@chromium.org
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chrom... ==========
asvitkine@chromium.org changed reviewers: + afakhry@chromium.org
Thanks! TBR'ing OWNERS for downstream users of the API that I'm adding a param for. (mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chromium.org) https://codereview.chromium.org/2558913003/diff/120001/components/variations/... File components/variations/variations_associated_data.h (right): https://codereview.chromium.org/2558913003/diff/120001/components/variations/... components/variations/variations_associated_data.h:61: // This collected is used by Google web properties for signed in users only, On 2016/12/12 16:04:43, jwd wrote: > nit: collection Done.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, treib@chromium.org, jwd@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2558913003/#ps140001 (title: "Address nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by asvitkine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
components/omnibox/... lgtm But, one minor comment: The changelist says: >>> Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. >>> The comments in components/omnibox/... say >>> // Note: It's fine to pass in |is_signed_in| false, which does not affect // transmission of experiment ids coming from the variations server. >>> I think it'd be clearer to say something like: // The |is_signed_in| state doesn't matter; it does not affect // transmission of experiment ids coming from the variations server. The former version simply seems to say that |is_signed_in| == false does not affect transmission of experiment ids. Yet, the changelist description implies that either state is fine. The comment should be clear on this. --mark
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481580293383670,
"parent_rev": "4f87094298441f6722a0ba4a352db48c3080d4f4", "commit_rev":
"53afa6e1ad0612bd0d4b51067bd3da39b2373f78"}
Message was sent while issue was closed.
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chrom... ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chrom... Review-Url: https://codereview.chromium.org/2558913003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chrom... Review-Url: https://codereview.chromium.org/2558913003 ========== to ========== Restrict transmission of external exp ids to signed in users. Since external experiment ids are not based on Chrome's low entropy source, they do not have the same guarantees about not identifying a user as Chrome's variations. As such, we should only transmit them for signed in users, whose identity is already known by Google so there's no risk of identifying them through these headers. Note: The signed-in state checking in this CL is only done for web content area requests and not other internal requests, like to the suggestion service, where it treats the state as "not signed in". This is fine to do because variations service ids are still sent, which is what the other call sites are interested in. BUG=672532 TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chrom... Committed: https://crrev.com/9ed7b5611a61505c3dba734fe55b92211df3c2f6 Cr-Commit-Position: refs/heads/master@{#437959} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9ed7b5611a61505c3dba734fe55b92211df3c2f6 Cr-Commit-Position: refs/heads/master@{#437959}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2569973002/ by yutak@chromium.org. The reason for reverting is: Speculative revert for recent flakiness in Contextual Search related test failures on "Android Tests" bot: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests?numbuild... e.g. https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/3... I'm not very confident that this is the real offender; if it's not, I will reland this CL..
Message was sent while issue was closed.
On 2016/12/13 06:05:56, Yuta Kitamura wrote: > A revert of this CL (patchset #6 id:140001) has been created in > https://codereview.chromium.org/2569973002/ by mailto:yutak@chromium.org. > > The reason for reverting is: Speculative revert for recent flakiness in > Contextual Search related test failures on > "Android Tests" bot: > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests?numbuild... > > e.g. > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/3... > > I'm not very confident that this is the real > offender; if it's not, I will reland this CL.. It turned out this was not a culprit; relanding. Sorry for all the fuss!
Message was sent while issue was closed.
On 2016/12/12 22:38:42, Mark P wrote: > components/omnibox/... lgtm > > But, one minor comment: > > The changelist says: > >>> > Note: The signed-in state checking in this CL is only done for > web content area requests and not other internal requests, > like to the suggestion service, where it treats the state as > "not signed in". This is fine to do because variations service > ids are still sent, which is what the other call sites are > interested in. > >>> > > The comments in components/omnibox/... say > >>> > // Note: It's fine to pass in |is_signed_in| false, which does not affect > // transmission of experiment ids coming from the variations server. > >>> > > I think it'd be clearer to say something like: > // The |is_signed_in| state doesn't matter; it does not affect > // transmission of experiment ids coming from the variations server. > > The former version simply seems to say that |is_signed_in| == false > does not affect transmission of experiment ids. Yet, the changelist > description implies that either state is fine. The comment should > be clear on this. > > --mark Sorry, I didn't ignore this intentionally - just didn't have time to come back to this as I landed the CL right before my holiday vacation. So I'm not a big fan of your suggested "// The |is_signed_in| state doesn't matter" wording - because it does matter in the sense that it's ok to pass in false but we definitely don't want "true" to be passed when it's not the case. Perhaps we can say something like: "// It's OK to pass in |is_signed_in| state false if its unknown, as it does not affect transmission of experiment ids coming from the variations server."
Message was sent while issue was closed.
On 2017/01/11 21:58:55, Alexei Svitkine (slow) wrote: > On 2016/12/12 22:38:42, Mark P wrote: > > components/omnibox/... lgtm > > > > But, one minor comment: > > > > The changelist says: > > >>> > > Note: The signed-in state checking in this CL is only done for > > web content area requests and not other internal requests, > > like to the suggestion service, where it treats the state as > > "not signed in". This is fine to do because variations service > > ids are still sent, which is what the other call sites are > > interested in. > > >>> > > > > The comments in components/omnibox/... say > > >>> > > // Note: It's fine to pass in |is_signed_in| false, which does not affect > > // transmission of experiment ids coming from the variations server. > > >>> > > > > I think it'd be clearer to say something like: > > // The |is_signed_in| state doesn't matter; it does not affect > > // transmission of experiment ids coming from the variations server. > > > > The former version simply seems to say that |is_signed_in| == false > > does not affect transmission of experiment ids. Yet, the changelist > > description implies that either state is fine. The comment should > > be clear on this. > > > > --mark > > Sorry, I didn't ignore this intentionally - just didn't have time to come back > to this as I landed the CL right before my holiday vacation. > > So I'm not a big fan of your suggested "// The |is_signed_in| state doesn't > matter" wording - because it does matter in the sense that it's ok to pass in > false but we definitely don't want "true" to be passed when it's not the case. > > Perhaps we can say something like: > > "// It's OK to pass in |is_signed_in| state false if its unknown, as it does not > affect transmission of experiment ids coming from the variations server." That suggestion sounds great to me, certainly better than mine. --mark
Message was sent while issue was closed.
feedback_uploader_chrome.cc lgtm |
