Groups

Mojofication on //device/hid. Convert //extensions to use hid mojo interface. [chromium/src : master]

14 views

Skip to first unread message

Ke He (Gerrit)

unread,

Sep 4, 2017, 6:55:36 AM9/4/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Hi, Reilly, PTAL on this, thanks very much.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 4, 2017, 7:00:46 AM9/4/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

1 comment:

File device/hid/hid_usage_and_page.h:
- Patch Set #3, Line 46: kPageApiTest = 0xFF01, // Only for HidApiTest.
  See https://cs.chromium.org/chromium/src/extensions/browser/api/hid/hid_apitest.cc?rcl=8b27fba0270d402310586cc7afc5ee0562073483&l=43
  hid_apitest uses 0xFF00 and 0xFF01 for testing, which causes crash in hid_struct_traits.cc.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 4, 2017, 11:01:34 PM9/4/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch Set 3:
(1 comment)

4 comments:

File chrome/browser/extensions/api/device_permissions_manager_unittest.cc:
- Patch Set #3, Line 42: FakeHidDeviceManager
  Create a Fake HidDeviceManager that don't really connect device service.
File device/hid/hid_manager.h:
- Patch Set #3, Line 56: AssociatedInterfacePtrSet
  Use the associated HidObserver intefacePtr to make sure GetDevicesCallback always run earlier than DeviceAdd()/DeviceRemoved().
File device/hid/hid_manager.cc:
- Patch Set #3, Line 88: auto binding = mojo::MakeStrongBinding(
  Use StrongBinding so HidConnectionManager has same lifetime as the mojo connection. If mojo connection is broken or client is destructed, HidConnectionManager instance will be automatically deleted too.
File extensions/browser/api/hid/hid_api.cc:
- Patch Set #3, Line 172: device_manager->Connect(
  Reuse the mojo connection in HidDeviceManager to do the connect(), we don't have to create a new mojo connection here.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 5, 2017, 4:08:44 PM9/5/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

20 comments:

File device/hid/hid_connection_manager.h:
- Patch Set #3, Line 18: HidConnectionManager
  I would prefer calling this HidConnectionImpl or MojoHidConnectionImpl (to distinguish it from HidConnection and its platform-specific subclasses).
File device/hid/hid_connection_manager.cc:
- Patch Set #3, Line 14: connection
  std::move(connection)
- Patch Set #3, Line 19:
```
  // Close connection in destructor because HidConnectionManager is StrongBound
  // with a HidConnectionPtr.
```
  "Close |connection| on destruction because this class is owned by a mojo::StrongBinding and will be destroyed when the pipe is closed."
- Patch Set #3, Line 29: base::Passed(&callback)
  std::move(callback) (Here and below whenever passing a move-only type to base::BindOnce.)
- Patch Set #3, Line 52:
```
  scoped_refptr<net::IOBufferWithSize> io_buffer(
      new net::IOBufferWithSize(buffer.size()));
```
  auto io_buffer = base::MakeRefCounted<net::IOBufferWithSize>(buffer.size());
- Patch Set #3, Line 100:
```
  scoped_refptr<net::IOBufferWithSize> io_buffer(
      new net::IOBufferWithSize(buffer.size()
```
  auto io_buffer = base::MakeRefCounted<net::IOBufferWithSize>(buffer.size());
File device/hid/hid_manager.h:
- Patch Set #3, Line 23: HidManager
  I would prefer calling this HidManagerImpl to avoid confusion between this and the generated HidManager type.
- Patch Set #3, Line 34: device::mojom::HidManager::
  As this is a subclass of device::mojom::HidManager the type name can be shortened to "GetDevicesCallback". (Here and below.)
- Patch Set #3, Line 35:
  nit: no space in the header between methods associated with the same interface.
File device/hid/hid_manager.cc:
- Patch Set #3, Line 40:
```
      base::Bind(&HidManager::CreateDeviceList, weak_factory_.GetWeakPtr(),
                 base::Passed(&callback), base::Passed(&observer_info)));
```
  Use base::BindOnce instead of base::Bind and std::move instead of base::Passed. (Here and below.)
