Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2003023003:
Add a settings button to Physical Web activity (Closed)
DescriptionAdd a settings button to Physical Web activity
Users currently don't have an easy way to get to the Physical Web
settings apart from navigating through the privacy preferences. This
change gives them an easy way directly from the ListUrlsActivity.
BUG=603132
Committed: https://crrev.com/72065e5edc05138c0e0a5cb95bc573a6bf93909b
Cr-Commit-Position: refs/heads/master@{#396068}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use IDS_MENU_PREFRENCES #
Total comments: 2
Patch Set 3 : Make closeItem close #
Total comments: 2
Patch Set 4 : Use setOnMenuItemClickListener #Messages
Total messages: 22 (6 generated)
cco3@chromium.org changed reviewers: + mattreynolds@chromium.org
https://codereview.chromium.org/2003023003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2003023003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:151: Reset Settings We could also use IDS_MENU_PREFERENCES instead of adding a new string
On 2016/05/23 19:45:59, mattreynolds wrote: > https://codereview.chromium.org/2003023003/diff/1/chrome/android/java/strings... > File chrome/android/java/strings/android_chrome_strings.grd (right): > > https://codereview.chromium.org/2003023003/diff/1/chrome/android/java/strings... > chrome/android/java/strings/android_chrome_strings.grd:151: Reset > Settings > > We could also use IDS_MENU_PREFERENCES instead of adding a new string Done.
lgtm
cco3@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@chromium.org changed reviewers: + tedchoc@chromium.org
conceptually this sounds useful, but given that this is a visual change to how users perceive chrome and this feature in particular, I was looking for the mocks and the UX team approval, but couldn't seem to find it. Could you include a link to it in the bug? including tedchoc@ in case he has any thoughts. https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:151: refreshItem.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener() { It this supposed to be |closeItem|? It looks to me like it will end up finish()-ing the activity.
Sorry, will get mocks. https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:151: refreshItem.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener() { On 2016/05/23 21:09:35, nyquist wrote: > It this supposed to be |closeItem|? It looks to me like it will end up > finish()-ing the activity. Done.
On 2016/05/23 21:22:18, cco3 wrote: > Sorry, will get mocks. Even if you don't have designer mocks, it is really helpful to just upload screenshots to the bug for UI related changes. > > https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java > (right): > > https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:151: > refreshItem.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener() { > On 2016/05/23 21:09:35, nyquist wrote: > > It this supposed to be |closeItem|? It looks to me like it will end up > > finish()-ing the activity. > > Done.
On 2016/05/23 21:40:05, Ted C wrote: > On 2016/05/23 21:22:18, cco3 wrote: > > Sorry, will get mocks. > > Even if you don't have designer mocks, it is really helpful to just > upload screenshots to the bug for UI related changes. Photo added to bug. > > > > > > https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java > > (right): > > > > > https://codereview.chromium.org/2003023003/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:151: > > refreshItem.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener() > { > > On 2016/05/23 21:09:35, nyquist wrote: > > > It this supposed to be |closeItem|? It looks to me like it will end up > > > finish()-ing the activity. > > > > Done.
lgtm, but please let tedchoc@ also have his say now that the screenshot is available.
https://codereview.chromium.org/2003023003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2003023003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:161: mSettingsButton = menu.add(R.string.menu_preferences) Why use onOptionsItemSelected for this and setOnMenuItemClickListener? (I like the former as it doesn't require a bunch more anonymous inner classes) Also, I would consider specifying an ID (specified in ids.xml) instead of keeping a reference to the MenuItem. Then use one of the more complicated Menu#add functions.
https://codereview.chromium.org/2003023003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2003023003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:161: mSettingsButton = menu.add(R.string.menu_preferences) On 2016/05/25 22:09:46, Ted C wrote: > Why use onOptionsItemSelected for this and setOnMenuItemClickListener? (I like > the former as it doesn't require a bunch more anonymous inner classes) > > Also, I would consider specifying an ID (specified in ids.xml) instead of > keeping a reference to the MenuItem. Then use one of the more complicated > Menu#add functions. Done.
lgtm
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2003023003/#ps60001 (title: "Use setOnMenuItemClickListener")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003023003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003023003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a settings button to Physical Web activity Users currently don't have an easy way to get to the Physical Web settings apart from navigating through the privacy preferences. This change gives them an easy way directly from the ListUrlsActivity. BUG=603132 ========== to ========== Add a settings button to Physical Web activity Users currently don't have an easy way to get to the Physical Web settings apart from navigating through the privacy preferences. This change gives them an easy way directly from the ListUrlsActivity. BUG=603132 Committed: https://crrev.com/72065e5edc05138c0e0a5cb95bc573a6bf93909b Cr-Commit-Position: refs/heads/master@{#396068} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/72065e5edc05138c0e0a5cb95bc573a6bf93909b Cr-Commit-Position: refs/heads/master@{#396068} |
