Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
965603002:
Creates reusable button for usage in Smart Lock infobars. (Closed)
DescriptionCreates reusable button for usage in Smart Lock infobars.
Button with text "More", which opens a popup with two elements: "Learn more" link to help center and link named "Settings" which opens general settings page.
This element is used by Smart Lock infobars.
BUG=454815
Committed: https://crrev.com/244e6e8bf8e7ea061047664a597a319f0f5a79e4
Cr-Commit-Position: refs/heads/master@{#319444}
Patch Set 1 #Patch Set 2 : #
Total comments: 23
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Messages
Total messages: 21 (7 generated)
melandory@chromium.org changed reviewers: + bauerb@chromium.org
Hi, review this Cl please. Thanks in advance!
https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:141: layout.setCustomViewInButtonRow(MoreSelector.createMoreSelector(getContext())); I think a slightly more common expression for this would be "overflow" instead of "more"... WDYT? https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:19: /** Button with text More, which opens a popup with two elements: Learn more link to help center and Start Javadoc on the next line if the comment is multiline. Also, put "More" in quotation marks? https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:20: * link to settings names Settings. Just "link to settings"? https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:22: public class MoreSelector { Add a private empty constructor to prevent this class from being instantiated. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:24: public static Button createMoreSelector(Context context) { Add a Javadoc comment please. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:36: /** Pops up menu with two items: Setting and Learn More when user clicks More button. */ "Settings" https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:39: final Context context = v.getContext(); If you move this one line up, you can use the context in the PopupMenu constructor. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:46: popup.inflate(R.menu.account_chooser_infobar_more_menu_popup); This resource seems to be specific to the account chooser. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:52: PreferencesLauncher.launchSettingsPage(context, null); I think this would be more useful if it opened a specific settings fragment... https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:55: // TODO(melandory): Learn more should open link to help center FYI: Often you can get placeholder links that you can put into the code right now, where the actual content can be put later online. I'd get into touch with the HC folks about that.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:141: layout.setCustomViewInButtonRow(MoreSelector.createMoreSelector(getContext())); On 2015/02/27 13:39:13, Bernhard Bauer wrote: > I think a slightly more common expression for this would be "overflow" instead > of "more"... WDYT? You meant to change MoreSelector to OverflowSelector, right? https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:19: /** Button with text More, which opens a popup with two elements: Learn more link to help center and On 2015/02/27 13:39:14, Bernhard Bauer wrote: > Start Javadoc on the next line if the comment is multiline. Also, put "More" in > quotation marks? Done. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:20: * link to settings names Settings. On 2015/02/27 13:39:14, Bernhard Bauer wrote: > Just "link to settings"? Tried to express it more clear. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:22: public class MoreSelector { On 2015/02/27 13:39:14, Bernhard Bauer wrote: > Add a private empty constructor to prevent this class from being instantiated. Done. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:24: public static Button createMoreSelector(Context context) { On 2015/02/27 13:39:13, Bernhard Bauer wrote: > Add a Javadoc comment please. Done. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:36: /** Pops up menu with two items: Setting and Learn More when user clicks More button. */ On 2015/02/27 13:39:14, Bernhard Bauer wrote: > "Settings" Done. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:39: final Context context = v.getContext(); On 2015/02/27 13:39:14, Bernhard Bauer wrote: > If you move this one line up, you can use the context in the PopupMenu > constructor. Done. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:46: popup.inflate(R.menu.account_chooser_infobar_more_menu_popup); On 2015/02/27 13:39:14, Bernhard Bauer wrote: > This resource seems to be specific to the account chooser. Done. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:52: PreferencesLauncher.launchSettingsPage(context, null); On 2015/02/27 13:39:13, Bernhard Bauer wrote: > I think this would be more useful if it opened a specific settings fragment... Agree, but this fragment not exists yet. I'll add TODO, in order to not forget to change it in a future. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:55: // TODO(melandory): Learn more should open link to help center On 2015/02/27 13:39:14, Bernhard Bauer wrote: > FYI: Often you can get placeholder links that you can put into the code right > now, where the actual content can be put later online. I'd get into touch with > the HC folks about that. sabineb is on this issue already :)
https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:141: layout.setCustomViewInButtonRow(MoreSelector.createMoreSelector(getContext())); On 2015/02/27 14:45:30, melandory wrote: > On 2015/02/27 13:39:13, Bernhard Bauer wrote: > > I think a slightly more common expression for this would be "overflow" instead > > of "more"... WDYT? > > You meant to change MoreSelector to OverflowSelector, right? Yup, I think that would be better. https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:52: PreferencesLauncher.launchSettingsPage(context, null); On 2015/02/27 14:45:30, melandory wrote: > On 2015/02/27 13:39:13, Bernhard Bauer wrote: > > I think this would be more useful if it opened a specific settings fragment... > > Agree, but this fragment not exists yet. I'll add TODO, in order to not forget > to change it in a future. Thanks! One thing you could think about already is how a client of this class would specify the fragment, and the help center URL (which I'm assuming the client would need to do, as otherwise this class wouldn't really be reusable). For example, you could make this class instantiatable and store these properties in there. https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java (right): https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:22: * link named "Settings" which opens general settings page. Merge these two lines. https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:53: popup.inflate(R.menu.more_menu_popup_for_smart_lock_infobars); So, what's the extent to which this is meant to be reusable? For smart lock infobars, as a generic UI element, ...? It might be worth mentioning that in e.g. the Javadoc comment (and maybe moving it to a directory for smart lock code, if such a thing exists).
https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/965603002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:141: layout.setCustomViewInButtonRow(MoreSelector.createMoreSelector(getContext())); On 2015/02/27 15:02:15, Bernhard Bauer wrote: > On 2015/02/27 14:45:30, melandory wrote: > > On 2015/02/27 13:39:13, Bernhard Bauer wrote: > > > I think a slightly more common expression for this would be "overflow" > instead > > > of "more"... WDYT? > > > > You meant to change MoreSelector to OverflowSelector, right? > > Yup, I think that would be better. Done. https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java (right): https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:22: * link named "Settings" which opens general settings page. On 2015/02/27 15:02:16, Bernhard Bauer wrote: > Merge these two lines. Done. https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:53: popup.inflate(R.menu.more_menu_popup_for_smart_lock_infobars); On 2015/02/27 15:02:16, Bernhard Bauer wrote: > So, what's the extent to which this is meant to be reusable? > For smart lock infobars, as a generic UI element, ...? Yes. > It might be worth mentioning that in > e.g. the Javadoc comment (and maybe moving it to a directory for smart lock > code, if such a thing exists). Nope such directory doesn't exists. AccountChooserInfoBar for now in the only example of SmartLock infobars. One more is on its way.
Patchset #4 (id:100001) has been deleted
Code-wise this LGTM, but can you update the description (and subject, which should match the first line of the description) please? It should mention what the button does and where it's used (and that this is for Android, if that's not obvious otherwise). https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java (right): https://codereview.chromium.org/965603002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/MoreSelector.java:53: popup.inflate(R.menu.more_menu_popup_for_smart_lock_infobars); On 2015/02/27 15:42:49, melandory wrote: > On 2015/02/27 15:02:16, Bernhard Bauer wrote: > > So, what's the extent to which this is meant to be reusable? > > For smart lock infobars, as a generic UI element, ...? > Yes. > > It might be worth mentioning that in > > e.g. the Javadoc comment (and maybe moving it to a directory for smart lock > > code, if such a thing exists). > > Nope such directory doesn't exists. AccountChooserInfoBar for now in the only > example of SmartLock infobars. One more is on its way. OK. Once there are more smart lock infobars I think it definitely would make sense to move them to their own directory.
melandory@chromium.org changed reviewers: + yusufo@chromium.org
yusufo@chromium.org: Please review changes in chrome/android/java/res/menu/account_chooser_infobar_more_menu_popup.xml chrome/android/java/res/menu/overflow_selector_for_smart_lock_infobars.xml Thanks in advance.
melandory@chromium.org changed reviewers: + dfalcantara@chromium.org - yusufo@chromium.org
On 2015/03/01 18:59:12, melandory wrote: > mailto:yusufo@chromium.org: Please review changes in > > chrome/android/java/res/menu/account_chooser_infobar_more_menu_popup.xml > chrome/android/java/res/menu/overflow_selector_for_smart_lock_infobars.xml > > Thanks in advance. dfalcantara@ maybe you can look at this renaming: chrome/android/java/res/menu/account_chooser_infobar_more_menu_popup.xml chrome/android/java/res/menu/overflow_selector_for_smart_lock_infobars.xml
On 2015/03/01 18:59:12, melandory wrote: > mailto:yusufo@chromium.org: Please review changes in > > chrome/android/java/res/menu/account_chooser_infobar_more_menu_popup.xml > chrome/android/java/res/menu/overflow_selector_for_smart_lock_infobars.xml > > Thanks in advance. dfalcantara@ maybe you can look at this renaming: chrome/android/java/res/menu/account_chooser_infobar_more_menu_popup.xml chrome/android/java/res/menu/overflow_selector_for_smart_lock_infobars.xml
lgtm
Fixed a typo in your commit message.
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/965603002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/244e6e8bf8e7ea061047664a597a319f0f5a79e4 Cr-Commit-Position: refs/heads/master@{#319444} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