- Patch Set #3, Line 51: base::Passed(&observer_info)
  nullptr can be passed here to indicate an invalid device::mojom::HidObserverAssociatedPtrInfo.
- Patch Set #3, Line 58:
```
std::vector<device::mojom::HidDeviceInfoPtr> device_list;
  for (auto& device : devices)
    device_list.push_back(device->Clone());
```
  Why declare a new vector here instead of just calling Run() with |devices| directly?
File device/hid/public/interfaces/hid.mojom:
- Patch Set #3, Line 88: eariler
  earlier
- Patch Set #3, Line 89: GetDevicesAndRegister(associated HidObserver observer_info)
  Convention is to refer to interfaces like HidObserver as a "client" so I would name this GetDevicesAndSetClient and rename HidObserver to HidManagerClient.
- Patch Set #3, Line 116:
  nit: trailing space at the end of the line
File device/hid/public/interfaces/hid_struct_traits.cc:
- Patch Set #3, Line 78:
```
    case device::HidUsageAndPage::Page::kPageApiTest:
      return device::mojom::HidPage::PageApiTest;
```
  Thinking about this, usages and usage pages from a device can have any value so the Mojo interface should not be this strict. Instead we should use uint16_t for both and replace these enums with a list of useful (but not exhaustive) constants.
File extensions/browser/api/device_permissions_prompt.cc:
- Patch Set #3, Line 261: DCHECK(device);
  This DCHECK isn't really necessary because |device| is not optional in the Mojo interface.
File extensions/browser/api/hid/hid_api.cc:
- Patch Set #3, Line 262: static_cast<int>
  Is this static_cast still necessary?
- Patch Set #3, Line 288:
```
for (auto& r : parameters_->data)
    buffer.push_back(static_cast<uint8_t>(r));
```
  buffer.insert(buffer.end(), parameters_->data.begin(), parameters_->data.end());
  If it doesn't like that because of implicit casts between char* and uint8_t* then,
  const uint8_t* data = reinterpret_cast<const uint8_t*>(parameters_->data.data());
  buffer.insert(buffer.end(), data, data + parameters_->data.size());
- Patch Set #3, Line 353:
```
for (auto& r : parameters_->data)
    buffer.push_back(static_cast<uint8_t>(r));
```
  Same here.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 6, 2017, 5:22:07 AM9/6/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Thanks very much:)

20 comments:

File device/hid/hid_connection_manager.h:

- Patch Set #3, Line 18:

