Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
497563002:
[Android] Use correct enhanced bookmarks enabling logic. (Closed)
|
Created:
6 years, 4 months ago by Kibeom Kim (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, Ted C, Ian Wen, newt (away), cleer1, noyau (Ping after 24h) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Android] Use the correct enhanced bookmarks enabling logic.
Previously, Android's enhanced bookmarks feature depended on
--manual-enhanced-bookmarks flag and nothing else. But the
correct way is to use UpdateBookmarksExperimentState function's logic,
which desktop has been using.
BUG=386785
Committed: https://crrev.com/eedd8b84de9b16ad2af273dbf1f33e3971290565
Cr-Commit-Position: refs/heads/master@{#293509}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use proper enhanced bookmarks flag logic (changed completely) #
Total comments: 6
Patch Set 3 : call UpdateBookmarksExperimentState on startup, JNI binding, etc.. #
Total comments: 4
Patch Set 4 : JNI mistake #
Total comments: 2
Patch Set 5 : Fixed switch checking and confirmed working correctly #Patch Set 6 : indent fix #
Total comments: 2
Patch Set 7 : Removed opt-in flag from this CL's scope. #Patch Set 8 : indent fix #Patch Set 9 : undo function renaming #Patch Set 10 : undo indent fix #
Total comments: 2
Patch Set 11 : Profile * ----> Profile* #
Total comments: 2
Patch Set 12 : || nit #
Messages
Total messages: 45 (8 generated)
lgtm
https://codereview.chromium.org/497563002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/497563002/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:1619: kOsDesktop, Why is this one not | kOsAndroid also?
https://codereview.chromium.org/497563002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/497563002/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:1619: kOsDesktop, On 2014/08/22 01:13:14, Yaron wrote: > Why is this one not | kOsAndroid also? Yes, this and below #1632 should also have kOsAndroid eventually. But for this CL I'm just trying to expose the flag we're using to chrome://flags so that people can turn on without rooting. I was thinking of using enhanced_bookmarks_features.h later when we are OK to turn on by default, because that logic assumes setting the flag is opt out and also reported as UMA, so changing that assumption only for Android seemed non-trivial. I can try this route if you think it's better.
https://codereview.chromium.org/497563002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/497563002/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2039: case chrome::VersionInfo::CHANNEL_DEV: Flags shouldn't be channel-specific. See also the discussion from https://codereview.chromium.org/203473003/#msg39 to the end of that thread.
https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right): https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.h:35: void UpdateBookmarksExperimentState( Yaron: I think I need to call this on profile initialization and profile changed. Do you know where would be the proper places to call?
https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right): https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.h:35: void UpdateBookmarksExperimentState( On 2014/08/26 23:54:50, Kibeom Kim wrote: > Yaron: I think I need to call this on profile initialization and profile > changed. Do you know where would be the proper places to call? Why is this different from desktop versions?
https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right): https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.h:35: void UpdateBookmarksExperimentState( On 2014/08/27 00:09:59, Yaron wrote: > On 2014/08/26 23:54:50, Kibeom Kim wrote: > > Yaron: I think I need to call this on profile initialization and profile > > changed. Do you know where would be the proper places to call? > > Why is this different from desktop versions? The reason I think it's ok is that Finch is per install (not user profile) so is independent of it. If a new user signs in and gets the sync variant, it will fire ExperimentEnabled when it downloads that update.
https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right): https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.h:35: void UpdateBookmarksExperimentState( On 2014/08/27 00:12:13, Yaron wrote: > The reason I think it's ok is that Finch is per install (not user profile) so is > independent of it. If a new user signs in and gets the sync variant, it will > fire ExperimentEnabled when it downloads that update. I see, then I'll just call it once on initialization. (to read the opt-in flag)
https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:199: return bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED Why can't you use the switches variant? Isn't that set by ForceFinchBookmarkExperimentIfNeeded which runsa fter you opt_in code?
https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:199: return bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED On 2014/08/27 20:42:15, Yaron wrote: > Why can't you use the switches variant? Isn't that set by > ForceFinchBookmarkExperimentIfNeeded which runsa fter you opt_in code? I see. I was following the logic here https://chromereviews.googleplex.com/37287013/diff/30010/chrome/browser/bookm... but looks like the following codeblock should have the same effect. bool IsEnhancedBookmarksExperimentEnabled() { CommandLine* command_line = CommandLine::ForCurrentProcess(); if (command_line->HasSwitch(switches::kManualEnhancedBookmarks)) { return true; } return IsEnhancedBookmarksExperimentEnabledFromFinch(); } Actually, I'm not sure why the current IsEnhancedBookmarksExperimentEnabled() returns true on kManualEnhancedBookmarksOptout switch. Do you know? (or I guess I should ask yefim@)
yfriedman@chromium.org changed reviewers: + yefim@chromium.org
yes, +Yefim
updated. (diff to base will be easier to read) https://codereview.chromium.org/497563002/diff/40001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/40001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:186: command_line->HasSwitch(switches::kManualEnhancedBookmarksOptout)) { yefim@: what's the reason for returning true on the optout flag here?
https://codereview.chromium.org/497563002/diff/40001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/497563002/diff/40001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2025: bool SkipConditionalExperiment(const Experiment& experiment) { This whole function looks wrong to me. Why does enhanced-bookmarks-experiment need special-casing at all? (The enable-data-reduction-proxy-dev check below looks wrong too.)
https://codereview.chromium.org/497563002/diff/60001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/60001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:182: if (command_line->HasSwitch(switches::kManualEnhancedBookmarks)) actually, this is wrong, I should get the switch values from PrefService*, fixing..
https://codereview.chromium.org/497563002/diff/60001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/60001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:182: if (command_line->HasSwitch(switches::kManualEnhancedBookmarks)) On 2014/08/27 22:25:53, Kibeom Kim wrote: > actually, this is wrong, I should get the switch values from PrefService*, > fixing.. Done.
https://codereview.chromium.org/497563002/diff/100001/chrome/browser/bookmark... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/100001/chrome/browser/bookmark... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:211: base::StringValue(switches::kManualEnhancedBookmarks)) I don't understand the distinction here. Either explain here or elsewhere (if needed) but I thought that if sync sets the flags, they are propagated to the command line. What causes this to be handled differently from desktop other than the fact that optin is allowed?
Discussed with Yaron offline, it was super confusing, but finally we figured out @_@. I will update again, and come back to Nico's comment later.
ptal. Reduced the scope of CL and split the opt-in flag part to https://codereview.chromium.org/517713002/ https://codereview.chromium.org/497563002/diff/40001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/497563002/diff/40001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2025: bool SkipConditionalExperiment(const Experiment& experiment) { On 2014/08/27 21:52:39, Nico (hiding) wrote: > This whole function looks wrong to me. Why does enhanced-bookmarks-experiment > need special-casing at all? > > (The enable-data-reduction-proxy-dev check below looks wrong too.) So the logic was added here. https://codereview.chromium.org/68173031 . To my understanding, the purpose was to show the opt-out flag only if we are running an experiment for the user, by chrome sync or finch. Maybe yefim@ can comment, but I think if we show the flag always, it can mess up the stats we're monitoring. https://codereview.chromium.org/497563002/diff/100001/chrome/browser/bookmark... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/100001/chrome/browser/bookmark... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:211: base::StringValue(switches::kManualEnhancedBookmarks)) On 2014/08/28 00:17:40, Yaron_OOO_8.28-8.29 wrote: > I don't understand the distinction here. Either explain here or elsewhere (if > needed) but I thought that if sync sets the flags, they are propagated to the > command line. What causes this to be handled differently from desktop other than > the fact that optin is allowed? Done.
lgtm https://codereview.chromium.org/497563002/diff/180001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/497563002/diff/180001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:106: Profile *profile = ProfileAndroid::FromProfileAndroid(j_profile); nit: Profile* profile (I guess this is consistent with other calls but inconsistent with other calls I've seen). Please update others
https://codereview.chromium.org/497563002/diff/180001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/497563002/diff/180001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:106: Profile *profile = ProfileAndroid::FromProfileAndroid(j_profile); On 2014/09/03 01:11:25, Yaron wrote: > nit: Profile* profile > (I guess this is consistent with other calls but inconsistent with other calls > I've seen). Please update others Done.
https://codereview.chromium.org/497563002/diff/40001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/497563002/diff/40001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2025: bool SkipConditionalExperiment(const Experiment& experiment) { I confirmed with Mark and Yefim. If user is not in the experiment group, by finch or Chrome sync, the flag doesn't do anything so there is no reason to show to the user.
kkimlabs@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/browser/bookmarks/enhanced_bookmarks_features.*
LGTM https://codereview.chromium.org/497563002/diff/200001/chrome/browser/bookmark... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/200001/chrome/browser/bookmark... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:201: || bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED_FROM_FINCH; nit: I believe style guide says || should be on previous line.
https://codereview.chromium.org/497563002/diff/200001/chrome/browser/bookmark... File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right): https://codereview.chromium.org/497563002/diff/200001/chrome/browser/bookmark... chrome/browser/bookmarks/enhanced_bookmarks_features.cc:201: || bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED_FROM_FINCH; On 2014/09/03 21:26:20, sky wrote: > nit: I believe style guide says || should be on previous line. Done.
about_flags lgtm
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/497563002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/497563002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/497563002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/497563002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 440f8036ea7fe437976e64e6685716ef0ef2f093
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/eedd8b84de9b16ad2af273dbf1f33e3971290565 Cr-Commit-Position: refs/heads/master@{#293509} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
