Update app info and install prompt UI to show retained devices.

Update app info and install prompt UI to show retained devices.

This patch updates the app info dialog and the extensions install prompt
dialog user interfaces to show, for each extension, the devices that are
recorded in the SavedDevicesService. Along with retained files, access
to these devices can be revoked by clicking a button in these dialog
boxes. Devices not written to ExtensionPrefs (devices that are ephemeral
because the do not have a serial number) are not shown here. Permission
to access them is dropped when they are disconnected or Chrome exits.

BUG=346953
Committed: https://crrev.com/c64d3dd16d71e8f18c05b7d48d74722c8c922dfc
Cr-Commit-Position: refs/heads/master@{#297262}

[Reilly Grant (use Gerrit), 2014-09-19 01:19:20]

reillyg@chromium.org changed reviewers:
+ finnur@chromium.org, tapted@chromium.org

[Reilly Grant (use Gerrit), 2014-09-19 01:41:24]

reillyg@chromium.org changed reviewers:
+ avi@chromium.org

[Reilly Grant (use Gerrit), 2014-09-19 02:32:24]

Patchset #2 (id:20001) has been deleted

[Reilly Grant (use Gerrit), 2014-09-19 02:32:31]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2014-09-19 02:32:58]

This patch depends on https://codereview.chromium.org/580963002/.

[sashab, 2014-09-19 02:38:26]

I really need to be in OWNERS for app_info_dialog :'( But thanks for the CC
:)

On Fri Sep 19 2014 at 12:33:00 PM <reillyg@chromium.org> wrote:

> Reviewers: tapted, Finnur (OOO until Sept 23), Avi,
>
> Message:
> This patch depends on https://codereview.chromium.org/580963002/.
>
> Description:
> Update app info and install prompt UI to show retained devices.
>
> This patch updates the app info dialog and the extensions install prompt
> dialog user interfaces to show, for each extension, the devices that are
> recorded in the SavedDevicesService. Along with retained files, access
> to these devices can be revoked by clicking a button in these dialog
> boxes. Devices not written to ExtensionPrefs (devices that are ephemeral
> because the do not have a serial number) are not shown here. Permission
> to access them is dropped when they are disconnected or Chrome exits.
>
> BUG=346953
>
> Please review this at https://codereview.chromium.org/580363002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+382, -45 lines):
>    M apps/saved_devices_service.h
>    M apps/saved_devices_service.cc
>    M apps/saved_devices_service_unittest.cc
>    M chrome/app/generated_resources.grd
>    M chrome/browser/extensions/api/developer_private/developer_
> private_api.cc
>    M chrome/browser/extensions/extension_install_prompt.h
>    M chrome/browser/extensions/extension_install_prompt.cc
>    M chrome/browser/ui/cocoa/extensions/extension_install_
> view_controller.mm
>    M
> chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.h
>    M
> chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
>    M chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
>    M chrome/browser/ui/webui/extensions/extension_settings_handler.cc
>    M extensions/extensions_strings.grd
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2014-09-19 03:54:46]

Dunno what you wanted me for; I imagine the Cocoa stuff. That LGTM, as does what
sense I can make of the extensions code.

https://codereview.chromium.org/580363002/diff/40001/apps/saved_devices_servi...
File apps/saved_devices_service.cc (right):

https://codereview.chromium.org/580363002/diff/40001/apps/saved_devices_servi...
apps/saved_devices_service.cc:309:
base::ASCIIToUTF16(base::StringPrintf("0x%04X", it->product_id)),
For this and all the other cases, string_number_conversions.h's HexEncode,
perhaps?

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:326: <message
name="IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_PID_SERIAL" desc="Permission
string for accessing a USB device with a known vendor name and serial number and
a hexidecimal product ID.">
hexadecimal

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:329: <message
name="IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_VID_PID_SERIAL" desc="Permission
string for accessing a USB device with a known serial number and a hexidecimal
vendor and product IDs.">
ditto

[tapted, 2014-09-19 04:09:09]

Mostly lg - just one question below (What happens if an extension has both
retained devices and retained files?)