- I would prefer calling this HidConnectionImpl or MojoHidConnectionImpl (to distinguish it from HidCo […]
  Done
File device/hid/hid_connection_manager.cc:

- Patch Set #3, Line 14:

- std::move(connection)
  Done

- Patch Set #3, Line 19:

- "Close |connection| on destruction because this class is owned by a mojo::StrongBinding and will be […]
  Done

- Patch Set #3, Line 29:

- std::move(callback) (Here and below whenever passing a move-only type to base::BindOnce. […]
  Done

- Patch Set #3, Line 52:

- auto io_buffer = base::MakeRefCounted<net::IOBufferWithSize>(buffer. […]
  Done

- Patch Set #3, Line 100:

- auto io_buffer = base::MakeRefCounted<net::IOBufferWithSize>(buffer. […]
  Done
File device/hid/hid_manager.h:

- Patch Set #3, Line 23:

- I would prefer calling this HidManagerImpl to avoid confusion between this and the generated HidMana […]
  Done

- Patch Set #3, Line 34:

- As this is a subclass of device::mojom::HidManager the type name can be shortened to "GetDevicesCall […]
  Done

- Patch Set #3, Line 35:
  nit: no space in the header between methods associated with the same interface.

- Patch Set #3, Line 40:

- Use base::BindOnce instead of base::Bind and std::move instead of base::Passed. (Here and below. […]
  The definition of HidService::GetDevicesCallback is a RepeatingCallback, I'll change it to OnceCallback in the next CL that handles //device/u2f, then I'll also change here too.
  So How about I leave it as a TODO and resolve it in next CL?

- Patch Set #3, Line 51:

- nullptr can be passed here to indicate an invalid device::mojom::HidObserverAssociatedPtrInfo.
  Done

- Patch Set #3, Line 58:

- Why declare a new vector here instead of just calling Run() with |devices| directly?

- Patch Set #3, Line 88:

- earlier
  Done

- Patch Set #3, Line 89:

- Convention is to refer to interfaces like HidObserver as a "client" so I would name this GetDevicesA […]
  Done

- Patch Set #3, Line 116:
  nit: trailing space at the end of the line

- Patch Set #3, Line 78:

- Thinking about this, usages and usage pages from a device can have any value so the Mojo interface s […]
  changed to uint16_t. And removed the codes in traits.
  Done.
File extensions/browser/api/device_permissions_prompt.cc:
- Patch Set #3, Line 261: if (HasUnprotec

- This DCHECK isn't really necessary because |device| is not optional in the Mojo interface.

- Done
File extensions/browser/api/hid/hid_api.cc:
- Patch Set #3, Line 262: (*buffer)[0];

- Is this static_cast still necessary?

- Done

- Patch Set #3, Line 288:

- ```
  const uint8_t* data =
```
  buffer.insert(buffer.end(), parameters_->data.begin(), parameters_->data.end()); […]
  Done

- Patch Set #3, Line 353:

std::vector<uint8_t> buffer;
  buffer.push_back(static_cast<uint8_t>(parame

Same here.

Done

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 6, 2017, 5:39:40 AM9/6/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch Set 4:
(20 comments)
Thanks very much:)

PTAL again. Thanks:)

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 6, 2017, 5:42:13 AM9/6/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

By the way, will we move the //device/u2f into device service? if no, I'll start to mojofy it. Thanks!

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 6, 2017, 11:45:57 AM9/6/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch Set 4:

By the way, will we move the //device/u2f into device service? if no, I'll start to mojofy it. Thanks!

I'll check with kpaulhamus@ and piperc@ on what their plans for //device/u2f and the WebAuthn implementation are. The original architecture did not anticipate the device service but already includes a Mojo layer.

1 comment:

File device/hid/hid_manager.cc:

- Patch Set #3, Line 40:

- The definition of HidService::GetDevicesCallback is a RepeatingCallback, I'll change it to OnceCallb […]
  It may be cleaner to wrap these callbacks in AdaptCallbackForRepeating and then do a pass through to remove the wrapping when HidService is updated.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 6, 2017, 4:35:17 PM9/6/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch Set 4:
(1 comment)

Patch Set 4:
By the way, will we move the //device/u2f into device service? if no, I'll start to mojofy it. Thanks!
I'll check with kpaulhamus@ and piperc@ on what their plans for //device/u2f and the WebAuthn implementation are. The original architecture did not anticipate the device service but already includes a Mojo layer.

I spoke to kpaulhamus@ and our plan for now is to not move //device/u2f into the device service so it should be updated to use the HID Mojo interface.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 6, 2017, 10:10:16 PM9/6/17

to Reilly Grant, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Darin Fisher, chromium...@chromium.org, John Abd-El-Malek, Commit Bot, Adam Barth, Aaron Boodman

Ke He uploaded patch set #6 to this change.

Mojofication on //device/hid. Convert //extensions to use hid mojo interface.

In this CL:
1) Add the HidManager and HidConnection interfaces in hid.mojom, and
   implements those mojo interfaces in //device/hid.

2) Convert the clients in //extensions to use hid mojo interfaces.

3) Change the type of HidUsageAndPage::usage_page to uint16_t.

4) Rewrite the hid_apitest base on the new added mojo interfaces.

TODO:
1) Move client library files into device/hid/public/cpp.
2) Host HidService by DeviceService instead of DeviceClient.
3) Mojofy //device/u2f Or just move it into DeviceService.

BUG=728223

