iOS Chromoting Client

iOS Chromoting Client

Code drop from Fusion. Integration with gyp will follow.

BUG=331356

[aboone, 2014-03-06 18:36:42]

iOS Chromoting, I'll send out a separate email with the packaged app and other
details.

[Sergey Ulanov, 2014-03-07 01:23:46]

+dcaiafa

[dcaiafa, 2014-03-19 01:14:14]

First pass.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/Chromoting/main.mm
File remoting/ios/Chromoting/main.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/Chromoting/main...
remoting/ios/Chromoting/main.mm:19: // namespace google_apis, which has our
clientid and client secrets, has some
nit: Is this comment still relevant? If not, please remove.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
File remoting/ios/bridge/client_bridge.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.h:32: class ClientBridge : public
base::RefCounted<ClientBridge> {
Does it really need to be ref-counted? I would prefer if it had a clear owner,
like ClientController. See client_instance.h for comments on an a circular
reference between ClientBridge and ClientInstance.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.h:39: virtual ~ClientBridge();
Make destructor private, and friend base::RefCounted<ClientBridge>. See
ref_counted.h for details.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.h:89: // Updates cursor shape. Call on display
thread.
nit: There doesn't seem to be a display thread like in the Android counterpart.
Should it be the UI thread instead? Here and below.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
File remoting/ios/bridge/client_bridge.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:46:
base::Bind(&ClientBridge::DetachFromVmAndSignal,
DetachFromVmAndSignal just signals the event. Do we need this at all?

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:50: done_event.Wait();
Remove duplicate.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:53: // CLIENT is requesting a new
connection to a HOST
nit: method description comments belong in the header. Here and below.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:83: // HOST reporting connection status
nit: empty line between methods.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:88: if (controller_) {
Is it really valid for controller_ to be NULL here? If not, please add a DCHECK
and remove the if-block, here and below.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:160: return
base::Bind(&ClientBridge::RedrawCanvas, base::Unretained(this));
Delete this method (see comments in client_instance.cc).

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
File remoting/ios/bridge/client_controller.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.h:24: -
(void)requestHostPin:(BOOL)pairingSupported;
nit: Add blank line between methods.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.h:45: // implimented class ClientBridge to
process input/output with the
nit: spelling.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
File remoting/ios/bridge/client_controller.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:12: // The value indicateing a
successful connection has been established via a call
nit: spelling

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:26: @implementation ClientController
nit: move class description to header.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:97: -
(void)commitPairinedentials:(NSString*)hostId
method name spelling

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:160: // HOST reporting connection
status.  Translate the |state| and |error| into a
Please move method descriptions to header.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
File remoting/ios/bridge/client_instance.cc (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:107: if
(!bridge_->network_task_runner()->BelongsToCurrentThread()) {
Move this to the top of the function.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:167: base::Unretained(bridge_.get()),
ClientBridge is currently RefCounted, so there shouldn't be any reason to use
Unretained here. However, if you decide to make ClientBridge non-refcounted as I
suggested earlier, then the pattern I suggest would be the following:

<Here> {
  PostTask(&ClientInstance::ReportConnectionStatus, this, ...);
}

ClientInstance::ReportConnectionStatus {
  DCHECK(ui_task_runner()->BelongsToCurrentThread());
  if (bridge_) {
    bridge_->ReportConnectionStatus(...);
  }
}

Cleanup() {
  bridge_ = NULL;
  ...
}

This way it's guaranteed that ClientBridge will not receive any more callbacks
after calling session_->Cleanup();

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:194: base::Unretained(bridge_.get()),
Same as OnConnectionState().

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:240: new
FrameConsumerBridge(bridge_->BindToFrameConsumerBridgeCallback()));
Create the binding on site instead of using a helper method.
i.e. base::Bind(&ClientBridge::RedrawCanvas, bridge_)

Also, same comments as in OnConnectionState applies.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:241: view_weak_factory_.reset(
Move the WeakPtrFactory into FrameConsumerBridge, or have it extend
SupportsWeakPtr.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
File remoting/ios/bridge/client_instance.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.h:37: // The instance does not take
ownership of |objc_runtime|. To connect with an
s/objc_runtime/bridge/

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.h:37: // The instance does not take
ownership of |objc_runtime|. To connect with an
ClientInstance does take ownership of bridge which is not reflected in the
signature - it should use scoped_refptr if that's really the intention.

However, there seems to be a circular reference between ClientBridge and
ClientInstance. Maybe the intention was to not take a reference on ClientBridge?
If so, remove the scoped_refptr from bridge_.

Going even further, does ClientBridge really need to be ref-countable?

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.h:111: // This group of variables is to be
used on the display thread.
s/display/UI

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
File remoting/ios/bridge/frame_consumer_bridge.cc (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.cc:38: if (frame_producer_) {
Is it valid for frame_producer_ to be NULL? If not, add a DCHECK and remove
if-block, here and below.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.cc:39:
frame_producer_->DrawBuffer(buffer);
I don't think you can reuse the buffer here. In the current setup, won't the
callback process the buffer asynchronously? The callback is the new owner of the
buffer, and its signature should be updated to reflect that (use scoped_ptr).

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.cc:56: AllocateBuffer();
The name of the method is confusing. It's main purpose is not to allocate
buffers. Maybe rename it to "DrawBuffer"?

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
File remoting/ios/bridge/frame_consumer_bridge.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.h:19: // FrameConsumer implementation
that draws onto a byte buffer.
Description doesn't match functionality.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.h:22: // The instance does not take
ownership of |objc_runtime|.
Comment doesn't match signature.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
File remoting/ios/bridge/frame_consumer_bridge_callback.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge_callback.h:14:
FrameConsumerBridgeCallback;
Scope this typedef into FrameConsumerBridge, rename it "Callback" (or maybe
"OnFrameCallback", and delete this file.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/data_store.mm
File remoting/ios/data_store.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/data_store.mm#n...
remoting/ios/data_store.mm:27: ^{ sharedStore = [[super allocWithZone:nil]
init]; });
Why allocWithZone instead of just alloc?

https://codereview.chromium.org/186733007/diff/1/remoting/ios/data_store.mm#n...
remoting/ios/data_store.mm:58: @{
Maybe use a intermediate local variable for this NSDictionary to improve
readability, here and below.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/host_refresh.mm
File remoting/ios/host_refresh.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/host_refresh.mm...
remoting/ios/host_refresh.mm:28: // createing an oAuth2 request to google api's
including the needed scopes to
nit: spelling

https://codereview.chromium.org/186733007/diff/1/remoting/ios/host_refresh.mm...
remoting/ios/host_refresh.mm:40: [NSString
stringWithUTF8String:remoting::ServiceUrls::GetInstance()
Store intermediate values in locals to improve readability, here and below.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_input.mm
File remoting/ios/key_input.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_input.mm#ne...
remoting/ios/key_input.mm:47: // When inserting multiple charactors, procces
them one at a time.  |text| is as
nit: spelling

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_input.mm#ne...
remoting/ios/key_input.mm:68: // The delivery may be an upper case or specicial
charactor.  Chromoting is just
nit: spelling

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_map_us.h
File remoting/ios/key_map_us.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_map_us.h#ne...
remoting/ios/key_map_us.h:149: const uint32_t kKeyCodeUS[] = {0,  // [0]     
Numbered by index, not by count
Use same indenting as in kIsShiftRequiredUS.

[dcaiafa, 2014-03-19 17:42:20]

+hbengali@

[aboone, 2014-03-21 16:42:07]

I've submitted an update.  I've started to implement more of the unit tests. 
Particularly in the bridge folder, I decided testing was going to be too
difficult with the previous class structure.  I refactored to something easier
to segregate the different layers.  The client to host interface (ClientProxy),
the host to client interface(HostProxy), and the thread/protocol
manager(ClientInstance).  This removed some more redundant code, and was able to
segregate the c++ from the objc a bit better.  I will probably do more of this
type of work as I implement unit tests of the other classes, particularly the
host_view_controller should be broken out to segregate all the positioning and
drawing support functions from the controller.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/Chromoting/main.mm
File remoting/ios/Chromoting/main.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/Chromoting/main...
remoting/ios/Chromoting/main.mm:19: // namespace google_apis, which has our
clientid and client secrets, has some
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: Is this comment still relevant? If not, please remove.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
File remoting/ios/bridge/client_bridge.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.h:32: class ClientBridge : public
base::RefCounted<ClientBridge> {
On 2014/03/19 01:14:15, dcaiafa wrote:
> Does it really need to be ref-counted? I would prefer if it had a clear owner,
> like ClientController. See client_instance.h for comments on an a circular
> reference between ClientBridge and ClientInstance.

Changed to a SupportWeakPtr<>

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.h:39: virtual ~ClientBridge();
On 2014/03/19 01:14:15, dcaiafa wrote:
> Make destructor private, and friend base::RefCounted<ClientBridge>. See
> ref_counted.h for details.

Requirements after change to using a WeakPtr are different, so I don't think
this comment is relevent now

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.h:89: // Updates cursor shape. Call on display
thread.
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: There doesn't seem to be a display thread like in the Android
counterpart.
> Should it be the UI thread instead? Here and below.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
File remoting/ios/bridge/client_bridge.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:46:
base::Bind(&ClientBridge::DetachFromVmAndSignal,
On 2014/03/19 01:14:15, dcaiafa wrote:
> DetachFromVmAndSignal just signals the event. Do we need this at all?

Removed.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:50: done_event.Wait();
On 2014/03/19 01:14:15, dcaiafa wrote:
> Remove duplicate.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:53: // CLIENT is requesting a new
connection to a HOST
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: method description comments belong in the header. Here and below.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:83: // HOST reporting connection status
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: empty line between methods.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:88: if (controller_) {
On 2014/03/19 01:14:15, dcaiafa wrote:
> Is it really valid for controller_ to be NULL here? If not, please add a
DCHECK
> and remove the if-block, here and below.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_b...
remoting/ios/bridge/client_bridge.mm:160: return
base::Bind(&ClientBridge::RedrawCanvas, base::Unretained(this));
On 2014/03/19 01:14:15, dcaiafa wrote:
> Delete this method (see comments in client_instance.cc).

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
File remoting/ios/bridge/client_controller.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.h:24: -
(void)requestHostPin:(BOOL)pairingSupported;
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: Add blank line between methods.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.h:45: // implimented class ClientBridge to
process input/output with the
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: spelling.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
File remoting/ios/bridge/client_controller.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:12: // The value indicateing a
successful connection has been established via a call
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: spelling

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:26: @implementation ClientController
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: move class description to header.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:97: -
(void)commitPairinedentials:(NSString*)hostId
On 2014/03/19 01:14:15, dcaiafa wrote:
> method name spelling

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_c...
remoting/ios/bridge/client_controller.mm:160: // HOST reporting connection
status.  Translate the |state| and |error| into a
On 2014/03/19 01:14:15, dcaiafa wrote:
> Please move method descriptions to header.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
File remoting/ios/bridge/client_instance.cc (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:107: if
(!bridge_->network_task_runner()->BelongsToCurrentThread()) {
On 2014/03/19 01:14:15, dcaiafa wrote:
> Move this to the top of the function.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:167: base::Unretained(bridge_.get()),
On 2014/03/19 01:14:15, dcaiafa wrote:
> ClientBridge is currently RefCounted, so there shouldn't be any reason to use
> Unretained here. However, if you decide to make ClientBridge non-refcounted as
I
> suggested earlier, then the pattern I suggest would be the following:
> 
> <Here> {
>   PostTask(&ClientInstance::ReportConnectionStatus, this, ...);
> }
> 
> ClientInstance::ReportConnectionStatus {
>   DCHECK(ui_task_runner()->BelongsToCurrentThread());
>   if (bridge_) {
>     bridge_->ReportConnectionStatus(...);
>   }
> }
> 
> Cleanup() {
>   bridge_ = NULL;
>   ...
> }
> 
> This way it's guaranteed that ClientBridge will not receive any more callbacks
> after calling session_->Cleanup();

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:194: base::Unretained(bridge_.get()),
On 2014/03/19 01:14:15, dcaiafa wrote:
> Same as OnConnectionState().

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:240: new
FrameConsumerBridge(bridge_->BindToFrameConsumerBridgeCallback()));
On 2014/03/19 01:14:15, dcaiafa wrote:
> Create the binding on site instead of using a helper method.
> i.e. base::Bind(&ClientBridge::RedrawCanvas, bridge_)
> 
> Also, same comments as in OnConnectionState applies.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.cc:241: view_weak_factory_.reset(
On 2014/03/19 01:14:15, dcaiafa wrote:
> Move the WeakPtrFactory into FrameConsumerBridge, or have it extend
> SupportsWeakPtr.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
File remoting/ios/bridge/client_instance.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.h:37: // The instance does not take
ownership of |objc_runtime|. To connect with an
On 2014/03/19 01:14:15, dcaiafa wrote:
> s/objc_runtime/bridge/

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.h:37: // The instance does not take
ownership of |objc_runtime|. To connect with an
On 2014/03/19 01:14:15, dcaiafa wrote:
> ClientInstance does take ownership of bridge which is not reflected in the
> signature - it should use scoped_refptr if that's really the intention.
> 
> However, there seems to be a circular reference between ClientBridge and
> ClientInstance. Maybe the intention was to not take a reference on
ClientBridge?
> If so, remove the scoped_refptr from bridge_.
> 
> Going even further, does ClientBridge really need to be ref-countable?

I've changed it to a WeakPtr to be explicit that bridge may not exist for the
lifetime of ClientInstance

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/client_i...
remoting/ios/bridge/client_instance.h:111: // This group of variables is to be
used on the display thread.
On 2014/03/19 01:14:15, dcaiafa wrote:
> s/display/UI

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
File remoting/ios/bridge/frame_consumer_bridge.cc (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.cc:38: if (frame_producer_) {
On 2014/03/19 01:14:15, dcaiafa wrote:
> Is it valid for frame_producer_ to be NULL? If not, add a DCHECK and remove
> if-block, here and below.

It can be NULL between a call to the constructor and a call to initialize.  But
I believe you're saying to disallow all workflows that would have that result,
which is understandable.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.cc:39:
frame_producer_->DrawBuffer(buffer);
On 2014/03/19 01:14:15, dcaiafa wrote:
> I don't think you can reuse the buffer here. In the current setup, won't the
> callback process the buffer asynchronously? The callback is the new owner of
the
> buffer, and its signature should be updated to reflect that (use scoped_ptr).

The callback is not a task post, its just an indirect function call which is
synchronous.  A series of functions ensues, and the buffer ends up at
[HostController applyFrame] which copies only the needed data so GL can access
it on the next paint.

The strategy you're envisioning uses more memory, but less memory writes.  If I
have time which I probably won't, I'll come back to this and try it out.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.cc:56: AllocateBuffer();
On 2014/03/19 01:14:15, dcaiafa wrote:
> The name of the method is confusing. It's main purpose is not to allocate
> buffers. Maybe rename it to "DrawBuffer"?

Renamed to DrawWithNewBuffer

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
File remoting/ios/bridge/frame_consumer_bridge.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.h:19: // FrameConsumer implementation
that draws onto a byte buffer.
On 2014/03/19 01:14:15, dcaiafa wrote:
> Description doesn't match functionality.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge.h:22: // The instance does not take
ownership of |objc_runtime|.
On 2014/03/19 01:14:15, dcaiafa wrote:
> Comment doesn't match signature.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
File remoting/ios/bridge/frame_consumer_bridge_callback.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/bridge/frame_co...
remoting/ios/bridge/frame_consumer_bridge_callback.h:14:
FrameConsumerBridgeCallback;
On 2014/03/19 01:14:15, dcaiafa wrote:
> Scope this typedef into FrameConsumerBridge, rename it "Callback" (or maybe
> "OnFrameCallback", and delete this file. 

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/data_store.mm
File remoting/ios/data_store.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/data_store.mm#n...
remoting/ios/data_store.mm:27: ^{ sharedStore = [[super allocWithZone:nil]
init]; });
On 2014/03/19 01:14:15, dcaiafa wrote:
> Why allocWithZone instead of just alloc?

allocWithZone has been overridden to always return the singleton.  If [super
alloc] was used then our version of allocWithZone would be called impliciently,
and an infinite loop would occur.

There is an explanation of it here
http://stackoverflow.com/questions/11962913/creating-a-singleton-with-allocwi...

https://codereview.chromium.org/186733007/diff/1/remoting/ios/data_store.mm#n...
remoting/ios/data_store.mm:58: @{
On 2014/03/19 01:14:15, dcaiafa wrote:
> Maybe use a intermediate local variable for this NSDictionary to improve
> readability, here and below. 

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/host_refresh.mm
File remoting/ios/host_refresh.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/host_refresh.mm...
remoting/ios/host_refresh.mm:28: // createing an oAuth2 request to google api's
including the needed scopes to
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: spelling

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/host_refresh.mm...
remoting/ios/host_refresh.mm:40: [NSString
stringWithUTF8String:remoting::ServiceUrls::GetInstance()
On 2014/03/19 01:14:15, dcaiafa wrote:
> Store intermediate values in locals to improve readability, here and below.

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_input.mm
File remoting/ios/key_input.mm (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_input.mm#ne...
remoting/ios/key_input.mm:47: // When inserting multiple charactors, procces
them one at a time.  |text| is as
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: spelling

I'm getting the point, I'll start using a spellchecker to review before
committing.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_input.mm#ne...
remoting/ios/key_input.mm:68: // The delivery may be an upper case or specicial
charactor.  Chromoting is just
On 2014/03/19 01:14:15, dcaiafa wrote:
> nit: spelling

Done.

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_map_us.h
File remoting/ios/key_map_us.h (right):

https://codereview.chromium.org/186733007/diff/1/remoting/ios/key_map_us.h#ne...
remoting/ios/key_map_us.h:149: const uint32_t kKeyCodeUS[] = {0,  // [0]     
Numbered by index, not by count
On 2014/03/19 01:14:15, dcaiafa wrote:
> Use same indenting as in kIsShiftRequiredUS.

Done.

[dcaiafa, 2014-05-02 16:06:38]

lgtm

[dcaiafa, 2014-05-06 21:19:59]

+ rvargas@ for +net/url_request in remoting/ios/bridge/DEPS approval

[rvargas (doing something else), 2014-05-06 21:34:42]

DEPS lgtm

[dcaiafa, 2014-05-06 21:38:54]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-05-06 21:39:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboone@fusionlabs.net/186733007/60001

[dcaiafa, 2014-05-06 21:50:06]

The CQ bit was unchecked by dcaiafa@chromium.org

[dcaiafa, 2014-05-06 22:59:39]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-05-06 23:02:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboone@fusionlabs.net/186733007/60001

[dcaiafa, 2014-05-06 23:16:25]

The CQ bit was unchecked by dcaiafa@chromium.org

[dcaiafa, 2014-05-15 17:41:39]

I'm closing this issue. The code landed in the following CL:
https://codereview.chromium.org/278863003/

which was a patch of this CL minus .xcodeproj.