Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
362553004:
Disable session invalidations on Android with a flag. (Closed)
|
Created:
6 years, 5 months ago by maxbogue Modified:
6 years, 5 months ago CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDisable session invalidations on Android with a flag.
This will allow us to roll the change out gradually using
Finch.
BUG=385213
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283476
Patch Set 1 #Patch Set 2 : Attempt to fix try errors by reading the flag at run time. #Patch Set 3 : Try using LibraryLoader.isInitialized() to fix try tests. #Patch Set 4 : Try checking the ClankSessionNotifications flag at run time. #Patch Set 5 : getNonInvalidationType() -> isNonInvalidationType() #
Total comments: 2
Patch Set 6 : Remove unnecessary else clause. #Patch Set 7 : Rename flag to AndroidSessionNotifications. #Messages
Total messages: 17 (0 generated)
Please review!
how does this work if you first register with the sessions datatype, and then the finch experiment changes. will the client then eventually remove the registration? the implementation of the flag lg by the way. https://codereview.chromium.org/362553004/diff/80001/sync/android/java/src/or... File sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java (right): https://codereview.chromium.org/362553004/diff/80001/sync/android/java/src/or... sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:102: return mNonInvalidationType; Since you return in the if-clause, just unindent the else-clause (i.e. remove else)
On 2014/07/09 21:50:19, nyquist wrote: > how does this work if you first register with the sessions datatype, and then > the finch experiment changes. will the client then eventually remove the > registration? > the implementation of the flag lg by the way. > > https://codereview.chromium.org/362553004/diff/80001/sync/android/java/src/or... > File > sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java > (right): > > https://codereview.chromium.org/362553004/diff/80001/sync/android/java/src/or... > sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:102: > return mNonInvalidationType; > Since you return in the if-clause, just unindent the else-clause (i.e. remove > else) As far as I know, changes to the flag will be reflected the next time Sync initializes (sign out/back in, the app restarts, etc).
https://codereview.chromium.org/362553004/diff/80001/sync/android/java/src/or... File sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java (right): https://codereview.chromium.org/362553004/diff/80001/sync/android/java/src/or... sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:102: return mNonInvalidationType; On 2014/07/09 21:50:19, nyquist wrote: > Since you return in the if-clause, just unindent the else-clause (i.e. remove > else) Done.
meek: PTAL I think you're right in that sync is covered at next startup. I'm more concerned about how invalidations registrations would act. Adding meek@ as well.
After starting the invalidation client, the InvalidationService should get a #reissueRegistrations call. If the set of registrations has changed (is different from what Tango has recorded for the client), registrations should be synced. It might be worth testing the behavior.
On 2014/07/10 17:17:19, colinmeek wrote: > After starting the invalidation client, the InvalidationService should get a > #reissueRegistrations call. If the set of registrations has changed (is > different from what Tango has recorded for the client), registrations should be > synced. It might be worth testing the behavior. How would I test that? And does the Finch function actually call out to the server every time it's called? Otherwise it wouldn't change anyways.
The invalidation client periodically verifies that its internal registration state is consistent with the backend registration state (by exchanging digests). The internal registration state is reset whenever the client starts. Automated testing would be very tricky outside of google3, but you can hopefully manually verify that registrations are successfully updated with the flag switch.
On 2014/07/11 19:35:37, colinmeek wrote: > The invalidation client periodically verifies that its internal registration > state is consistent with the backend registration state (by exchanging digests). > The internal registration state is reset whenever the client starts. Automated > testing would be very tricky outside of google3, but you can hopefully manually > verify that registrations are successfully updated with the flag switch. I can pass the flag into the chrome shell and see sessions disable, if that's what you mean.
On 2014/07/11 20:05:06, maxbogue wrote: > On 2014/07/11 19:35:37, colinmeek wrote: > > The invalidation client periodically verifies that its internal registration > > state is consistent with the backend registration state (by exchanging > digests). > > The internal registration state is reset whenever the client starts. Automated > > testing would be very tricky outside of google3, but you can hopefully > manually > > verify that registrations are successfully updated with the flag switch. > > I can pass the flag into the chrome shell and see sessions disable, if that's > what you mean. If you start up Chrome, without the flag set, and then set the field trial (which might happen after I while I assume), does that trigger re-registrations? What happens if you restart Chrome, does that register the correct registrations? This is regarding the InvalidationController and InvalidationService code.
On 2014/07/11 23:24:50, nyquist wrote: > On 2014/07/11 20:05:06, maxbogue wrote: > > On 2014/07/11 19:35:37, colinmeek wrote: > > > The invalidation client periodically verifies that its internal registration > > > state is consistent with the backend registration state (by exchanging > > digests). > > > The internal registration state is reset whenever the client starts. > Automated > > > testing would be very tricky outside of google3, but you can hopefully > > manually > > > verify that registrations are successfully updated with the flag switch. > > > > I can pass the flag into the chrome shell and see sessions disable, if that's > > what you mean. > > If you start up Chrome, without the flag set, and then set the field trial > (which might happen after I while I assume), does that trigger re-registrations? > What happens if you restart Chrome, does that register the correct > registrations? > > This is regarding the InvalidationController and InvalidationService code. Changing the flag isn't going to trigger a re-registration. I don't see how it could. Restarting Chrome will.
lgtm
On 2014/07/15 18:49:30, nyquist wrote: > lgtm If I switch the command line flag and restart Chrome Shell, it does update the registration status. Hopefully that alleviates some concerns.
lgtm
The CQ bit was checked by maxbogue@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maxbogue@chromium.org/362553004/120001
Message was sent while issue was closed.
Change committed as 283476 |