Change-Id: I1de1b5211f7ef37f54d4bc18c7995a3b47cc7da2
---
M chrome/browser/extensions/api/device_permissions_manager_unittest.cc
M content/public/app/mojo/content_browser_manifest.json
M device/hid/BUILD.gn
A device/hid/hid_connection_impl.cc
A device/hid/hid_connection_impl.h
A device/hid/hid_manager_impl.cc
A device/hid/hid_manager_impl.h
M device/hid/hid_service.h
M device/hid/hid_usage_and_page.h
M device/hid/public/interfaces/hid.mojom
M device/hid/public/interfaces/hid_struct_traits.cc
M device/hid/public/interfaces/hid_struct_traits.h
M extensions/browser/BUILD.gn
M extensions/browser/api/device_permissions_prompt.cc
M extensions/browser/api/hid/hid_api.cc
M extensions/browser/api/hid/hid_api.h
M extensions/browser/api/hid/hid_apitest.cc
M extensions/browser/api/hid/hid_connection_resource.cc
M extensions/browser/api/hid/hid_connection_resource.h
M extensions/browser/api/hid/hid_device_manager.cc
M extensions/browser/api/hid/hid_device_manager.h
M extensions/shell/browser/shell_device_client.cc
M extensions/shell/browser/shell_device_client.h
M services/device/BUILD.gn
M services/device/device_service.cc
M services/device/device_service.h
M services/device/manifest.json
27 files changed, 763 insertions(+), 468 deletions(-)

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 7, 2017, 1:22:01 AM9/7/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Thanks! PTAL:)

1 comment:

File device/hid/hid_manager.cc:

- Patch Set #3, Line 40:

- It may be cleaner to wrap these callbacks in AdaptCallbackForRepeating and then do a pass through to […]
  Thanks. Change the GetDevices() and Connect() to use AdaptCallbackForRepeating.
  Done.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 7, 2017, 2:03:24 AM9/7/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch Set 4:

Patch Set 4:
(1 comment)
Patch Set 4:
By the way, will we move the //device/u2f into device service? if no, I'll start to mojofy it. Thanks!
I'll check with kpaulhamus@ and piperc@ on what their plans for //device/u2f and the WebAuthn implementation are. The original architecture did not anticipate the device service but already includes a Mojo layer.
I spoke to kpaulhamus@ and our plan for now is to not move //device/u2f into the device service so it should be updated to use the HID Mojo interface.

Hi Reilly,
IIUC, the implementation of u2f is still ongoing. The //device/u2f is not used at all so far, right? I didn't find any clients of it.

The problem is: //device/u2f needs to get a "Connector" to connect the device service. Usually the Connector is get from "content::ServiceManagerConnection", while we cannot introduce the deps to //content in //device/u2f folder, so the only way is to let the client pass a Connector to the //device/u2f.

As you said their architecture already includes a mojo layer, so it should be able to pass the Connector to //device/u2f. right? I'll continue the work of "using HID mojo interfaces in u2f" by temporarily using the "content::ServiceManagerConnection", and will replace them by passing Connector when everything is ready.

WDYT? Thanks very much!

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 7, 2017, 12:01:57 PM9/7/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch Set 7:

Patch Set 4:
Patch Set 4:
(1 comment)
Patch Set 4:
By the way, will we move the //device/u2f into device service? if no, I'll start to mojofy it. Thanks!
I'll check with kpaulhamus@ and piperc@ on what their plans for //device/u2f and the WebAuthn implementation are. The original architecture did not anticipate the device service but already includes a Mojo layer.
I spoke to kpaulhamus@ and our plan for now is to not move //device/u2f into the device service so it should be updated to use the HID Mojo interface.
Hi Reilly,
IIUC, the implementation of u2f is still ongoing. The //device/u2f is not used at all so far, right? I didn't find any clients of it.
The problem is: //device/u2f needs to get a "Connector" to connect the device service. Usually the Connector is get from "content::ServiceManagerConnection", while we cannot introduce the deps to //content in //device/u2f folder, so the only way is to let the client pass a Connector to the //device/u2f.
As you said their architecture already includes a mojo layer, so it should be able to pass the Connector to //device/u2f. right? I'll continue the work of "using HID mojo interfaces in u2f" by temporarily using the "content::ServiceManagerConnection", and will replace them by passing Connector when everything is ready.
WDYT? Thanks very much!

Yes, adding a service_manager::Connector* parameter to the U2fRequest constructor seems reasonable.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 7, 2017, 2:57:19 PM9/7/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

17 comments:

File device/hid/hid_connection_impl.h:
- Patch Set #7, Line 24: device::mojom::HidConnection::ReadCallback
  Here and below for the other callbacks "device::mojom::HidConnection" is not necessary because this class extends device::mojom::HidConnection.
