Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018593002:
Use onOptionsItemSelected in Physical Web activity (Closed)
DescriptionUse onOptionsItemSelected in Physical Web activity
Using onOptionsItemsSelected will let us handle menu items click
more nicely without a bunch of anonymous classes.
BUG=603132
Committed: https://crrev.com/7c87ec1b2118585a557696b4f963df0f5d975c44
Cr-Commit-Position: refs/heads/master@{#396979}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Explicitly return true in onOptionsItemSelected #Patch Set 3 : Remove dead stores #
Total comments: 2
Messages
Total messages: 26 (11 generated)
cco3@chromium.org changed reviewers: + nyquist@chromium.org, tedchoc@chromium.org
lgtm!
The CQ bit was checked by cco3@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018593002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018593002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
cco3@chromium.org changed reviewers: + dfalcantara@chromium.org
Need an LGTM on ids.xml
https://codereview.chromium.org/2018593002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2018593002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:158: finish(); These should be returning true so the parent doesn't try do something with the menu item you clicked on. https://codereview.chromium.org/2018593002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:166: Log.e(TAG, "Unkown menu item selected"); nit: unknown.
https://codereview.chromium.org/2018593002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2018593002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:158: finish(); On 2016/05/27 17:53:45, dfalcantara wrote: > These should be returning true so the parent doesn't try do something with the > menu item you clicked on. Done. https://codereview.chromium.org/2018593002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:166: Log.e(TAG, "Unkown menu item selected"); On 2016/05/27 17:53:45, dfalcantara wrote: > nit: unknown. Done.
lgtm
lgtm
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2018593002/#ps20001 (title: "Explicitly return true in onOptionsItemSelected")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018593002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018593002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, nyquist@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2018593002/#ps40001 (title: "Remove dead stores")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018593002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018593002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use onOptionsItemSelected in Physical Web activity Using onOptionsItemsSelected will let us handle menu items click more nicely without a bunch of anonymous classes. BUG=603132 ========== to ========== Use onOptionsItemSelected in Physical Web activity Using onOptionsItemsSelected will let us handle menu items click more nicely without a bunch of anonymous classes. BUG=603132 Committed: https://crrev.com/7c87ec1b2118585a557696b4f963df0f5d975c44 Cr-Commit-Position: refs/heads/master@{#396979} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7c87ec1b2118585a557696b4f963df0f5d975c44 Cr-Commit-Position: refs/heads/master@{#396979}
Message was sent while issue was closed.
mattreynolds@chromium.org changed reviewers: + mattreynolds@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2018593002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/2018593002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:137: menu.add(0, R.id.menu_id_refresh, 0, R.string.physical_web_refresh) The order param treats 0 as a special value (Menu.NONE means "don't care about the order"); we should use something else if we do care about the order. https://codereview.chromium.org/2018593002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:141: menu.add(0, R.id.menu_id_close, 1, R.string.close) Do we want to make this the rightmost button? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
