Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159533002:
Add a section to Site Settings listing USB devices. (Closed)
|
Created:
4 years, 5 months ago by Reilly Grant (use Gerrit) Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@android_usb_settings Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a section to Site Settings listing USB devices.
The new section lists USB devices that the user has granted a website
permission to access. Tapping on a particular device brings you to the
site settings view for that site.
BUG=601627
Committed: https://crrev.com/e11e868ed72f931b1fc1f72df3475e5698462057
Cr-Commit-Position: refs/heads/master@{#412722}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressed twellington@'s feedback. #
Total comments: 1
Patch Set 3 : Make new preference code USB-specific for now. #
Total comments: 1
Patch Set 4 : Rename UsbPreferences and remove autoclose behavior. #Patch Set 5 : Restore automatic dismissal behavior and fix a bug while searching. #
Total comments: 2
Patch Set 6 : Add comment explaining the automatic dismiss behavior. #
Total comments: 24
Patch Set 7 : Address most of tedchoc@'s comments. #Patch Set 8 : Update string to "No USB devices here" #Patch Set 9 : Switch to ArrayList and other cleanups. #Patch Set 10 : Iterate ArrayLists by index. #Messages
Total messages: 55 (32 generated)
Description was changed from ========== Add a section to Site Settings listing USB devices. The new section lists USB devices that the user has granted a website permission to access. Tapping on a particular device brings you to the site settings view for that site. BUG=430999,601627 ========== to ========== Add a section to Site Settings listing USB devices. The new section lists USB devices that the user has granted a website permission to access. Tapping on a particular device brings you to the site settings view for that site. BUG=601627 ==========
reillyg@chromium.org changed reviewers: + tedchoc@chromium.org, twellington@chromium.org
Patchset #1 (id:1) has been deleted
tedchoc@, please review //chrome/android/java/res. twellington@, please review //chrome/android/java/src.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:39: // Multiple sites may have access to the same object. A canonical UsbInfo for each object is Does using a UsbInfo object work if this is displaying Bluetooth devices?
https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:39: // Multiple sites may have access to the same object. A canonical UsbInfo for each object is On 2016/08/04 at 01:11:47, Theresa Wellington wrote: > Does using a UsbInfo object work if this is displaying Bluetooth devices? When Bluetooth starts storing permissions we will have to add a class like ObjectInfo from which UsbInfo and BluetoothInfo both derive and so this code will probably change to operate on this new parent class. Since that isn't necessary at the moment I'm working work UsbInfos directly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:30: * Shows a list of items that the user has granted to websites through a chooser. For example, nit: "Shows a list of items that the user has granted websites permission to access.."? https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:39: // Multiple sites may have access to the same object. A canonical UsbInfo for each object is On 2016/08/04 01:15:52, Reilly Grant wrote: > On 2016/08/04 at 01:11:47, Theresa Wellington wrote: > > Does using a UsbInfo object work if this is displaying Bluetooth devices? > > When Bluetooth starts storing permissions we will have to add a class like > ObjectInfo from which UsbInfo and BluetoothInfo both derive and so this code > will probably change to operate on this new parent class. Since that isn't > necessary at the moment I'm working work UsbInfos directly. That sounds like a good plan. Will you please add a TODO or note somewhere? The JavaDoc for the class makes it sound like things besides USB are already supported. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:157: * Refreshes |mPermissionsByObject| and |mSitesByObject| with new data from C++. nit: s/C++/native https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:170: if (mSitesByObject.isEmpty()) { Does it matter if this is mSitesByObject vs mPermissionsByObject? https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:71: public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) { Would it make sense to pull this out into a super class to avoid the duplicate code in this method and onOptionsItemSelected? https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:119: // sites with permission for this device. Does this comment go with resetList() (if the set of sites was included, we don't need to call getInfo())? https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:166: getActivity().finish(); Should we display something if there are no sites with permission to access a device (is that ever expected to happen?) rather than finishing the activity? https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:187: listView.setDivider(null); Does this need to be called every time resetList() is executed or can it be done once in the constructor like in SingleChooserPreference? https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:12: * When these objects are compared only the identity of the device, not which site has permission to nit: This sentence feels incomplete. Maybe something like "... only the identify of the device is used.."
https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:30: * Shows a list of items that the user has granted to websites through a chooser. For example, On 2016/08/04 at 16:39:54, Theresa Wellington wrote: > nit: "Shows a list of items that the user has granted websites permission to access.."? Done. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:39: // Multiple sites may have access to the same object. A canonical UsbInfo for each object is On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > On 2016/08/04 01:15:52, Reilly Grant wrote: > > On 2016/08/04 at 01:11:47, Theresa Wellington wrote: > > > Does using a UsbInfo object work if this is displaying Bluetooth devices? > > > > When Bluetooth starts storing permissions we will have to add a class like > > ObjectInfo from which UsbInfo and BluetoothInfo both derive and so this code > > will probably change to operate on this new parent class. Since that isn't > > necessary at the moment I'm working work UsbInfos directly. > > That sounds like a good plan. Will you please add a TODO or note somewhere? The JavaDoc for the class makes it sound like things besides USB are already supported. Done. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:157: * Refreshes |mPermissionsByObject| and |mSitesByObject| with new data from C++. On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > nit: s/C++/native Done. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:170: if (mSitesByObject.isEmpty()) { On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > Does it matter if this is mSitesByObject vs mPermissionsByObject? No, the key sets should be equal. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:71: public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) { On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > Would it make sense to pull this out into a super class to avoid the duplicate code in this method and onOptionsItemSelected? Can I do that in a followup patch? I don't want to touch all of the other PreferenceFragments in this patch since it's already big enough. It makes sense to implement a "SearchablePreferenceFragment" with this common code in it. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:119: // sites with permission for this device. On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > Does this comment go with resetList() (if the set of sites was included, we don't need to call getInfo())? I've removed this comment since the one down in resetList() makes the intent clearer. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:166: getActivity().finish(); On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > Should we display something if there are no sites with permission to access a device (is that ever expected to happen?) rather than finishing the activity? This will happen if the user returns to this activity from SingleWebsitePreferences after revoking some premissions. Instead of returning to an empty list the intent is to send them all the way back to SingleChooserPreferences which handles the case of there being no permission by displaying a special message. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:187: listView.setDivider(null); On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > Does this need to be called every time resetList() is executed or can it be done once in the constructor like in SingleChooserPreference? It probably can be. https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:12: * When these objects are compared only the identity of the device, not which site has permission to On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > nit: This sentence feels incomplete. Maybe something like "... only the identify of the device is used.." Added a verb.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:71: public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) { On 2016/08/04 19:57:11, Reilly Grant wrote: > On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > > Would it make sense to pull this out into a super class to avoid the duplicate > code in this method and onOptionsItemSelected? > > Can I do that in a followup patch? I don't want to touch all of the other > PreferenceFragments in this patch since it's already big enough. It makes sense > to implement a "SearchablePreferenceFragment" with this common code in it. sg https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:166: getActivity().finish(); On 2016/08/04 19:57:11, Reilly Grant wrote: > On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > > Should we display something if there are no sites with permission to access a > device (is that ever expected to happen?) rather than finishing the activity? > > This will happen if the user returns to this activity from > SingleWebsitePreferences after revoking some premissions. Instead of returning > to an empty list the intent is to send them all the way back to > SingleChooserPreferences which handles the case of there being no permission by > displaying a special message. Just to check my understanding, they're here -> single site, revoke permission -> press back button which sends them back to this fragment, we call getInfo() in onResume, the list is now empty so we finish the activity? Will the user see this screen while we're fetching website perms?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java (right): https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:166: getActivity().finish(); On 2016/08/04 at 20:22:28, Theresa Wellington wrote: > On 2016/08/04 19:57:11, Reilly Grant wrote: > > On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > > > Should we display something if there are no sites with permission to access a > > device (is that ever expected to happen?) rather than finishing the activity? > > > > This will happen if the user returns to this activity from > > SingleWebsitePreferences after revoking some premissions. Instead of returning > > to an empty list the intent is to send them all the way back to > > SingleChooserPreferences which handles the case of there being no permission by > > displaying a special message. > > Just to check my understanding, they're here -> single site, revoke permission -> press back button which sends them back to this fragment, we call getInfo() in onResume, the list is now empty so we finish the activity? Will the user see this screen while we're fetching website perms? That's correct. In theory the user could see the old version of the activity but in practice it happens more quickly than I've been able to notice.
On 2016/08/04 21:30:25, Reilly Grant wrote: > https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java > (right): > > https://codereview.chromium.org/2159533002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleObjectPreferences.java:166: > getActivity().finish(); > On 2016/08/04 at 20:22:28, Theresa Wellington wrote: > > On 2016/08/04 19:57:11, Reilly Grant wrote: > > > On 2016/08/04 at 16:39:55, Theresa Wellington wrote: > > > > Should we display something if there are no sites with permission to > access a > > > device (is that ever expected to happen?) rather than finishing the > activity? > > > > > > This will happen if the user returns to this activity from > > > SingleWebsitePreferences after revoking some premissions. Instead of > returning > > > to an empty list the intent is to send them all the way back to > > > SingleChooserPreferences which handles the case of there being no permission > by > > > displaying a special message. > > > > Just to check my understanding, they're here -> single site, revoke permission > -> press back button which sends them back to this fragment, we call getInfo() > in onResume, the list is now empty so we finish the activity? Will the user see > this screen while we're fetching website perms? > > That's correct. In theory the user could see the old version of the activity but > in practice it happens more quickly than I've been able to notice. Won't it depend on how many website permissions there are to fetch? I think we should run this by UX.
https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:45: // TODO: UsbInfo will need to be replaced with something more generic when other types of I was thinking through this more and wondering - is bluetooth in flight? If not, can this class just be UsbChooserPreference so that it's really clear later on when refactoring that this was designed for usb (and so that it makes sense if we don't get to bluetooth for a long time)?
On 2016/08/05 at 21:29:51, twellington wrote: > https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java (right): > > https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:45: // TODO: UsbInfo will need to be replaced with something more generic when other types of > I was thinking through this more and wondering - is bluetooth in flight? If not, can this class just be UsbChooserPreference so that it's really clear later on when refactoring that this was designed for usb (and so that it makes sense if we don't get to bluetooth for a long time)? Bluetooth is coming but not for at least a quarter as they have other priorities. I've renamed the classes to make them USB specific for now.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/05 23:59:28, Reilly Grant wrote: > On 2016/08/05 at 21:29:51, twellington wrote: > > > https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... > > File > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java > (right): > > > > > https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:45: > // TODO: UsbInfo will need to be replaced with something more generic when other > types of > > I was thinking through this more and wondering - is bluetooth in flight? If > not, can this class just be UsbChooserPreference so that it's really clear later > on when refactoring that this was designed for usb (and so that it makes sense > if we don't get to bluetooth for a long time)? > > Bluetooth is coming but not for at least a quarter as they have other > priorities. I've renamed the classes to make them USB specific for now. Thank you. I think the names are a tad confusing without reading the JavaDocs (UsbPreference vs UsbDevicePreference sound like the same thing), so I'll think a little on how we may be able to distinguish them. Wrt to dismissing the UsbDevicePreference (formerly SingleObjectPreference) if the user returns to it after revoking permission from a SitePreference page -- I think there are two options. 1) Pass something to the SingleWebsitePreference indicating its the only site with permission for xyz device. When SingleWebsitePermission is finished it can send something back that indicates the USB permission was revoked so that the activity can be finished immediately. This seems feasible but possibly complex. 2) Show some error text, e.g. "None" or "No websites have permission to access this device." The only action the user can take is to press back again to get back to the list of USB devices. This seems much simpler. Has this been implemented on desktop yet? Can we pull any guidance from there?
https://codereview.chromium.org/2159533002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbPreferences.java (right): https://codereview.chromium.org/2159533002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbPreferences.java:172: mEmptyView.setText(R.string.no_saved_website_settings); "You have no saved website settings." doesn't seem like the right string to show here.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/09 at 00:23:42, twellington wrote: > On 2016/08/05 23:59:28, Reilly Grant wrote: > > On 2016/08/05 at 21:29:51, twellington wrote: > > > > > https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java > > (right): > > > > > > > > https://codereview.chromium.org/2159533002/diff/40001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleChooserPreferences.java:45: > > // TODO: UsbInfo will need to be replaced with something more generic when other > > types of > > > I was thinking through this more and wondering - is bluetooth in flight? If > > not, can this class just be UsbChooserPreference so that it's really clear later > > on when refactoring that this was designed for usb (and so that it makes sense > > if we don't get to bluetooth for a long time)? > > > > Bluetooth is coming but not for at least a quarter as they have other > > priorities. I've renamed the classes to make them USB specific for now. > > Thank you. I think the names are a tad confusing without reading the JavaDocs (UsbPreference vs UsbDevicePreference sound like the same thing), so I'll think a little on how we may be able to distinguish them. > > Wrt to dismissing the UsbDevicePreference (formerly SingleObjectPreference) if the user returns to it after revoking permission from a SitePreference page -- I think there are two options. > 1) Pass something to the SingleWebsitePreference indicating its the only site with permission for xyz device. When SingleWebsitePermission is finished it can send something back that indicates the USB permission was revoked so that the activity can be finished immediately. This seems feasible but possibly complex. > 2) Show some error text, e.g. "None" or "No websites have permission to access this device." The only action the user can take is to press back again to get back to the list of USB devices. This seems much simpler. > > Has this been implemented on desktop yet? Can we pull any guidance from there? The desktop design is very different. This is closer to the desktop MD mocks which have not yet been implemented. I've renamed UsbPreferences to UsbChooserPreferences to try to make the names more descriptive. I've also removed the call to Activity.finish() and now just display an empty list. Unfortunately I can see how to display text in UsbChooserPreferences when the list is empty but don't know how to do it in UsbDevicePreferences because technically the list is never empty. SingleCategoryPreferences doesn't solve this either.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
As discussed with rolfe@ I've put back the automatic dismissal of UsbDevicePreferences when the site list is empty and fixed an issue with the "clear all" button when a search is in progress.
lgtm % 1 comment https://codereview.chromium.org/2159533002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java (right): https://codereview.chromium.org/2159533002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:166: getActivity().finish(); Please add a comment here explaining the behavior and how one might get into this state.
tedchoc@, please take a look. https://codereview.chromium.org/2159533002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java (right): https://codereview.chromium.org/2159533002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:166: getActivity().finish(); On 2016/08/12 at 18:24:59, Theresa Wellington wrote: > Please add a comment here explaining the behavior and how one might get into this state. Done.
https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/re... File chrome/android/java/res/xml/single_object_preferences.xml (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/re... chrome/android/java/res/xml/single_object_preferences.xml:9: android:widgetLayout="@layout/usb_permission" /> this seems more specific than the name of the file suggests. should this be renamed usb_[chooser|device]_preferences.xml based on the other renames? https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:135: getPreferenceScreen().removePreference(findPreference(USB_KEY)); move down a line for alpha ordering like the keys above? https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:46: private Map<UsbInfo, LinkedList<UsbInfo>> mPermissionsByObject = new HashMap<>(); unless you need some property of LinkedList, it'd be better to do Map<UsbInfo, List<UsbInfo>> https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:136: LinkedList<UsbInfo> permissionSet = mPermissionsByObject.get(info); naming nit, but it's odd to have these be called permissionSet when it's actually a list. permissionList and sitesList below make more sense to me. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:191: extras.putString( any reason we don't set these extras when we are building it in resetList above? Will they change between resetList and the time onPreferenceClick is called? Seems kinda odd to have it set it in both places. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:197: "org.chromium.chrome.browser.preferences.website.UsbDevicePreferences"); can we use UsbDevicePreferences.class.getFullName() or something here...i always forget in java which one should return this. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:130: for (UsbInfo info : mUsbInfos) info.revoke(); Since this is a linkedlist, I don't believe this will allocate an interator object. But normally, we prefer the for (int i = 0; i...) method of iterating lists. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:140: "org.chromium.chrome.browser.preferences.website.SingleWebsitePreferences"); same comment about using class names directly instead of this. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:58: return mObject.hashCode(); I find this to be somewhat odd. Why not just expose mObject and use that for the key in your maps? This just seems like you are special casing this behavior to one implementation in the UI and that seems fragile. If we keep this as is, I think we should write some simple tests that force this behavior to be maintained as I don't think the comment above would be sufficient for people to realize the implications of changing it.
https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/re... File chrome/android/java/res/xml/single_object_preferences.xml (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/re... chrome/android/java/res/xml/single_object_preferences.xml:9: android:widgetLayout="@layout/usb_permission" /> On 2016/08/15 at 20:31:59, Ted C wrote: > this seems more specific than the name of the file suggests. should this be renamed usb_[chooser|device]_preferences.xml based on the other renames? Done. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:135: getPreferenceScreen().removePreference(findPreference(USB_KEY)); On 2016/08/15 at 20:31:59, Ted C wrote: > move down a line for alpha ordering like the keys above? Done. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:46: private Map<UsbInfo, LinkedList<UsbInfo>> mPermissionsByObject = new HashMap<>(); On 2016/08/15 at 20:31:59, Ted C wrote: > unless you need some property of LinkedList, it'd be better to do Map<UsbInfo, List<UsbInfo>> It needs to be LinkedList so that it's Serializable. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:136: LinkedList<UsbInfo> permissionSet = mPermissionsByObject.get(info); On 2016/08/15 at 20:31:59, Ted C wrote: > naming nit, but it's odd to have these be called permissionSet when it's actually a list. permissionList and sitesList below make more sense to me. Agreed, it was a HashSet in an earlier version of this patch. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:191: extras.putString( On 2016/08/15 at 20:31:59, Ted C wrote: > any reason we don't set these extras when we are building it in resetList above? Will they change between resetList and the time onPreferenceClick is called? Seems kinda odd to have it set it in both places. To avoid serializing a bunch of stuff that won't be necessary unless the entry is actually tapped. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:197: "org.chromium.chrome.browser.preferences.website.UsbDevicePreferences"); On 2016/08/15 at 20:31:59, Ted C wrote: > can we use UsbDevicePreferences.class.getFullName() or something here...i always forget in java which one should return this. Done. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:130: for (UsbInfo info : mUsbInfos) info.revoke(); On 2016/08/15 at 20:31:59, Ted C wrote: > Since this is a linkedlist, I don't believe this will allocate an interator object. But normally, we prefer the for (int i = 0; i...) method of iterating lists. I don't understand this comment. Using LinkedList.get(i) seems very odd to me versus using an iterator because the former is O(n) for looking up each item unless the runtime implements some sort of workaround. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:140: "org.chromium.chrome.browser.preferences.website.SingleWebsitePreferences"); On 2016/08/15 at 20:31:59, Ted C wrote: > same comment about using class names directly instead of this. Done. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbInfo.java:58: return mObject.hashCode(); On 2016/08/15 at 20:31:59, Ted C wrote: > I find this to be somewhat odd. > > Why not just expose mObject and use that for the key in your maps? Good idea, done.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Nothing blocking for me...just some clarifications on my previous comments. Do with it as you will. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:46: private Map<UsbInfo, LinkedList<UsbInfo>> mPermissionsByObject = new HashMap<>(); On 2016/08/16 17:41:59, Reilly Grant wrote: > On 2016/08/15 at 20:31:59, Ted C wrote: > > unless you need some property of LinkedList, it'd be better to do Map<UsbInfo, > List<UsbInfo>> > > It needs to be LinkedList so that it's Serializable. I guess the fundamental question I have is why LinkedList vs ArrayList? We use ArrayList much more commonly. That is also the basis for my Iterator vs i++ comment in the other file. An ArrayList is not penalized for lookups in the list while LinkedList is. Personally, I'd vote for switching to ArrayList for nothing but consistency. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:191: extras.putString( On 2016/08/16 17:41:59, Reilly Grant wrote: > On 2016/08/15 at 20:31:59, Ted C wrote: > > any reason we don't set these extras when we are building it in resetList > above? Will they change between resetList and the time onPreferenceClick is > called? Seems kinda odd to have it set it in both places. > > To avoid serializing a bunch of stuff that won't be necessary unless the entry > is actually tapped. Does it actually get serialized though? Just adding extras doesn't do anything: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... It wouldn't happen unless the extra is being serialized when you are calling addPreference above. https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... Nothing obviously jumps out as forcing a serialization in the addPreference call: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... I guess this just feels like a bit of premature optimization. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:130: for (UsbInfo info : mUsbInfos) info.revoke(); On 2016/08/16 17:41:59, Reilly Grant wrote: > On 2016/08/15 at 20:31:59, Ted C wrote: > > Since this is a linkedlist, I don't believe this will allocate an interator > object. But normally, we prefer the for (int i = 0; i...) method of iterating > lists. > > I don't understand this comment. Using LinkedList.get(i) seems very odd to me > versus using an iterator because the former is O(n) for looking up each item > unless the runtime implements some sort of workaround. You are definitely correct. LinkedList isn't the place to use .get(i). The problem with foreach iteration in java is that it calls .iterator() and results in another object allocation. We try to avoid that where possible on list iteration, so we avoid LinkedLists in general (per my comment in the other thread). Since we also lose the typed-ness of the mUsbInfos across the serialization boundary, this looks odd at first sight until I realized it is a LinkedList
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:46: private Map<UsbInfo, LinkedList<UsbInfo>> mPermissionsByObject = new HashMap<>(); On 2016/08/17 at 14:49:13, Ted C wrote: > On 2016/08/16 17:41:59, Reilly Grant wrote: > > On 2016/08/15 at 20:31:59, Ted C wrote: > > > unless you need some property of LinkedList, it'd be better to do Map<UsbInfo, > > List<UsbInfo>> > > > > It needs to be LinkedList so that it's Serializable. > > I guess the fundamental question I have is why LinkedList vs ArrayList? > > We use ArrayList much more commonly. That is also the basis for my Iterator > vs i++ comment in the other file. An ArrayList is not penalized for > lookups in the list while LinkedList is. Personally, I'd vote for switching > to ArrayList for nothing but consistency. Done. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbChooserPreferences.java:191: extras.putString( On 2016/08/17 at 14:49:13, Ted C wrote: > On 2016/08/16 17:41:59, Reilly Grant wrote: > > On 2016/08/15 at 20:31:59, Ted C wrote: > > > any reason we don't set these extras when we are building it in resetList > > above? Will they change between resetList and the time onPreferenceClick is > > called? Seems kinda odd to have it set it in both places. > > > > To avoid serializing a bunch of stuff that won't be necessary unless the entry > > is actually tapped. > > Does it actually get serialized though? Just adding extras doesn't do anything: > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > It wouldn't happen unless the extra is being serialized when you are calling > addPreference above. > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > Nothing obviously jumps out as forcing a serialization in the addPreference call: > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > I guess this just feels like a bit of premature optimization. Done. https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java (right): https://codereview.chromium.org/2159533002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/UsbDevicePreferences.java:130: for (UsbInfo info : mUsbInfos) info.revoke(); On 2016/08/17 at 14:49:14, Ted C wrote: > On 2016/08/16 17:41:59, Reilly Grant wrote: > > On 2016/08/15 at 20:31:59, Ted C wrote: > > > Since this is a linkedlist, I don't believe this will allocate an interator > > object. But normally, we prefer the for (int i = 0; i...) method of iterating > > lists. > > > > I don't understand this comment. Using LinkedList.get(i) seems very odd to me > > versus using an iterator because the former is O(n) for looking up each item > > unless the runtime implements some sort of workaround. > > You are definitely correct. LinkedList isn't the place to use .get(i). > > The problem with foreach iteration in java is that it calls .iterator() and > results in another object allocation. We try to avoid that where possible > on list iteration, so we avoid LinkedLists in general (per my comment in > the other thread). Since we also lose the typed-ness of the mUsbInfos across > the serialization boundary, this looks odd at first sight until I realized > it is a LinkedList Done.
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 reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2159533002/#ps200001 (title: "Iterate ArrayLists by index.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add a section to Site Settings listing USB devices. The new section lists USB devices that the user has granted a website permission to access. Tapping on a particular device brings you to the site settings view for that site. BUG=601627 ========== to ========== Add a section to Site Settings listing USB devices. The new section lists USB devices that the user has granted a website permission to access. Tapping on a particular device brings you to the site settings view for that site. BUG=601627 Committed: https://crrev.com/e11e868ed72f931b1fc1f72df3475e5698462057 Cr-Commit-Position: refs/heads/master@{#412722} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e11e868ed72f931b1fc1f72df3475e5698462057 Cr-Commit-Position: refs/heads/master@{#412722} |