File device/hid/hid_connection_impl.cc:
- Patch Set #7, Line 25: device::mojom::HidConnection::ReadCallback
  Same here in the .cc file.
File extensions/browser/api/device_permissions_prompt.cc:
- Patch Set #7, Line 207: }
  nit: no braces for single-line if here and below
- Patch Set #7, Line 218: auto
  service_manager::Connector
- Patch Set #7, Line 220:
  nit: no space required here, |connector| is defined specifically for this line.
- Patch Set #7, Line 289: {
  nit: no braces for single-line for loop
File extensions/browser/api/hid/hid_api.cc:
- Patch Set #7, Line 289: const uint8_t
  auto if that allows this to fit on one line
- Patch Set #7, Line 356: const uint8_t
  same here
File extensions/browser/api/hid/hid_apitest.cc:
- Patch Set #7, Line 56: FakeHidConnectionImpl(scoped_refptr<device::HidDeviceInfo> device)
  nit: explicit keyword needed when there is a single argument to a constructor.
- Patch Set #7, Line 81: static_cast<int>
  I don't believe this static_cast is necessary.
- Patch Set #7, Line 88:
```
    for (size_t i = 0; i < sizeof(kExpected) - 1; ++i) {
      if (buffer[i + 1] != kExpected[i]) {
```
  Why not use memcmp as before?
- Patch Set #7, Line 101: static_cast<int>
  If you make expected_report_id a uint8_t then this static_cast is unnecessary.
- Patch Set #7, Line 124:
```
    int report_id = static_cast<int>(buffer[0]);
    int expected_report_id = device_->has_report_id() ? 1 : 0;
    if (report_id != expected_report_id) {
      std::move(callback).Run(false);
      return;
    }

    for (size_t i = 0; i < sizeof(kExpected) - 1; ++i) {
      if (buffer[i + 1] != kExpected[i]) {
        std::move(callback).Run(false);
        return;
      }
    }
```
  Same comments here as in Write().
- Patch Set #7, Line 174: DCHECK(0);
  NOTREACHED();
- Patch Set #7, Line 183: auto binding =
  No need to save the return value of this call.
File extensions/browser/api/hid/hid_device_manager.cc:
- Patch Set #7, Line 280: // Init enumeration and set self as a client of HidManager.
  This comment is unnecessary.
- Patch Set #7, Line 283:
```
  // Binds the |hid_manager_| no matter the mojo connection will be created
  // or not. So it is always safe to use the |hid_manager_| after initialize
  // although it might don't work.
```
  |hid_manager_| is initialized and safe to use whether or not the connection is successful.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 8, 2017, 3:30:22 AM9/8/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Thanks! CL updated.

17 comments:

File device/hid/hid_connection_impl.h:
- Patch Set #7, Line 24: ReadCallback callback) override;
  Here and below for the other callbacks "device::mojom::HidConnection" is not necessary because this […]
  Oh, Sorry.
  Done.
File device/hid/hid_connection_impl.cc:
- Patch Set #7, Line 25: HECK(hid_connection_);

- Same here in the .cc file.

- Patch Set #7, Line 207:
  nit: no braces for single-line if here and below

- Done

- Patch Set #7, Line 218:

- service_manager::Connector
  Done
- Patch Set #7, Line 220: mojo::MakeRequest(&hid_manager_));

- nit: no space required here, |connector| is defined specifically for this line.

- Done

- Patch Set #7, Line 289:
  nit: no braces for single-line for loop

- Done
File extensions/browser/api/hid/hid_api.cc:
- Patch Set #7, Line 289: auto* data =

- auto if that allows this to fit on one line

- Done
- Patch Set #7, Line 356: buffer.insert
  same here
  Done
File extensions/browser/api/hid/hid_apitest.cc:
- Patch Set #7, Line 56: explicit FakeHidConnectionImpl(scoped_refptr<device::HidDeviceInfo> device)

- nit: explicit keyword needed when there is a single argument to a constructor.

- Done
- Patch Set #7, Line 81: buffer[0];

- I don't believe this static_cast is necessary.

- Done