It might be nice if there was more code sharing between devices/files... but I
guess there's not really much gained

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:345:
IDS_EXTENSION_PROMPT_PERMISSIONS_CLEAR_RETAINED_FILES_AND_DEVICES_BUTTON;
(hm I guess clang-format put a line-break there to try to save one space..
probably best to leave it)

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:402: std::vector<int>
message_ids;
(optional) I would have used the iterator begin/end vector constructor here, but
I think I'm weird. What you have is fine, and consistent with the above.

i.e.
  const std::vector<int> message_ids(
      kRetainedDevicesMessageIDs,
      kRetainedDevicesMessageIDs + arraysize(kRetainedDevicesMessageIDs));

or (C++11)
  const std::vector<int> message_ids(
      begin(kRetainedDevicesMessageIDs),
      end(kRetainedDevicesMessageIDs));

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:511: const base::string16&
nit: maybe return a value? just for consistency with the rest of the API (i.e.
that it works in this case feels like an implementation detail that shouldn't
leak out to the public class API). I don't really feel strongly about this
though

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm
(right):

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:668:
type = ExtensionInstallPrompt::RETAINED_DEVICES_DETAILS;
What happens if an extension has both retained devices and retained files?

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:674:
[children
nit: a temporary `title` var will probably make this wrapping more sane. i.e.

      NSString* title =
          SysUTF16ToNSString(prompt.GetRetainedDeviceMessageString(i));
      [children addObject:[self buildItemWithTitle:title
                                    cellAttributes:kUseBullet
                                          children:nil]];

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:683:
[warnings
same here, clang-format will probably do something like

    NSString* title = SysUTF16ToNSString(prompt.GetRetainedDevicesHeading());
    [warnings addObject:[self buildItemWithTitle:title
                                  cellAttributes:kBoldText | kAutoExpandCell |
                                                 kNoExpandMarker
                                        children:children]];

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:326: <message
name="IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_PID_SERIAL" desc="Permission
string for accessing a USB device with a known vendor name and serial number and
a hexidecimal product ID.">
hexidecimal -> hexadecimal

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:329: <message
name="IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_VID_PID_SERIAL" desc="Permission
string for accessing a USB device with a known serial number and a hexidecimal
vendor and product IDs.">
hexidecimal -> hexadecimal

[Reilly Grant (use Gerrit), 2014-09-19 19:30:10]

Thank you both for the reviews. Avi, yes, I was looking to you for the Cocoa
changes.

https://codereview.chromium.org/580363002/diff/40001/apps/saved_devices_servi...
File apps/saved_devices_service.cc (right):

https://codereview.chromium.org/580363002/diff/40001/apps/saved_devices_servi...
apps/saved_devices_service.cc:309:
base::ASCIIToUTF16(base::StringPrintf("0x%04X", it->product_id)),
On 2014/09/19 03:54:46, Avi wrote:
> For this and all the other cases, string_number_conversions.h's HexEncode,
> perhaps?

HexEncode is great for printing uint8_t arrays but for primitive types it isn't
safe because of varying endianness.

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:402: std::vector<int>
message_ids;
On 2014/09/19 04:09:09, tapted wrote:
> (optional) I would have used the iterator begin/end vector constructor here,
but
> I think I'm weird. What you have is fine, and consistent with the above.
> 
> i.e.
>   const std::vector<int> message_ids(
>       kRetainedDevicesMessageIDs,
>       kRetainedDevicesMessageIDs + arraysize(kRetainedDevicesMessageIDs));
> 
> or (C++11)
>   const std::vector<int> message_ids(
>       begin(kRetainedDevicesMessageIDs),
>       end(kRetainedDevicesMessageIDs));

Done.

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:511: const base::string16&
On 2014/09/19 04:09:09, tapted wrote:
> nit: maybe return a value? just for consistency with the rest of the API (i.e.
> that it works in this case feels like an implementation detail that shouldn't
> leak out to the public class API). I don't really feel strongly about this
> though

Done.

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm
(right):

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:668:
type = ExtensionInstallPrompt::RETAINED_DEVICES_DETAILS;
On 2014/09/19 04:09:09, tapted wrote:
> What happens if an extension has both retained devices and retained files?

You get two lists and the button says "Revoke File and Device Access".

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:674:
[children
On 2014/09/19 04:09:09, tapted wrote:
> nit: a temporary `title` var will probably make this wrapping more sane. i.e.
> 
>       NSString* title =
>           SysUTF16ToNSString(prompt.GetRetainedDeviceMessageString(i));
>       [children addObject:[self buildItemWithTitle:title
>                                     cellAttributes:kUseBullet
>                                           children:nil]];

Done.

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:326: <message
name="IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_PID_SERIAL" desc="Permission
string for accessing a USB device with a known vendor name and serial number and
a hexidecimal product ID.">
On 2014/09/19 04:09:09, tapted wrote:
> hexidecimal -> hexadecimal

Done.

https://codereview.chromium.org/580363002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:329: <message
name="IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_VID_PID_SERIAL" desc="Permission
string for accessing a USB device with a known serial number and a hexidecimal
vendor and product IDs.">
On 2014/09/19 04:09:09, tapted wrote:
> hexidecimal -> hexadecimal

Done.

[tapted, 2014-09-22 01:04:36]

lgtm

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm
(right):

https://codereview.chromium.org/580363002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:668:
type = ExtensionInstallPrompt::RETAINED_DEVICES_DETAILS;
On 2014/09/19 19:30:09, reillyg wrote:
> On 2014/09/19 04:09:09, tapted wrote:
> > What happens if an extension has both retained devices and retained files?
> 
> You get two lists and the button says "Revoke File and Device Access".
> 

Ah - cool. On the cocoa side it looked like `type` was getting clobbered if it
was previously set in the previous `if` block as well. It's not clear that it's
essentially unused after each call to -[warnings addObject:..]

An improvement to reduce confusion might be to redeclare `type` inside each
block . i.e.

  ExtensionInstallPrompt::DetailsType type =
      ExtensionInstallPrompt::PERMISSIONS_DETAILS;
  if (prompt.ShouldShowPermissions()) {
    NSMutableArray* children = [NSMutableArray array];
    ...

could become

  if (prompt.ShouldShowPermissions()) {
    const ExtensionInstallPrompt::DetailsType type =
        ExtensionInstallPrompt::PERMISSIONS_DETAILS;
    NSMutableArray* children = [NSMutableArray array];
    ...


But this is OK too - I just need to expand my diff a bit more :)

[Finnur, 2014-09-23 09:46:03]

Can you post a screenshot of how this looks?

https://codereview.chromium.org/580363002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/580363002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:311: }
nit: No need for braces (single-line).

https://codereview.chromium.org/580363002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:348: } else {
Ummm... Won't this use the RetainedDevices label when the extension has neither
retained files not retained devices?

[Reilly Grant (use Gerrit), 2014-09-25 06:12:54]

Patchset #3 (id:80001) has been deleted

[Reilly Grant (use Gerrit), 2014-09-25 17:05:46]

Patchset #3 (id:100001) has been deleted

[Reilly Grant (use Gerrit), 2014-09-25 17:06:22]

I've attached screenshots of the new UI to the bug here:

https://code.google.com/p/chromium/issues/detail?id=346953#c18

https://codereview.chromium.org/580363002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/580363002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:311: }
On 2014/09/23 09:46:03, Finnur wrote:
> nit: No need for braces (single-line).

The C++ style guide allows braces for single-line ifs.

https://codereview.chromium.org/580363002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:348: } else {
On 2014/09/23 09:46:03, Finnur wrote:
> Ummm... Won't this use the RetainedDevices label when the extension has
neither
> retained files not retained devices? 

Since HasAcceptButtonLabel() will return false when ShouldDisplayRevokeButton()
is false this function won't be called and there won't be a second button on
this dialog box at all.

[Finnur, 2014-09-26 09:41:57]

LGTM

[Reilly Grant (use Gerrit), 2014-09-29 20:18:36]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2014-09-29 20:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/580363002/140001

[commit-bot: I haz the power, 2014-09-29 21:10:20]

Committed patchset #4 (id:140001) as 3132fec61b38dd7e5cd0a5e52cba2ebd3e617474

[commit-bot: I haz the power, 2014-09-29 21:15:24]

Patchset 4 (id:??) landed as
https://crrev.com/c64d3dd16d71e8f18c05b7d48d74722c8c922dfc
Cr-Commit-Position: refs/heads/master@{#297262}