Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986173003:
Fixes that host restriction was sometimes applied even when switched off. (Closed)
|
Created:
4 years, 7 months ago by jkrcal Modified:
4 years, 7 months ago CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes that host restriction was sometimes applied even when switched off.
Host restriction in ntp_snippets_service is used even if it is off in
OnSuggestionsChanged. Now the observer is added only when host
restriction is on.
BUG=606320
Committed: https://crrev.com/6bdd3e5b737cb3d6e737ac10b8ec562c0fa0a22e
Cr-Commit-Position: refs/heads/master@{#394377}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixing related API changes #
Total comments: 2
Patch Set 3 : Minor fixes #
Messages
Total messages: 24 (11 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
PTAL.
treib@chromium.org changed reviewers: + dgn@chromium.org
Wow, that was fast! +dgn explicitly, since this will break the "clear snippets on signout" if host restrictions are off. LGTM, but please wait for Nicolas to weigh in before committing. https://codereview.chromium.org/1986173003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1986173003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:189: if (snippets_fetcher_->UseHostRestriction() && suggestions_service_) { Should we (in another CL) make UseHostRestriction (and similarly UseAuthentication) static, to make it a bit clearer that they can never change at runtime?
Ah, also maybe update the descriptions: Host restriction WAS used even if ... As it is, it sounds like you're describing the new state after this CL.
Description was changed from ========== Fixes host restriction being sometimes applied even when switched off. Host restriction in ntp_snippets_service is used even if it is off in OnSuggestionsChanged. Now the observer is added only when host restriction is on. BUG=606320 ========== to ========== Fixes that host restriction was sometimes applied even when switched off. Host restriction in ntp_snippets_service is used even if it is off in OnSuggestionsChanged. Now the observer is added only when host restriction is on. BUG=606320 ==========
lgtm, I'll directly listen to sync state changes instead of trying to guess it from the returned hosts. This should not affect the feature then.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986173003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986173003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Marc: I've forgotten to add related changes in the fetcher. Added for in the second revision. Please complain if you lgtm does not stretch to such changes :) https://codereview.chromium.org/1986173003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1986173003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:189: if (snippets_fetcher_->UseHostRestriction() && suggestions_service_) { On 2016/05/17 15:23:58, Marc Treib wrote: > Should we (in another CL) make UseHostRestriction (and similarly > UseAuthentication) static, to make it a bit clearer that they can never change > at runtime? I agree, in a separate CL. Heh, I've forgotten to make them public in first place :) I was puzzled because I did it in https://codereview.chromium.org/1978513002/. I took over all changes in ntp_snippets_fetcher.h from there to make merging easier.
Still LGTM, with 2 more nits https://codereview.chromium.org/1986173003/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1986173003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:68: // Enumaration listing all possible variants of dealing with personalization. EnumEration, or just Enum :) https://codereview.chromium.org/1986173003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:101: // Returns the last json fetched from the server. Nope :)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986173003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986173003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/1986173003/#ps40001 (title: "Minor fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986173003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986173003/40001
Message was sent while issue was closed.
Description was changed from ========== Fixes that host restriction was sometimes applied even when switched off. Host restriction in ntp_snippets_service is used even if it is off in OnSuggestionsChanged. Now the observer is added only when host restriction is on. BUG=606320 ========== to ========== Fixes that host restriction was sometimes applied even when switched off. Host restriction in ntp_snippets_service is used even if it is off in OnSuggestionsChanged. Now the observer is added only when host restriction is on. BUG=606320 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fixes that host restriction was sometimes applied even when switched off. Host restriction in ntp_snippets_service is used even if it is off in OnSuggestionsChanged. Now the observer is added only when host restriction is on. BUG=606320 ========== to ========== Fixes that host restriction was sometimes applied even when switched off. Host restriction in ntp_snippets_service is used even if it is off in OnSuggestionsChanged. Now the observer is added only when host restriction is on. BUG=606320 Committed: https://crrev.com/6bdd3e5b737cb3d6e737ac10b8ec562c0fa0a22e Cr-Commit-Position: refs/heads/master@{#394377} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6bdd3e5b737cb3d6e737ac10b8ec562c0fa0a22e Cr-Commit-Position: refs/heads/master@{#394377} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
