Fix a couple of bugs relating to sending Tab info with chrome.runtime.connect and

Fix a couple of bugs relating to sending Tab info with chrome.runtime.connect and
sendMessage: don't send it when the connection originates from an extension page,
and don't sent it regardless to platform apps. To compensate, send the URL down
separately.


BUG=55316, 234384
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196316

[not at google - send to devlin, 2013-04-23 01:05:22]

Also add tests.

I haven't actually quite finished this. I realised in my writing of the CL
description that the "not sending to platform apps" thing didn't actually
happen, I'll need to add that. It's an accident that the tests I changed pass;
there aren't any tests for content script -> app communication.

But, if you want to give early feedback that'd be cool. I don't expect the
changes to implement the no-tab-to-app thing to be any more than a couple of
lines.

[Matt Perry, 2013-04-23 18:00:16]

LGTM

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/messaging/message_service.cc (left):

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/messaging/message_service.cc:235: 
Why delete this?

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/messaging/message_service.cc:316: GURL(),  //
source URL doesn't make sense for opening to tabs
Source tab doesn't make sense to me either. Do you know why we included it
before? I can't think of a case where it would happen.

[not at google - send to devlin, 2013-04-23 18:30:18]

Ok, comments addressed, and I added the platform app check. Led itself to a tiny
cleanup. PTAL.

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/messaging/message_service.cc (left):

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/messaging/message_service.cc:235: 
On 2013/04/23 18:00:17, Matt Perry wrote:
> Why delete this?

Maybe I'm missing something subtle (or obvious), but it seems like a copy-paste
gone wrong. Tab value isn't actually used anywhere.

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/messaging/message_service.cc:316: GURL(),  //
source URL doesn't make sense for opening to tabs
On 2013/04/23 18:00:17, Matt Perry wrote:
> Source tab doesn't make sense to me either. Do you know why we included it
> before? I can't think of a case where it would happen.

Oh, right. Yeah we presumably included it before because there were no tests to
assert what we documented. I think that GetTabId above will always be -1 in this
case though, since these calls can only come from extension processes, so
source_tab will always be NULL here. I'll make it explicit, thanks.

[Matt Perry, 2013-04-23 18:39:37]

lgtm

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/messaging/message_service.cc (left):

https://codereview.chromium.org/14301016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/messaging/message_service.cc:235: 
On 2013/04/23 18:30:18, kalman wrote:
> On 2013/04/23 18:00:17, Matt Perry wrote:
> > Why delete this?
> 
> Maybe I'm missing something subtle (or obvious), but it seems like a
copy-paste
> gone wrong. Tab value isn't actually used anywhere.

Oh right, good point :)

[commit-bot: I haz the power, 2013-04-23 18:40:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/14301016/5001

[commit-bot: I haz the power, 2013-04-23 18:41:09]

Presubmit check for 14301016-5001 failed and returned exit status 1.

INFO:root:Found 24 file(s).

..ERROR:root:File system is offline, cannot read
docs/templates/public/extensions/
ERROR:root:File system is offline, cannot read docs/templates/public/
.
----------------------------------------------------------------------
Ran 3 tests in 3.570s

OK
Rendering 2 explicit files...
Rendering extensions/runtime.html...
Took 2.65625500679 seconds
Rendering apps/runtime.html...
Took 0.912538051605 seconds
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/renderer_host/chrome_render_message_filter.cc
    chrome/browser/renderer_host/chrome_render_message_filter.h
    chrome/common/extensions/extension_messages.h
    chrome/renderer/chrome_mock_render_thread.cc
    chrome/renderer/chrome_mock_render_thread.h

Presubmit checks took 6.1s to calculate.

[not at google - send to devlin, 2013-04-23 18:43:39]

+jschuh for message change
+sky for non-extension changes

[not at google - send to devlin, 2013-04-23 20:45:24]

sky -> thestig (sky is busy)
jschuh -> cdn (jschuh is ooo)

[Lei Zhang, 2013-04-23 20:52:14]

non-extension bits lgtm

[commit-bot: I haz the power, 2013-04-23 23:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/14301016/5001

[not at google - send to devlin, 2013-04-23 23:26:47]

tbr'ing for extension_messages since it was pretty trivial. apologies if I
introduce something that can break out of the sandbox.

[Cris Neckar, 2013-04-23 23:29:18]

Please don't TBR these. I am in the process of looking now.

[not at google - send to devlin, 2013-04-23 23:29:46]

ok

[Cris Neckar, 2013-04-23 23:34:09]

lgtm for ipc security review

[commit-bot: I haz the power, 2013-04-23 23:35:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/14301016/5001

[commit-bot: I haz the power, 2013-04-24 02:02:37]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-24 22:00:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/14301016/47001

[commit-bot: I haz the power, 2013-04-24 23:53:46]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-24 23:54:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/14301016/47001

[commit-bot: I haz the power, 2013-04-25 01:42:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-25 02:22:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/14301016/47001

[commit-bot: I haz the power, 2013-04-25 04:55:02]

Change committed as 196316