- Patch Set #7, Line 88:

    if (memcmp(buffer.data() + 1, kExpected, sizeof(kExpected) - 1) != 0) {
      std::move(callback).Run(false);

- Why not use memcmp as before?

- Done
- Patch Set #7, Line 101: d::move(callback

- If you make expected_report_id a uint8_t then this static_cast is unnecessary.

- Done

- Patch Set #7, Line 124:

    int expected_report_id = device_->has_report_id() ? 1 : 0;


    if (report_id != expected_report_id) {
      std::move(callback).Run(false);
      return;
    }

    if (memcmp(buffer.data() + 1, kExpected, sizeof(kExpected) - 1) != 0) {
      std::move(callback).Run(false);
      return;
    }

    std::move(callback).Run(true);
  }

- Same comments here as in Write().

- Done

- Patch Set #7, Line 174:

- NOTREACHED();
  Done
- Patch Set #7, Line 183: std::move(call

- No need to save the return value of this call.

- Done. And same in HidManagerImpl.
File extensions/browser/api/hid/hid_device_manager.cc:
- Patch Set #7, Line 280: DCHECK(!hid_manager_);
  This comment is unnecessary.
  Done

- Patch Set #7, Line 283:

  // connection is successful.
  device::mojom::HidManagerRequest request = mojo::MakeRequest(&hid_manager_);
  device::mojom::HidManagerClientA

- |hid_manager_| is initialized and safe to use whether or not the connection is successful.

- Thanks! Done.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 8, 2017, 1:48:32 PM9/8/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek

Patch set 8:Code-Review +1

3 comments:

File device/hid/hid_connection_impl.h:
- Patch Set #8, Line 16: asks the HidConnection to delegate
  delegates to HidConnection
- Patch Set #8, Line 23: implemenation
  implementation
File device/hid/hid_connection_impl.cc:
- Patch Set #8, Line 19: connection
  hid_connection_

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 8, 2017, 8:16:37 PM9/8/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, John Abd-El-Malek, Tom Sepez, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Hi, John, PTAL on //content/, Tom, PTAL on the mojo security.
Thanks all!

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 8, 2017, 8:16:37 PM9/8/17

to Tom Sepez, John Abd-El-Malek, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant

Ke He would like Tom Sepez and John Abd-El-Malek to review this change.

27 files changed, 737 insertions(+), 473 deletions(-)

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 8, 2017, 8:37:15 PM9/8/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, John Abd-El-Malek, Tom Sepez, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

3 comments:

File device/hid/hid_connection_impl.h:
- Patch Set #8, Line 16: delegates to HidConnection the rea
  delegates to HidConnection
  Done
- Patch Set #8, Line 23: implementatio
  implementation
  Done
File device/hid/hid_connection_impl.cc:
- Patch Set #8, Line 19: hid_connec
  hid_connection_
  Done

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Tom Sepez (Gerrit)

unread,

Sep 11, 2017, 6:51:26 PM9/11/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Colin Blundell, John Abd-El-Malek, Reilly Grant, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

1 comment:

File device/hid/public/interfaces/hid.mojom:
- Patch Set #9, Line 63:
  Is this returned packed by the device in this manner, or are we shuffling bytes around to do this ourselves? If the later, can we strip this out into a separate argument?

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 11, 2017, 7:25:56 PM9/11/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, John Abd-El-Malek, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

1 comment:

File device/hid/public/interfaces/hid.mojom:

- Patch Set #9, Line 63:

- Is this returned packed by the device in this manner, or are we shuffling bytes around to do this ou […]
  If the device sends one the report ID is packed by like this on the wire. For consistency between platforms (some of which pack it in themselves) the C++ interface always includes it in the buffer for consistency and to reduce the number of places where we need to expand or shrink the buffer in order to add or remove it.
  The chrome.hid extension API always handles it as a separate parameter since buffers needed to be copied on that boundary anyways. We could do either here since Mojo represents a similar boundary.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

John Abd-El-Malek (Gerrit)

unread,

Sep 11, 2017, 9:11:42 PM9/11/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 8:

Hi, John, PTAL on //content/, Tom, PTAL on the mojo security.
Thanks all!

You'll need a security reviewer for the manifest change in content.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 11, 2017, 10:40:50 PM9/11/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, John Abd-El-Malek, Reilly Grant, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 9:

