[Extensions] Finish making port creation asynchronous

[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

BUG=642380
Committed: https://crrev.com/e6037629d0de58e95969abdde7a0d6d52fac120c
Cr-Commit-Position: refs/heads/master@{#418403}

[Devlin, 2016-09-12 22:43:40]

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

[commit-bot: I haz the power, 2016-09-12 22:44:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 23:35:07]

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

[commit-bot: I haz the power, 2016-09-12 23:35:08]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-13 00:27:06]

Description was changed from

==========
[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

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

to

==========
[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

BUG=642380
==========

[Devlin, 2016-09-13 00:27:23]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Devlin, 2016-09-13 00:27:24]

lazyboy@, mind taking a look?

[lazyboy, 2016-09-13 00:34:40]

lgtm.

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:208: void
ExtensionFrameHelper::RequestNativePortId(
nit: RequestNativeAppPortId

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:216: // TODO(devlin): Make this
async. crbug.com/642380
Remove.

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/messagi...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/messagi...
extensions/renderer/messaging_bindings.cc:479: DCHECK(frame_helper);
I've found from reviews that this is unncessary, since things will crash on next
line anyway if frame_helper is nullptr.

[Devlin, 2016-09-13 16:49:52]

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

[Devlin, 2016-09-13 16:49:55]

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:208: void
ExtensionFrameHelper::RequestNativePortId(
On 2016/09/13 00:34:39, lazyboy wrote:
> nit: RequestNativeAppPortId

Done.

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:216: // TODO(devlin): Make this
async. crbug.com/642380
On 2016/09/13 00:34:39, lazyboy wrote:
> Remove.

Whoops, done.

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/messagi...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/messagi...
extensions/renderer/messaging_bindings.cc:479: DCHECK(frame_helper);
On 2016/09/13 00:34:39, lazyboy wrote:
> I've found from reviews that this is unncessary, since things will crash on
next
> line anyway if frame_helper is nullptr.

I don't quite agree.  For one thing, even if it crashes immediately, DCHECK()s
give us nice, readable stack traces (DCHECK failed at line n), whereas segfaults
sometimes just say "segfault", and you're left to determine where exactly it
happened.  Additionally, depending on how the compiler generates the code,
accessing a nullptr might not always crash immediately.  For instance, if the
compiler inlines the method, it could then try to read from the necessary
offsets for the variables used (and unlikely, but possibly (?) succeed), and you
can get a crash somewhere deeper in the stack.  Overall, I'd just prefer the
DCHECK unless there's good reason not to.  WDYT?

[commit-bot: I haz the power, 2016-09-13 16:50:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2016-09-13 17:22:08]

still lgtm.

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/messagi...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2331263002/diff/1/extensions/renderer/messagi...
extensions/renderer/messaging_bindings.cc:479: DCHECK(frame_helper);
On 2016/09/13 16:49:55, Devlin wrote:
> On 2016/09/13 00:34:39, lazyboy wrote:
> > I've found from reviews that this is unncessary, since things will crash on
> next
> > line anyway if frame_helper is nullptr.
> 
> I don't quite agree.  For one thing, even if it crashes immediately, DCHECK()s
> give us nice, readable stack traces (DCHECK failed at line n), whereas
segfaults
> sometimes just say "segfault",
In practice, I've found that too while running locally. The review comment
I mentioned was proabably because they have same effect in crash/ logs?
Either way, I'm fine with the DCHECK.

 and you're left to determine where exactly it
> happened.  Additionally, depending on how the compiler generates the code,
> accessing a nullptr might not always crash immediately.
Good point.

  For instance, if the
> compiler inlines the method, it could then try to read from the necessary
> offsets for the variables used (and unlikely, but possibly (?) succeed), and
you
> can get a crash somewhere deeper in the stack.  Overall, I'd just prefer the
> DCHECK unless there's good reason not to.  WDYT?

[Devlin, 2016-09-13 17:34:25]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2016-09-13 17:34:30]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-09-13 17:34:34]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-09-13 17:34:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 17:43:20]

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

[commit-bot: I haz the power, 2016-09-13 17:43:22]

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

[Devlin, 2016-09-13 17:45:36]

rdevlin.cronin@chromium.org changed reviewers:
+ rsesek@chromium.org

[Devlin, 2016-09-13 17:45:36]

+rsesek@ for extension_messages.h

[Robert Sesek, 2016-09-13 22:21:13]

lgtm

[Devlin, 2016-09-13 23:02:48]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-09-13 23:03:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 23:10:59]

Description was changed from

==========
[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

BUG=642380
==========

to

==========
[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

BUG=642380
==========

[commit-bot: I haz the power, 2016-09-13 23:11:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-13 23:12:41]

Description was changed from

==========
[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

BUG=642380
==========

to

==========
[Extensions] Finish making port creation asynchronous

Make extension port creation for tabs.connect and
runtime.connectNative asynchronous.

BUG=642380

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

[commit-bot: I haz the power, 2016-09-13 23:12:42]

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