Update ChannelReader to use AttachmentBroker.

Update ChannelReader to use AttachmentBroker.

Previously, translating from bytes into messages and dispatching the messages
was accomplished in a single step. I've split this into two steps. The function
TranslateInputData turns bytes into messages, and puts those messages in a
queue. The function DispatchMessages() dispatches messages from the queue.
Messages cannot be dispatched until all attachments have been brokered, so
DispatchMessages() will sometimes wait (asynchronously) for the attachments to
get brokered.

BUG=466437
Committed: https://crrev.com/de9412b8832db0c6e1e283e9f8fce53eb4fd4e67
Cr-Commit-Position: refs/heads/master@{#340509}

[erikchen, 2015-06-25 00:17:14]

Patchset #3 (id:40001) has been deleted

[erikchen, 2015-06-25 00:17:19]

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

[erikchen, 2015-06-25 01:35:26]

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

[erikchen, 2015-06-25 01:35:27]

tsepez: Please review.

This CL depends on 2 previous CLs:
1) "Stub in more IPC attachment brokering functionality."
2) "Make attachment broker a subclass of IPC::Listener."

[erikchen, 2015-07-13 17:29:24]

tsepez: Please review.

[Tom Sepez, 2015-07-13 18:28:48]

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker.cc
File ipc/attachment_broker.cc (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker....
ipc/attachment_broker.cc:14: void
AttachmentBroker::AddObserver(AttachmentBroker::Observer* observer) {
do we care if |observer| has already been added?

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker....
ipc/attachment_broker.cc:19: for (std::vector<Observer*>::iterator it =
observers_.begin();
can we use ::find here?  How about auto keyword to avoid complicated type name

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker_...
File ipc/attachment_broker_win.h (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker_...
ipc/attachment_broker_win.h:49: typedef
std::vector<scoped_refptr<BrokerableAttachment>> AttachmentVector;
nit: using is preferred over typedef.

https://codereview.chromium.org/1206093002/diff/100001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:40: bool BrokerableAttachment::NeedsBrokering()
const {
Any reason not to just make these inline in the header to maybe avoid a function
call?

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:90: if (!GetNonBrokeredAttachments(m))
Do we leak |m| here if we return early?

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:178: for (AttachmentIdSet::iterator it =
blocked_ids_.begin();
nit: find()

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader.h
File ipc/ipc_channel_reader.h (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.h:111: // Exposed for testing purposes only.
nit: maybe these go into a subclass in the test file, with the corresponding
fields being protected (rather than private) in this file.

[erikchen, 2015-07-17 01:27:46]

Patchset #6 (id:120001) has been deleted

[erikchen, 2015-07-17 18:44:01]

tsepez: PTAL

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker.cc
File ipc/attachment_broker.cc (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker....
ipc/attachment_broker.cc:14: void
AttachmentBroker::AddObserver(AttachmentBroker::Observer* observer) {
On 2015/07/13 18:28:47, Tom Sepez wrote:
> do we care if |observer| has already been added?

I added logic to prevent the same observer from being added twice.

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker....
ipc/attachment_broker.cc:19: for (std::vector<Observer*>::iterator it =
observers_.begin();
On 2015/07/13 18:28:47, Tom Sepez wrote:
> can we use ::find here?  How about auto keyword to avoid complicated type name

Used auto and find.

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker_...
File ipc/attachment_broker_win.h (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/attachment_broker_...
ipc/attachment_broker_win.h:49: typedef
std::vector<scoped_refptr<BrokerableAttachment>> AttachmentVector;
On 2015/07/13 18:28:47, Tom Sepez wrote:
> nit: using is preferred over typedef.

Done.

https://codereview.chromium.org/1206093002/diff/100001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:40: bool BrokerableAttachment::NeedsBrokering()
const {
On 2015/07/13 18:28:47, Tom Sepez wrote:
> Any reason not to just make these inline in the header to maybe avoid a
function
> call?

I try to keep as much code as possible out of header files. I claim that modern
compilers are smart enough that for optimized binaries, there is minimal
correlation between the location of the definition (.cc vs .h) and whether the
function ends up inlined.

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:90: if (!GetNonBrokeredAttachments(m))
On 2015/07/13 18:28:47, Tom Sepez wrote:
> Do we leak |m| here if we return early?

Yup. That's what I deserve for not using scoped_ptr. Fixed to use scoped_ptr.

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:178: for (AttachmentIdSet::iterator it =
blocked_ids_.begin();
On 2015/07/13 18:28:47, Tom Sepez wrote:
> nit: find()

Done.

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader.h
File ipc/ipc_channel_reader.h (right):

https://codereview.chromium.org/1206093002/diff/100001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.h:111: // Exposed for testing purposes only.
On 2015/07/13 18:28:47, Tom Sepez wrote:
> nit: maybe these go into a subclass in the test file, with the corresponding
> fields being protected (rather than private) in this file.

The Google C++ style guide indicates that data members must be private.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

[Tom Sepez, 2015-07-17 21:34:07]

lgtm

[erikchen, 2015-07-20 23:44:10]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-20 23:44:48]

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

[commit-bot: I haz the power, 2015-07-21 00:08:10]

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

[commit-bot: I haz the power, 2015-07-21 00:08:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-07-21 21:36:04]

Patchset #8 (id:180001) has been deleted

[erikchen, 2015-07-21 21:37:42]

tsepez: PTAL

Patch set 8 wraps calls to member functions of AttachmentBroker in compile time
conditionals to prevent the symbols referenced in the implementation of
AttachmentBroker from leaking into non-win targets (notably nacl_sfi and
nacl_non_sfi)

[Tom Sepez, 2015-07-21 22:13:07]

lgtm

[erikchen, 2015-07-21 22:19:57]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-21 22:20:44]

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

[commit-bot: I haz the power, 2015-07-21 23:52:44]

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

[commit-bot: I haz the power, 2015-07-21 23:52:45]

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, 2015-07-22 01:00:02]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-22 01:00:41]

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

[commit-bot: I haz the power, 2015-07-22 01:22:56]

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

[commit-bot: I haz the power, 2015-07-22 01:22:57]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-07-22 01:41:33]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-22 01:42:09]

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

[commit-bot: I haz the power, 2015-07-22 02:15:07]

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

[commit-bot: I haz the power, 2015-07-22 02:15:07]

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_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2015-07-22 18:05:02]

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

[commit-bot: I haz the power, 2015-07-22 18:05:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1206093002/220001

[erikchen, 2015-07-22 18:10:16]

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

[erikchen, 2015-07-22 18:10:25]

Patchset #9 (id:220001) has been deleted

[commit-bot: I haz the power, 2015-07-22 18:10:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1206093002/240001

[commit-bot: I haz the power, 2015-07-22 19:36:35]

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

[commit-bot: I haz the power, 2015-07-22 19:36:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-07-22 22:11:58]

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

[commit-bot: I haz the power, 2015-07-22 22:13:08]

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

[commit-bot: I haz the power, 2015-07-22 23:07:55]

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

[commit-bot: I haz the power, 2015-07-22 23:07:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-07-22 23:43:29]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-07-22 23:43:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1206093002/#ps280001
(title: "Add an optimization to immediately dispatch messages after
translation.")

[erikchen, 2015-07-22 23:43:30]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-07-22 23:43:42]

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

[commit-bot: I haz the power, 2015-07-22 23:45:18]

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

[commit-bot: I haz the power, 2015-07-23 00:19:14]

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

[commit-bot: I haz the power, 2015-07-23 00:19:15]

Dry run: This issue passed the CQ dry run.

[erikchen, 2015-07-23 00:34:37]

On 2015/07/23 00:19:15, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

tsepez: PTAL

(all CQ tests pass now. Somehow I failed to check this before).

The only major change since patch set 8 is that when Messages are translated and
placed in the queue for later dispatch, they are copied. I added in an
optimization that tries to immediately dispatch messages after translation to
avoid this copy (if possible).

[erikchen, 2015-07-24 00:23:41]

On 2015/07/23 00:34:37, erikchen wrote:
> On 2015/07/23 00:19:15, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> tsepez: PTAL
> 
> (all CQ tests pass now. Somehow I failed to check this before).
> 
> The only major change since patch set 8 is that when Messages are translated
and
> placed in the queue for later dispatch, they are copied. I added in an
> optimization that tries to immediately dispatch messages after translation to
> avoid this copy (if possible).

tsepez: Ping?

The only relevant change is to ipc/ipc_channel_reader.cc. Diffing patch set 8
against patch set 11 will show the changes since you last reviewed this CL.

[Tom Sepez, 2015-07-27 15:21:47]

lgtm

https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:110: // While we could theoretically updated
blocked_ids_ and call
nit: I didn't see the code to which this comment applies.

[erikchen, 2015-07-27 16:23:48]

https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:110: // While we could theoretically updated
blocked_ids_ and call
On 2015/07/27 15:21:47, Tom Sepez wrote:
> nit: I didn't see the code to which this comment applies.

This comment explains the lack of additional code here. Would you like me to
reword the comment to explain his more clearly?

[Tom Sepez, 2015-07-27 16:28:23]

On 2015/07/27 16:23:48, erikchen wrote:
>
https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader.cc
> File ipc/ipc_channel_reader.cc (right):
> 
>
https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader...
> ipc/ipc_channel_reader.cc:110: // While we could theoretically updated
> blocked_ids_ and call
> On 2015/07/27 15:21:47, Tom Sepez wrote:
> > nit: I didn't see the code to which this comment applies.
> 
> This comment explains the lack of additional code here. Would you like me to
> reword the comment to explain his more clearly?

Probably, since I didn't catch on while reading it :).

[erikchen, 2015-07-27 17:15:38]

https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1206093002/diff/280001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:110: // While we could theoretically updated
blocked_ids_ and call
On 2015/07/27 16:23:48, erikchen wrote:
> On 2015/07/27 15:21:47, Tom Sepez wrote:
> > nit: I didn't see the code to which this comment applies.
> 
> This comment explains the lack of additional code here. Would you like me to
> reword the comment to explain his more clearly?

I started rewriting this comment several times in an attempt to explain why I
wasn't adding some code here. Eventually I decided it was much simpler to just
add the two lines of code that I was trying to avoid adding.

[erikchen, 2015-07-27 17:15:42]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-07-27 17:15:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1206093002/#ps300001
(title: "Comments from tsepez.")

[commit-bot: I haz the power, 2015-07-27 17:16:33]

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

[commit-bot: I haz the power, 2015-07-27 18:26:34]

Committed patchset #12 (id:300001)

[commit-bot: I haz the power, 2015-07-27 18:27:40]

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