Patch Set 8:
Hi, John, PTAL on //content/, Tom, PTAL on the mojo security.
Thanks all!
You'll need a security reviewer for the manifest change in content.

John, Thanks! Tom, PTAL on the manifest changes in content too, thanks!

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 12, 2017, 2:48:10 AM9/12/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, John Abd-El-Malek, Reilly Grant, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 9:
(1 comment)

Hi, Tom and Reilly, Thanks!

To separate the report_id as a parameter in mojo boundary or in extension boundary, I feel that both way are ok.

I think it might be better to keep the old interface so this CL can mainly focus on the mojofication. Even if we decide to go with the former way(if necessary), we can still do it in another CL.

WDYT? Thanks.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 12, 2017, 3:00:47 AM9/12/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, John Abd-El-Malek, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 9:

Patch Set 9:
(1 comment)
Hi, Tom and Reilly, Thanks!
To separate the report_id as a parameter in mojo boundary or in extension boundary, I feel that both way are ok.
I think it might be better to keep the old interface so this CL can mainly focus on the mojofication. Even if we decide to go with the former way(if necessary), we can still do it in another CL.
WDYT? Thanks.

I'd rather keep this as-is for now and evaluate afterwards whether changing the interface would simplify any implementation details. I went back and forth on this a few years ago and settled on the current interface as the best option.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 13, 2017, 6:37:52 AM9/13/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, John Abd-El-Malek, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 9:
Patch Set 9:
Patch Set 9:
(1 comment)
Hi, Tom and Reilly, Thanks!
To separate the report_id as a parameter in mojo boundary or in extension boundary, I feel that both way are ok.
I think it might be better to keep the old interface so this CL can mainly focus on the mojofication. Even if we decide to go with the former way(if necessary), we can still do it in another CL.
WDYT? Thanks.
I'd rather keep this as-is for now and evaluate afterwards whether changing the interface would simplify any implementation details. I went back and forth on this a few years ago and settled on the current interface as the best option.

Hi, Tom, friendly ping:)

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Tom Sepez (Gerrit)

unread,

Sep 13, 2017, 12:18:45 PM9/13/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, John Abd-El-Malek, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

1 comment:

File device/hid/public/interfaces/hid.mojom:

- Patch Set #9, Line 63:

- If the device sends one the report ID is packed by like this on the wire. […]
  Ok, if it's not too much trouble, let's split it out for clarity rather than commenting about it. Thanks.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 13, 2017, 9:55:40 PM9/13/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Tom Sepez, Reilly Grant, John Abd-El-Malek, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 9:
Patch Set 9:
Patch Set 9:
(1 comment)
Hi, Tom and Reilly, Thanks!
To separate the report_id as a parameter in mojo boundary or in extension boundary, I feel that both way are ok.
I think it might be better to keep the old interface so this CL can mainly focus on the mojofication. Even if we decide to go with the former way(if necessary), we can still do it in another CL.
WDYT? Thanks.
I'd rather keep this as-is for now and evaluate afterwards whether changing the interface would simplify any implementation details. I went back and forth on this a few years ago and settled on the current interface as the best option.

Hi, Tom, see above the discussions, we think maybe it is better to evaluate later. Do you think it's ok? Thanks.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 13, 2017, 10:36:12 PM9/13/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, John Abd-El-Malek, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
(1 comment)
Hi, Tom and Reilly, Thanks!
To separate the report_id as a parameter in mojo boundary or in extension boundary, I feel that both way are ok.
I think it might be better to keep the old interface so this CL can mainly focus on the mojofication. Even if we decide to go with the former way(if necessary), we can still do it in another CL.
WDYT? Thanks.
I'd rather keep this as-is for now and evaluate afterwards whether changing the interface would simplify any implementation details. I went back and forth on this a few years ago and settled on the current interface as the best option.
Hi, Tom, see above the discussions, we think maybe it is better to evaluate later. Do you think it's ok? Thanks.

Given the existing copies between std::vector<uint8_t> and net::IOBuffer making this change in this patch should not be difficult. Please give it a try.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 14, 2017, 1:39:08 AM9/14/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, John Abd-El-Malek, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Hi, Reilly and Tom, I split out the report_id in mojom interface. PTAL. Thanks!

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 14, 2017, 2:37:21 AM9/14/17

