Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1968283002:
Implementing Host List Context Menu and Delete Feature (Closed)
DescriptionImplementing Host List Context Menu and Delete Feature
* Implements host list context menu triggered by long-pressing the host item.
* Implements host deletion functionality.
BUG=607216
Committed: https://crrev.com/7bc6cf4ed459a86f9fa65e1dfbc098d101a81131
Cr-Commit-Position: refs/heads/master@{#393714}
Patch Set 1 : #
Total comments: 19
Patch Set 2 : UI Only & Reviewer's Feedback #
Total comments: 12
Patch Set 3 : Reviewer's Feedback #
Messages
Total messages: 46 (19 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968283002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968283002/20001
yuweih@chromium.org changed reviewers: + lambroslambrou@chromium.org
ptal https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a specific paired client." name="IDS_DELETE_PAIRED_CLIENT" formatter_data="android_java"> I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... Is it a good idea to copy the message and name it something like IDS_DELETE_HOST?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
I'm not sure whether implementing queuing is necessary but I think I should somehow factor out the OAuth fetching code...
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968283002/40001
I haven't looked at patch #2 yet. I notice that this CL seems to be more complex because of the mTriedNewAuthToken logic, which is no longer needed. As long as you fetch an OAuth token using GoogleAuthUtil and use the returned token immediately, you don't have to worry about testing and retrying it. Would it make sense to first create a separate CL to remove this logic, then re-do this CL on top of it? https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:412: private int getHostIndexForMenu(ContextMenu.ContextMenuInfo menuInfo) { static https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:418: ContextMenu.ContextMenuInfo menuInfo) { nit: Java line-continuation is +8-space indent. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:429: int i = item.getItemId(); nit: 'id' instead of 'i'. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:431: onHostClicked(getHostIndexForMenu(item.getMenuInfo())); Maybe pull out the index? int hostIndex = getHostIndexForMenu(item.getMenuInfo()); https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:454: /** Called to initialize the action bar. */ nit: indentation https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:581: final OAuthTokenFetcher.Callback callback) { nit: line-continuation is +8 space. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:632: mTriedNewAuthToken = false; I think we can get rid of the retry-if-auth-token-expired logic. The old AccountsManager API would often return invalid OAuth tokens which the application needed to test and refresh explicitly. The new GoogleAuthUtil class doesn't have this problem - it always hands us good tokens (it refreshes automatically if expired). Don't need to worry about it for this CL. https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... remoting/resources/remoting_strings.grd:711: <message desc="Confirmation prompt shown when a user attempts to delete a Me2Me host from their host list." name="IDS_CONFIRM_HOST_DELETE_ANDROID" formatter_data="android_java"> Translators might not understand "Me2e" or even "host". The only context they have is the English text and the description - they might not be familiar with our technology. Maybe phrase this as "...attempts to remove or deactivate a machine from the list of their remotely-accessible machines" ? https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a specific paired client." name="IDS_DELETE_PAIRED_CLIENT" formatter_data="android_java"> On 2016/05/11 23:43:59, Yuwei wrote: > I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... Is it > a good idea to copy the message and name it something like IDS_DELETE_HOST? Given the label is just "Delete", we should probably add a new generic "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. Note that if two messages have the same text, they will not be translated separately unless they have a "meaning" attribute (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). So you might want to add meaning="Generic Delete button" to a new IDS_DELETE message, in case the other "Delete" message was somehow translated specifically for paired clients (though it seems unlikely - translators probably wouldn't know what paired clients are anyway :) Jamie, WDYT?
lambroslambrou@chromium.org changed reviewers: + jamiewalch@chromium.org
Adding Jamie for thoughts about remoting_strings.grd.
Patchset #2 (id:40001) has been deleted
On 2016/05/12 23:42:14, Lambros wrote: > I haven't looked at patch #2 yet. > I notice that this CL seems to be more complex because of the mTriedNewAuthToken > logic, which is no longer needed. > As long as you fetch an OAuth token using GoogleAuthUtil and use the returned > token immediately, you don't have to worry about testing and retrying it. > Would it make sense to first create a separate CL to remove this logic, then > re-do this CL on top of it? > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:412: private > int getHostIndexForMenu(ContextMenu.ContextMenuInfo menuInfo) { > static > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:418: > ContextMenu.ContextMenuInfo menuInfo) { > nit: Java line-continuation is +8-space indent. > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:429: int i = > item.getItemId(); > nit: 'id' instead of 'i'. > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:431: > onHostClicked(getHostIndexForMenu(item.getMenuInfo())); > Maybe pull out the index? > int hostIndex = getHostIndexForMenu(item.getMenuInfo()); > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:454: /** > Called to initialize the action bar. */ > nit: indentation > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:581: final > OAuthTokenFetcher.Callback callback) { > nit: line-continuation is +8 space. > > https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:632: > mTriedNewAuthToken = false; > I think we can get rid of the retry-if-auth-token-expired logic. The old > AccountsManager API would often return invalid OAuth tokens which the > application needed to test and refresh explicitly. The new GoogleAuthUtil class > doesn't have this problem - it always hands us good tokens (it refreshes > automatically if expired). > > Don't need to worry about it for this CL. > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > File remoting/resources/remoting_strings.grd (right): > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > remoting/resources/remoting_strings.grd:711: <message desc="Confirmation prompt > shown when a user attempts to delete a Me2Me host from their host list." > name="IDS_CONFIRM_HOST_DELETE_ANDROID" formatter_data="android_java"> > Translators might not understand "Me2e" or even "host". The only context they > have is the English text and the description - they might not be familiar with > our technology. > Maybe phrase this as > "...attempts to remove or deactivate a machine from the list of their > remotely-accessible machines" ? > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a > specific paired client." name="IDS_DELETE_PAIRED_CLIENT" > formatter_data="android_java"> > On 2016/05/11 23:43:59, Yuwei wrote: > > I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... Is > it > > a good idea to copy the message and name it something like IDS_DELETE_HOST? > > Given the label is just "Delete", we should probably add a new generic > "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. > > Note that if two messages have the same text, they will not be translated > separately unless they have a "meaning" attribute > (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). > > So you might want to add meaning="Generic Delete button" to a new IDS_DELETE > message, in case the other "Delete" message was somehow translated specifically > for paired clients (though it seems unlikely - translators probably wouldn't > know what paired clients are anyway :) Jamie, WDYT? It does become a complicated CL. I will make this CL for UI only and make a separate CL for the OAuth token stuff.
https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a specific paired client." name="IDS_DELETE_PAIRED_CLIENT" formatter_data="android_java"> On 2016/05/12 23:42:13, Lambros wrote: > On 2016/05/11 23:43:59, Yuwei wrote: > > I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... Is > it > > a good idea to copy the message and name it something like IDS_DELETE_HOST? > > Given the label is just "Delete", we should probably add a new generic > "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. > > Note that if two messages have the same text, they will not be translated > separately unless they have a "meaning" attribute > (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). > > So you might want to add meaning="Generic Delete button" to a new IDS_DELETE > message, in case the other "Delete" message was somehow translated specifically > for paired clients (though it seems unlikely - translators probably wouldn't > know what paired clients are anyway :) Jamie, WDYT? > This is a pitfall of short strings: it's tough to know whether or not two concepts which use the same string in English will be the same in all languages. In this case, I think it's the same sense of the word "Delete" in both contexts, so using the same string is fine, but updating the description is probably a good idea.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
On 2016/05/13 00:45:31, Jamie wrote: > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > File remoting/resources/remoting_strings.grd (right): > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a > specific paired client." name="IDS_DELETE_PAIRED_CLIENT" > formatter_data="android_java"> > On 2016/05/12 23:42:13, Lambros wrote: > > On 2016/05/11 23:43:59, Yuwei wrote: > > > I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... > Is > > it > > > a good idea to copy the message and name it something like IDS_DELETE_HOST? > > > > Given the label is just "Delete", we should probably add a new generic > > "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. > > > > Note that if two messages have the same text, they will not be translated > > separately unless they have a "meaning" attribute > > > (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). > > > > So you might want to add meaning="Generic Delete button" to a new IDS_DELETE > > message, in case the other "Delete" message was somehow translated > specifically > > for paired clients (though it seems unlikely - translators probably wouldn't > > know what paired clients are anyway :) Jamie, WDYT? > > > > This is a pitfall of short strings: it's tough to know whether or not two > concepts which use the same string in English will be the same in all languages. > In this case, I think it's the same sense of the word "Delete" in both contexts, > so using the same string is fine, but updating the description is probably a > good idea. Maybe something like "Delete an item from a list"?
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a specific paired client." name="IDS_DELETE_PAIRED_CLIENT" formatter_data="android_java"> On 2016/05/13 00:45:31, Jamie wrote: > On 2016/05/12 23:42:13, Lambros wrote: > > On 2016/05/11 23:43:59, Yuwei wrote: > > > I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... > Is > > it > > > a good idea to copy the message and name it something like IDS_DELETE_HOST? > > > > Given the label is just "Delete", we should probably add a new generic > > "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. > > > > Note that if two messages have the same text, they will not be translated > > separately unless they have a "meaning" attribute > > > (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). > > > > So you might want to add meaning="Generic Delete button" to a new IDS_DELETE > > message, in case the other "Delete" message was somehow translated > specifically > > for paired clients (though it seems unlikely - translators probably wouldn't > > know what paired clients are anyway :) Jamie, WDYT? > > > > This is a pitfall of short strings: it's tough to know whether or not two > concepts which use the same string in English will be the same in all languages. > In this case, I think it's the same sense of the word "Delete" in both contexts, > so using the same string is fine, but updating the description is probably a > good idea. I agree that there are cases when it may be useful to have different strings for the same English strings used in different contexts. But unfortunately GRIT won't handle this properly. It hashes strings by their English variants and so 'Delete' can be translated only once in each language.
On 2016/05/13 17:35:24, Sergey Ulanov wrote: > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > File remoting/resources/remoting_strings.grd (right): > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a > specific paired client." name="IDS_DELETE_PAIRED_CLIENT" > formatter_data="android_java"> > On 2016/05/13 00:45:31, Jamie wrote: > > On 2016/05/12 23:42:13, Lambros wrote: > > > On 2016/05/11 23:43:59, Yuwei wrote: > > > > I may be misusing the message given the name "IDS_DELETE_PAIRED_CLIENT"... > > Is > > > it > > > > a good idea to copy the message and name it something like > IDS_DELETE_HOST? > > > > > > Given the label is just "Delete", we should probably add a new generic > > > "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. > > > > > > Note that if two messages have the same text, they will not be translated > > > separately unless they have a "meaning" attribute > > > > > > (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). > > > > > > So you might want to add meaning="Generic Delete button" to a new IDS_DELETE > > > message, in case the other "Delete" message was somehow translated > > specifically > > > for paired clients (though it seems unlikely - translators probably wouldn't > > > know what paired clients are anyway :) Jamie, WDYT? > > > > > > > This is a pitfall of short strings: it's tough to know whether or not two > > concepts which use the same string in English will be the same in all > languages. > > In this case, I think it's the same sense of the word "Delete" in both > contexts, > > so using the same string is fine, but updating the description is probably a > > good idea. > > I agree that there are cases when it may be useful to have different strings for > the same English strings used in different contexts. But unfortunately GRIT > won't handle this properly. It hashes strings by their English variants and so > 'Delete' can be translated only once in each language. So the meaning attribute will not make any difference when handling the string? That sounds bad... I used to see Windows Store translated "play" as for playing games into "play" as for playing music...
On 2016/05/13 18:01:43, Yuwei wrote: > On 2016/05/13 17:35:24, Sergey Ulanov wrote: > > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > > File remoting/resources/remoting_strings.grd (right): > > > > > https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... > > remoting/resources/remoting_strings.grd:1119: <message desc="Link to delete a > > specific paired client." name="IDS_DELETE_PAIRED_CLIENT" > > formatter_data="android_java"> > > On 2016/05/13 00:45:31, Jamie wrote: > > > On 2016/05/12 23:42:13, Lambros wrote: > > > > On 2016/05/11 23:43:59, Yuwei wrote: > > > > > I may be misusing the message given the name > "IDS_DELETE_PAIRED_CLIENT"... > > > Is > > > > it > > > > > a good idea to copy the message and name it something like > > IDS_DELETE_HOST? > > > > > > > > Given the label is just "Delete", we should probably add a new generic > > > > "IDS_DELETE" string and deprecate IDS_DELETE_PAIRED_CLIENT. > > > > > > > > Note that if two messages have the same text, they will not be translated > > > > separately unless they have a "meaning" attribute > > > > > > > > > > (https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-...). > > > > > > > > So you might want to add meaning="Generic Delete button" to a new > IDS_DELETE > > > > message, in case the other "Delete" message was somehow translated > > > specifically > > > > for paired clients (though it seems unlikely - translators probably > wouldn't > > > > know what paired clients are anyway :) Jamie, WDYT? > > > > > > > > > > This is a pitfall of short strings: it's tough to know whether or not two > > > concepts which use the same string in English will be the same in all > > languages. > > > In this case, I think it's the same sense of the word "Delete" in both > > contexts, > > > so using the same string is fine, but updating the description is probably a > > > good idea. > > > > I agree that there are cases when it may be useful to have different strings > for > > the same English strings used in different contexts. But unfortunately GRIT > > won't handle this properly. It hashes strings by their English variants and so > > 'Delete' can be translated only once in each language. > > So the meaning attribute will not make any difference when handling the string? > That sounds bad... I used to see Windows Store translated "play" as for playing > games into "play" as for playing music... The "meaning" attribute is supported by Grit for dealing with the same English string having different meanings in different contexts. I linked the Grit docs earlier - see also: https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit/ex...
On 2016/05/13 18:06:32, Lambros wrote: > The "meaning" attribute is supported by Grit for dealing with the same English > string having different meanings in different contexts. > I linked the Grit docs earlier - see also: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit/ex... Yes, sorry, my bad
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968283002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968283002/80001
(new) patch#2 is uploaded. This is after committing CL 1976853002. It may be easier to show diff to the master than to patch#1. PTAL https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:412: private int getHostIndexForMenu(ContextMenu.ContextMenuInfo menuInfo) { On 2016/05/12 23:42:13, Lambros wrote: > static Done. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:418: ContextMenu.ContextMenuInfo menuInfo) { On 2016/05/12 23:42:13, Lambros wrote: > nit: Java line-continuation is +8-space indent. Done. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:429: int i = item.getItemId(); On 2016/05/12 23:42:13, Lambros wrote: > nit: 'id' instead of 'i'. Done. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:431: onHostClicked(getHostIndexForMenu(item.getMenuInfo())); On 2016/05/12 23:42:13, Lambros wrote: > Maybe pull out the index? > int hostIndex = getHostIndexForMenu(item.getMenuInfo()); > Done. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:454: /** Called to initialize the action bar. */ On 2016/05/12 23:42:13, Lambros wrote: > nit: indentation Done. https://codereview.chromium.org/1968283002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:632: mTriedNewAuthToken = false; On 2016/05/12 23:42:13, Lambros wrote: > I think we can get rid of the retry-if-auth-token-expired logic. The old > AccountsManager API would often return invalid OAuth tokens which the > application needed to test and refresh explicitly. The new GoogleAuthUtil class > doesn't have this problem - it always hands us good tokens (it refreshes > automatically if expired). > > Don't need to worry about it for this CL. Done. https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/20001/remoting/resources/remo... remoting/resources/remoting_strings.grd:711: <message desc="Confirmation prompt shown when a user attempts to delete a Me2Me host from their host list." name="IDS_CONFIRM_HOST_DELETE_ANDROID" formatter_data="android_java"> On 2016/05/12 23:42:13, Lambros wrote: > Translators might not understand "Me2e" or even "host". The only context they > have is the English text and the description - they might not be familiar with > our technology. > Maybe phrase this as > "...attempts to remove or deactivate a machine from the list of their > remotely-accessible machines" ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... File remoting/android/java/res/menu/options_actionbar.xml (right): https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... remoting/android/java/res/menu/options_actionbar.xml:1: <?xml version="1.0" encoding="utf-8"?> Is this related to any action-bar? Maybe name this host_options.xml or host_context_menu.xml instead? https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... remoting/android/java/res/menu/options_actionbar.xml:10: <item android:id="@+id/actionbar_connect" host_menu_connect? (I think just 'connect' might be OK, I don't think it matters about name-collisions with other files?) https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... remoting/android/java/res/menu/options_actionbar.xml:12: app:showAsAction="never"/> Do we need to specify showAsAction for context-menu items? https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:422: HostInfo hostInfo = mHosts[hostIndex]; I would suggest factoring out this block into a separate method (probably taking a HostInfo parameter), to reduce indentation and keep this method short. https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:424: AlertDialog.Builder builder = new AlertDialog.Builder(this); Maybe remove |builder| temporary and do: new AlertDialog.Builder(this) .setMessage(...) .setPositive(...) ... https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... remoting/resources/remoting_strings.grd:1119: <message desc="Delete an item from a list." meaning="Delete an item from a list." name="IDS_DELETE_LIST_ITEM" formatter_data="android_java"> Maybe mention that this text appears on a button or a menu item, so translators keep it short. Something like: "This text is used on a button or context-menu or similar control. When chosen or pressed, it deletes a selected item from a list."
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968283002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968283002/100001
https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... File remoting/android/java/res/menu/options_actionbar.xml (right): https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... remoting/android/java/res/menu/options_actionbar.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/05/13 21:17:10, Lambros wrote: > Is this related to any action-bar? Maybe name this > host_options.xml or host_context_menu.xml instead? Done. Renamed to host_context_menu.xml https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... remoting/android/java/res/menu/options_actionbar.xml:10: <item android:id="@+id/actionbar_connect" On 2016/05/13 21:17:10, Lambros wrote: > host_menu_connect? (I think just 'connect' might be OK, I don't think it matters > about name-collisions with other files?) Renamed to connect and delete. The only concern is if later we need different connect and delete context menu item for different purpose then we will need to rename something. We can think about this when we get to this point... https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... remoting/android/java/res/menu/options_actionbar.xml:12: app:showAsAction="never"/> On 2016/05/13 21:17:10, Lambros wrote: > Do we need to specify showAsAction for context-menu items? Removed. https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:422: HostInfo hostInfo = mHosts[hostIndex]; On 2016/05/13 21:17:11, Lambros wrote: > I would suggest factoring out this block into a separate method (probably taking > a HostInfo parameter), to reduce indentation and keep this method short. Done. https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:424: AlertDialog.Builder builder = new AlertDialog.Builder(this); On 2016/05/13 21:17:11, Lambros wrote: > Maybe remove |builder| temporary and do: > > new AlertDialog.Builder(this) > .setMessage(...) > .setPositive(...) > ... Done. https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... remoting/resources/remoting_strings.grd:1119: <message desc="Delete an item from a list." meaning="Delete an item from a list." name="IDS_DELETE_LIST_ITEM" formatter_data="android_java"> On 2016/05/13 21:17:11, Lambros wrote: > Maybe mention that this text appears on a button or a menu item, so translators > keep it short. Something like: > > "This text is used on a button or context-menu or similar control. When chosen > or pressed, it deletes a selected item from a list." Done.
On 2016/05/13 23:43:54, Yuwei wrote: > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > File remoting/android/java/res/menu/options_actionbar.xml (right): > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > remoting/android/java/res/menu/options_actionbar.xml:1: <?xml version="1.0" > encoding="utf-8"?> > On 2016/05/13 21:17:10, Lambros wrote: > > Is this related to any action-bar? Maybe name this > > host_options.xml or host_context_menu.xml instead? > > Done. Renamed to host_context_menu.xml > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > remoting/android/java/res/menu/options_actionbar.xml:10: <item > android:id="@+id/actionbar_connect" > On 2016/05/13 21:17:10, Lambros wrote: > > host_menu_connect? (I think just 'connect' might be OK, I don't think it > matters > > about name-collisions with other files?) > > Renamed to connect and delete. The only concern is if later we need different > connect and delete context menu item for different purpose then we will need to > rename something. We can think about this when we get to this point... > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > remoting/android/java/res/menu/options_actionbar.xml:12: > app:showAsAction="never"/> > On 2016/05/13 21:17:10, Lambros wrote: > > Do we need to specify showAsAction for context-menu items? > > Removed. > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:422: HostInfo > hostInfo = mHosts[hostIndex]; > On 2016/05/13 21:17:11, Lambros wrote: > > I would suggest factoring out this block into a separate method (probably > taking > > a HostInfo parameter), to reduce indentation and keep this method short. > > Done. > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:424: > AlertDialog.Builder builder = new AlertDialog.Builder(this); > On 2016/05/13 21:17:11, Lambros wrote: > > Maybe remove |builder| temporary and do: > > > > new AlertDialog.Builder(this) > > .setMessage(...) > > .setPositive(...) > > ... > > Done. > > https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... > File remoting/resources/remoting_strings.grd (right): > > https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... > remoting/resources/remoting_strings.grd:1119: <message desc="Delete an item from > a list." meaning="Delete an item from a list." name="IDS_DELETE_LIST_ITEM" > formatter_data="android_java"> > On 2016/05/13 21:17:11, Lambros wrote: > > Maybe mention that this text appears on a button or a menu item, so > translators > > keep it short. Something like: > > > > "This text is used on a button or context-menu or similar control. When chosen > > or pressed, it deletes a selected item from a list." > > Done. sergeyu@ jamiewalch@ any last minute comment?
On 2016/05/13 23:52:54, Yuwei wrote: > On 2016/05/13 23:43:54, Yuwei wrote: > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > > File remoting/android/java/res/menu/options_actionbar.xml (right): > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > > remoting/android/java/res/menu/options_actionbar.xml:1: <?xml version="1.0" > > encoding="utf-8"?> > > On 2016/05/13 21:17:10, Lambros wrote: > > > Is this related to any action-bar? Maybe name this > > > host_options.xml or host_context_menu.xml instead? > > > > Done. Renamed to host_context_menu.xml > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > > remoting/android/java/res/menu/options_actionbar.xml:10: <item > > android:id="@+id/actionbar_connect" > > On 2016/05/13 21:17:10, Lambros wrote: > > > host_menu_connect? (I think just 'connect' might be OK, I don't think it > > matters > > > about name-collisions with other files?) > > > > Renamed to connect and delete. The only concern is if later we need different > > connect and delete context menu item for different purpose then we will need > to > > rename something. We can think about this when we get to this point... > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/r... > > remoting/android/java/res/menu/options_actionbar.xml:12: > > app:showAsAction="never"/> > > On 2016/05/13 21:17:10, Lambros wrote: > > > Do we need to specify showAsAction for context-menu items? > > > > Removed. > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... > > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java > (right): > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... > > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:422: > HostInfo > > hostInfo = mHosts[hostIndex]; > > On 2016/05/13 21:17:11, Lambros wrote: > > > I would suggest factoring out this block into a separate method (probably > > taking > > > a HostInfo parameter), to reduce indentation and keep this method short. > > > > Done. > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/android/java/s... > > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:424: > > AlertDialog.Builder builder = new AlertDialog.Builder(this); > > On 2016/05/13 21:17:11, Lambros wrote: > > > Maybe remove |builder| temporary and do: > > > > > > new AlertDialog.Builder(this) > > > .setMessage(...) > > > .setPositive(...) > > > ... > > > > Done. > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... > > File remoting/resources/remoting_strings.grd (right): > > > > > https://codereview.chromium.org/1968283002/diff/80001/remoting/resources/remo... > > remoting/resources/remoting_strings.grd:1119: <message desc="Delete an item > from > > a list." meaning="Delete an item from a list." name="IDS_DELETE_LIST_ITEM" > > formatter_data="android_java"> > > On 2016/05/13 21:17:11, Lambros wrote: > > > Maybe mention that this text appears on a button or a menu item, so > > translators > > > keep it short. Something like: > > > > > > "This text is used on a button or context-menu or similar control. When > chosen > > > or pressed, it deletes a selected item from a list." > > > > Done. > > sergeyu@ jamiewalch@ any last minute comment? LGTM from one committer is enough, unless there are unaddressed comments from others.
Description was changed from ========== Implementing Host List Context Menu and Delete Feature * Implements host list context menu triggered by long-pressing the host item. * Implements host deletion functionality. * Refactors some Auth token fetching code. BUG=607216 ========== to ========== Implementing Host List Context Menu and Delete Feature * Implements host list context menu triggered by long-pressing the host item. * Implements host deletion functionality. BUG=607216 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1968283002/#ps100001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968283002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968283002/100001
Message was sent while issue was closed.
Description was changed from ========== Implementing Host List Context Menu and Delete Feature * Implements host list context menu triggered by long-pressing the host item. * Implements host deletion functionality. BUG=607216 ========== to ========== Implementing Host List Context Menu and Delete Feature * Implements host list context menu triggered by long-pressing the host item. * Implements host deletion functionality. BUG=607216 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implementing Host List Context Menu and Delete Feature * Implements host list context menu triggered by long-pressing the host item. * Implements host deletion functionality. BUG=607216 ========== to ========== Implementing Host List Context Menu and Delete Feature * Implements host list context menu triggered by long-pressing the host item. * Implements host deletion functionality. BUG=607216 Committed: https://crrev.com/7bc6cf4ed459a86f9fa65e1dfbc098d101a81131 Cr-Commit-Position: refs/heads/master@{#393714} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7bc6cf4ed459a86f9fa65e1dfbc098d101a81131 Cr-Commit-Position: refs/heads/master@{#393714} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
