Clean up public interface of AttachmentBrokerUnprivileged.

Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}

Committed: https://crrev.com/8666287a9b42256b20692765f43c12a960398699
Cr-Commit-Position: refs/heads/master@{#375776}

[erikchen, 2016-02-08 02:06:09]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 02:06:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679763002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679763002/1

[commit-bot: I haz the power, 2016-02-08 03:12:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 03:12:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2016-02-08 18:17:30]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 18:19:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679763002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679763002/20001

[commit-bot: I haz the power, 2016-02-08 21:41:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 21:41:59]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-02-08 23:52:41]

Description was changed from

==========
Clean up attachment brokering.

BUG=
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

[erikchen, 2016-02-08 23:52:55]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

[erikchen, 2016-02-08 23:53:42]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2016-02-08 23:53:43]

tsepez: Please review ipc/

[Tom Sepez, 2016-02-09 00:26:19]

lgtm

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker.h
File ipc/attachment_broker.h (right):

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker.h...
ipc/attachment_broker.h:104: // Whether the process's broker is privileged.
nit: True iff the process's broker is privileged.
(Just to be clear on the interpretation of the return value).

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker.h...
ipc/attachment_broker.h:105: virtual bool IsPrivilegedBroker();
Nit: maybe HasPrivilegedBroker(), because this process doesn't have an IS_A
relationship with broker classes.

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker_u...
File ipc/attachment_broker_unprivileged.h (right):

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker_u...
ipc/attachment_broker_unprivileged.h:30: // AttachmentBroker overrides.
nit:  the convention is just to write

// AttachmentBroker:

[erikchen, 2016-02-09 02:38:51]

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker.h
File ipc/attachment_broker.h (right):

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker.h...
ipc/attachment_broker.h:104: // Whether the process's broker is privileged.
On 2016/02/09 00:26:19, Tom Sepez wrote:
> nit: True iff the process's broker is privileged.
> (Just to be clear on the interpretation of the return value).

Done.

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker.h...
ipc/attachment_broker.h:105: virtual bool IsPrivilegedBroker();
On 2016/02/09 00:26:19, Tom Sepez wrote:
> Nit: maybe HasPrivilegedBroker(), because this process doesn't have an IS_A
> relationship with broker classes.

My initial comment was inaccurate in its use of the word "process's broker".
This is a IS_A relationship, since this is an instance method on
AttachmentBroker.

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker_u...
File ipc/attachment_broker_unprivileged.h (right):

https://codereview.chromium.org/1679763002/diff/20001/ipc/attachment_broker_u...
ipc/attachment_broker_unprivileged.h:30: // AttachmentBroker overrides.
On 2016/02/09 00:26:19, Tom Sepez wrote:
> nit:  the convention is just to write
> 
> // AttachmentBroker:

Done.

[erikchen, 2016-02-09 02:39:33]

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

[erikchen, 2016-02-09 02:39:34]

avi: Please review content/

[Avi (use Gerrit), 2016-02-09 02:43:50]

lgtm

[erikchen, 2016-02-10 18:25:37]

erikchen@chromium.org changed reviewers:
+ mseaborn@chromium.org, sergeyu@chromium.org

[erikchen, 2016-02-10 18:25:38]

mseaborn: Please review components/nacl/
sergeyu: Please review remoting/host/

[Mark Seaborn, 2016-02-10 19:35:55]

On 2016/02/10 18:25:38, erikchen wrote:
> mseaborn: Please review components/nacl/

LGTM

[Sergey Ulanov, 2016-02-10 19:36:35]

remoting lgtm

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.cc (right):

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:151: IPC::AttachmentBroker* global =
IPC::AttachmentBroker::GetGlobal();
nit: call this broker instead of global.

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:543: IPC::AttachmentBroker* global =
IPC::AttachmentBroker::GetGlobal();
nit: broker

[erikchen, 2016-02-10 21:03:36]

Patchset #4 (id:60001) has been deleted

[erikchen, 2016-02-10 21:04:46]

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.cc (right):

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.cc:151: IPC::AttachmentBroker* global =
IPC::AttachmentBroker::GetGlobal();
On 2016/02/10 19:36:35, Sergey Ulanov wrote:
> nit: call this broker instead of global.

Done.

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/1679763002/diff/40001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:543: IPC::AttachmentBroker* global =
IPC::AttachmentBroker::GetGlobal();
On 2016/02/10 19:36:35, Sergey Ulanov wrote:
> nit: broker

Done.

[erikchen, 2016-02-10 21:04:50]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-02-10 21:04:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
mseaborn@chromium.org, avi@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1679763002/#ps80001
(title: "Comments from sergeyu.")

[commit-bot: I haz the power, 2016-02-10 21:09:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679763002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679763002/80001

[commit-bot: I haz the power, 2016-02-10 22:46:33]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

[commit-bot: I haz the power, 2016-02-10 22:46:33]

Committed patchset #4 (id:80001)

[erikchen, 2016-02-10 23:08:38]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/1688433005/ by erikchen@chromium.org.

The reason for reverting is:
c:\b\build\slave\win\build\src\remoting\host\remoting_me2me_host.cc(937) : error
C2065: 'attachment_broker_' : undeclared identifier

http://build.chromium.org/p/chromium/builders/Win/builds/40168.

[erikchen, 2016-02-16 19:12:22]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

[erikchen, 2016-02-16 19:14:50]

tsepez: PTAL

I added a method DeregisterBrokerCommunicationChannel to clear the channel, so
that we don't leave around a raw pointer to a destroyed object.

[erikchen, 2016-02-16 19:14:56]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-16 19:16:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679763002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679763002/100001

[Tom Sepez, 2016-02-16 19:32:13]

++lgm

[Tom Sepez, 2016-02-16 19:32:34]

On 2016/02/16 19:32:13, Tom Sepez wrote:
> ++lgm
++lgtm that is.

[commit-bot: I haz the power, 2016-02-16 19:40:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-16 19:40:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-02-16 20:36:25]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-02-16 20:36:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, avi@chromium.org, sergeyu@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1679763002/#ps120001
(title: "Rebase. Also fix compile errors in unit tests.")

[commit-bot: I haz the power, 2016-02-16 20:37:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679763002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679763002/120001

[commit-bot: I haz the power, 2016-02-16 22:01:04]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

[commit-bot: I haz the power, 2016-02-16 22:01:05]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-02-16 22:31:58]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/d00f5b6987750d39a5cc4859ee19c9ca89ed0cd9
Cr-Commit-Position: refs/heads/master@{#374759}
==========

[commit-bot: I haz the power, 2016-02-16 22:31:58]

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

[commit-bot: I haz the power, 2016-02-16 22:55:12]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/d00f5b6987750d39a5cc4859ee19c9ca89ed0cd9
Cr-Commit-Position: refs/heads/master@{#374759}
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}
==========

[commit-bot: I haz the power, 2016-02-16 22:55:12]

Patchset 6 (id:??) landed as
https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}

[erikchen, 2016-02-17 01:20:26]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1704743002/ by erikchen@chromium.org.

The reason for reverting is: failures on Chromium Memory FYI:

http://build.chromium.org/p/chromium.memory.fyi/
Failure notification for "memory test: remoting" on "Chromium Mac
(valgrind)(2)".
Please see if the failures are related to your commit and take appropriate
actions (e.g. revert, update suppressions, notify sheriff, etc.).

For more info on the memory waterfall please see these links:
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
http://dev.chromium.org/developers/how-tos/using-valgrind
http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer

By the way, the current memory sheriff is on the CC list.

http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28....

[erikchen, 2016-02-17 02:33:29]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}
==========

[erikchen, 2016-02-17 02:38:40]

On 2016/02/17 01:20:26, erikchen wrote:
> A revert of this CL (patchset #6 id:120001) has been created in
> https://codereview.chromium.org/1704743002/ by mailto:erikchen@chromium.org.
> 
> The reason for reverting is: failures on Chromium Memory FYI:
> 
> http://build.chromium.org/p/chromium.memory.fyi/
> Failure notification for "memory test: remoting" on "Chromium Mac
> (valgrind)(2)".
> Please see if the failures are related to your commit and take appropriate
> actions (e.g. revert, update suppressions, notify sheriff, etc.).
> 
> For more info on the memory waterfall please see these links:
>
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
> http://dev.chromium.org/developers/how-tos/using-valgrind
> http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer
> 
> By the way, the current memory sheriff is on the CC list.
> 
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28....

I tried to repro the Mac ASAN CHECK failure locally, but couldn't. I'm just
removing the CHECK, because it only ever gets triggered in ASAN tests, and it
appears to do so flakily:
https://code.google.com/p/chromium/issues/detail?id=534539.

[erikchen, 2016-02-17 02:38:50]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-02-17 02:38:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
mseaborn@chromium.org, avi@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1679763002/#ps140001
(title: "Remove CHECK.")

[commit-bot: I haz the power, 2016-02-17 02:40:32]

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

[commit-bot: I haz the power, 2016-02-17 04:09:27]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}
==========

[commit-bot: I haz the power, 2016-02-17 04:09:28]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-02-17 04:12:28]

Description was changed from

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}
==========

to

==========
Clean up public interface of AttachmentBrokerUnprivileged.

In the old interface, a static factory method returns a scoped_ptr, and the
caller had to manage the lifetime. Since this is a global object with minimal
memory footprint, and is required to outlive every IPC::Channel, it's much
easier for the global to never be destroyed. This also matches the interface for
AttachmentBrokerPrivileged.

BUG=584297

Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4
Cr-Commit-Position: refs/heads/master@{#375674}

Committed: https://crrev.com/8666287a9b42256b20692765f43c12a960398699
Cr-Commit-Position: refs/heads/master@{#375776}
==========

[commit-bot: I haz the power, 2016-02-17 04:12:29]

Patchset 7 (id:??) landed as
https://crrev.com/8666287a9b42256b20692765f43c12a960398699
Cr-Commit-Position: refs/heads/master@{#375776}