to Tom Sepez, Reilly Grant, John Abd-El-Malek, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Darin Fisher, Colin Blundell, chromium...@chromium.org, Commit Bot, Adam Barth, Aaron Boodman

Ke He uploaded patch set #16 to this change.

Mojofication on //device/hid.


In this CL:
1) Add the HidManager and HidConnection interfaces in hid.mojom, and
   implements those mojo interfaces in //device/hid.

2) Convert the clients in //extensions to use hid mojo interfaces.

3) Change the type of HidUsageAndPage::usage_page to uint16_t.

4) Rewrite the hid_apitest base on the new added mojo interfaces.

TODO:
1) Move client library files into device/hid/public/cpp.
2) Host HidService by DeviceService instead of DeviceClient.
3) Mojofy //device/u2f Or just move it into DeviceService.

BUG=728223

27 files changed, 734 insertions(+), 471 deletions(-)

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 14, 2017, 2:54:37 AM9/14/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, John Abd-El-Malek, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Hi, Sorry uploaded many useless patchsets, the "git cl upload" complains "Error after CL description prompt ...". In PatchSet 10 I split the report_id out from buffer in mojom interface.

Finally I failed to submit another CL which is for converting //device/u2f because of above error. I'll submit it after this CL gets landed.

Thanks!

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Tom Sepez (Gerrit)

unread,

Sep 14, 2017, 12:41:19 PM9/14/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch set 17:Code-Review +1

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reilly Grant (Gerrit)

unread,

Sep 14, 2017, 2:07:37 PM9/14/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

2 comments:

File device/hid/hid_connection_impl.cc:
- Patch Set #17, Line 41: size
  size - 1
File extensions/browser/api/hid/hid_api.cc:
- Patch Set #17, Line 262: DCHECK_GE(buffer->size(), 0u);
  This check is not particularly useful anymore.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 14, 2017, 8:33:49 PM9/14/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

1 comment:

File device/hid/hid_connection_impl.cc:

- Patch Set #17, Line 41: size
  size - 1

- nop, not size-1, should be size.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 14, 2017, 8:34:14 PM9/14/17

to chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Patch set 17:Commit-Queue +2

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Sep 14, 2017, 8:41:27 PM9/14/17

to Ke He, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

Commit Bot merged this change.

Approvals:
  Reilly Grant: Looks good to me
  Tom Sepez: Looks good to me
  Ke He: Commit

Mojofication on //device/hid.



In this CL:
1) Add the HidManager and HidConnection interfaces in hid.mojom, and
   implements those mojo interfaces in //device/hid.

2) Convert the clients in //extensions to use hid mojo interfaces.

3) Change the type of HidUsageAndPage::usage_page to uint16_t.

4) Rewrite the hid_apitest base on the new added mojo interfaces.

TODO:
1) Move client library files into device/hid/public/cpp.
2) Host HidService by DeviceService instead of DeviceClient.
3) Mojofy //device/u2f Or just move it into DeviceService.

BUG=728223

Change-Id: I1de1b5211f7ef37f54d4bc18c7995a3b47cc7da2

Reviewed-on: https://chromium-review.googlesource.com/648949
Reviewed-by: Tom Sepez <tse...@chromium.org>
Reviewed-by: Reilly Grant <rei...@chromium.org>
Commit-Queue: Ke He <ke...@intel.com>
Cr-Commit-Position: refs/heads/master@{#502109}

27 files changed, 734 insertions(+), 471 deletions(-)

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Ke He (Gerrit)

unread,

Sep 14, 2017, 8:42:55 PM9/14/17

to Commit Bot, chromium-a...@chromium.org, dari...@chromium.org, extension...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, mac-r...@chromium.org, yzshen...@chromium.org, Reilly Grant, Tom Sepez, Colin Blundell, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher

1 comment:

File extensions/browser/api/hid/hid_api.cc:

- Patch Set #17, Line 262: DCHECK_GE(buffer->size(), 0u);
  This check is not particularly useful anymore.

- oh, I forgot to remove this. Will do in next CL.

To view, visit change 648949. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu