Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(493)

Issue 709933002: Add frameId to MessageSender (extension messaging API) (Closed)

Created:
6 years, 1 month ago by robwu
Modified:
6 years, 1 month ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Add frameId to MessageSender (extension messaging API). Moved IPC for extension messaging from RenderView to RenderFrame. BUG=264286 R=kalman@chromium.org Committed: https://crrev.com/248d6a8456fdf67fa11be00e35105642f1871a9b Cr-Commit-Position: refs/heads/master@{#305148}

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : Add sender_frame_id to JS arguments #

Patch Set 4 : Add test that tests for a non-negative frameId #

Total comments: 26

Patch Set 5 : Address review comments in #2 #

Patch Set 6 : Generate frameId in browser #

Total comments: 13

Patch Set 7 : comments put message in assertTrue #

Total comments: 8

Patch Set 8 : update test fix some more compiler errors #

Total comments: 3

Patch Set 9 : frame_id -> pid #

Total comments: 2

Patch Set 10 : User scoped_ptr instead of ref, refactor test #

Total comments: 8

Patch Set 11 : fix nullptr + comments #24 #

Patch Set 12 : Move extension messaging IPC from RenderView to RenderFrame #

Total comments: 3

Patch Set 13 : import base/values.h instead of forward declaration to resolve C2027 #

Patch Set 14 : test: sender.tab.status = 'complete' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+243 lines, -116 lines) Patch
M chrome/browser/extensions/api/messaging/extension_message_port.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/messaging/extension_message_port.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -3 lines 0 comments Download
M chrome/browser/extensions/api/messaging/message_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/extensions/api/messaging/message_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 11 chunks +21 lines, -7 lines 0 comments Download
M chrome/browser/tab_contents/tab_util.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/tab_util.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +11 lines, -0 lines 0 comments Download
M chrome/renderer/extensions/extension_frame_helper.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -0 lines 0 comments Download
M chrome/renderer/extensions/extension_frame_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +38 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/messaging/connect/frame.js View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M chrome/test/data/extensions/api_test/messaging/connect/manifest.json View 1 2 3 4 5 6 7 2 chunks +6 lines, -1 line 0 comments Download
M chrome/test/data/extensions/api_test/messaging/connect/page.js View 1 2 3 4 5 6 7 2 chunks +14 lines, -0 lines 0 comments Download
M chrome/test/data/extensions/api_test/messaging/connect/test.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +48 lines, -0 lines 0 comments Download
M extensions/common/api/runtime.json View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M extensions/common/extension_messages.h View 1 2 3 4 5 2 chunks +12 lines, -1 line 0 comments Download
M extensions/renderer/dispatcher.h View 1 2 3 4 5 3 chunks +2 lines, -2 lines 0 comments Download
M extensions/renderer/dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -6 lines 0 comments Download
M extensions/renderer/extension_helper.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +0 lines, -14 lines 0 comments Download
M extensions/renderer/extension_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +0 lines, -35 lines 0 comments Download
M extensions/renderer/messaging_bindings.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -8 lines 0 comments Download
M extensions/renderer/messaging_bindings.cc View 1 2 3 4 5 6 7 8 9 10 11 12 chunks +32 lines, -23 lines 0 comments Download
M extensions/renderer/resources/messaging.js View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
M extensions/renderer/runtime_custom_bindings.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +7 lines, -9 lines 0 comments Download

Messages

Total messages: 58 (20 generated)
robwu
Ben: PTAL.
6 years, 1 month ago (2014-11-10 00:11:07 UTC) #1
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/80001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/80001/chrome/browser/extensions/api/messaging/message_service.cc#newcode346 chrome/browser/extensions/api/messaging/message_service.cc:346: source_frame_id = -1; I don't like rewriting the value ...
6 years, 1 month ago (2014-11-10 18:36:43 UTC) #2
robwu
https://codereview.chromium.org/709933002/diff/80001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/80001/chrome/browser/extensions/api/messaging/message_service.cc#newcode346 chrome/browser/extensions/api/messaging/message_service.cc:346: source_frame_id = -1; On 2014/11/10 18:36:42, kalman wrote: > ...
6 years, 1 month ago (2014-11-10 21:43:07 UTC) #3
robwu
Lei: Please rubberstamp the trivial change in chrome/browser/renderer_host/chrome_extension_message_filter.cc Tom: Please review extensions/common/extension_messages.h
6 years, 1 month ago (2014-11-10 21:54:05 UTC) #5
Tom Sepez
> Tom: Please review extensions/common/extension_messages.h The messages themselves are OK, but what are you planning ...
6 years, 1 month ago (2014-11-10 22:16:53 UTC) #6
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/80001/extensions/renderer/runtime_custom_bindings.cc File extensions/renderer/runtime_custom_bindings.cc (right): https://codereview.chromium.org/709933002/diff/80001/extensions/renderer/runtime_custom_bindings.cc#newcode64 extensions/renderer/runtime_custom_bindings.cc:64: info.source_frame_id = -1; On 2014/11/10 21:43:07, robwu wrote: > ...
6 years, 1 month ago (2014-11-10 22:20:17 UTC) #7
robwu
Tom: I've changed the IPC message following Ben's comment. https://codereview.chromium.org/709933002/diff/80001/extensions/renderer/runtime_custom_bindings.cc File extensions/renderer/runtime_custom_bindings.cc (right): https://codereview.chromium.org/709933002/diff/80001/extensions/renderer/runtime_custom_bindings.cc#newcode64 extensions/renderer/runtime_custom_bindings.cc:64: ...
6 years, 1 month ago (2014-11-10 23:13:39 UTC) #8
Tom Sepez
> Updated CL. I've added a new IPC message ExtensionMsg_TabConnectionInfo because > ExtensionMsg_DispatchOnConnect had already ...
6 years, 1 month ago (2014-11-10 23:17:50 UTC) #9
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/140001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/140001/chrome/browser/extensions/api/messaging/message_service.cc#newcode345 chrome/browser/extensions/api/messaging/message_service.cc:345: int source_frame_id = source_routing_id; It looks like this declaration ...
6 years, 1 month ago (2014-11-11 00:03:04 UTC) #10
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode126 chrome/test/data/extensions/api_test/messaging/connect/test.js:126: chrome.test.log("sendMessageFromFrameInTab: got frameId " + On 2014/11/10 21:43:07, robwu ...
6 years, 1 month ago (2014-11-11 00:24:03 UTC) #11
robwu
https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode126 chrome/test/data/extensions/api_test/messaging/connect/test.js:126: chrome.test.log("sendMessageFromFrameInTab: got frameId " + On 2014/11/11 00:24:03, kalman ...
6 years, 1 month ago (2014-11-11 00:34:26 UTC) #12
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode126 chrome/test/data/extensions/api_test/messaging/connect/test.js:126: chrome.test.log("sendMessageFromFrameInTab: got frameId " + On 2014/11/11 00:34:26, robwu ...
6 years, 1 month ago (2014-11-11 00:37:08 UTC) #13
robwu
https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/80001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode126 chrome/test/data/extensions/api_test/messaging/connect/test.js:126: chrome.test.log("sendMessageFromFrameInTab: got frameId " + On 2014/11/11 00:37:08, kalman ...
6 years, 1 month ago (2014-11-11 00:41:03 UTC) #14
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/180001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/180001/chrome/browser/extensions/api/messaging/message_service.cc#newcode345 chrome/browser/extensions/api/messaging/message_service.cc:345: RenderFrameHost* rfh = RenderFrameHost::FromID(source_frame_id, It looks to me like ...
6 years, 1 month ago (2014-11-11 01:00:57 UTC) #15
robwu
https://codereview.chromium.org/709933002/diff/180001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/180001/chrome/browser/extensions/api/messaging/message_service.cc#newcode345 chrome/browser/extensions/api/messaging/message_service.cc:345: RenderFrameHost* rfh = RenderFrameHost::FromID(source_frame_id, On 2014/11/11 01:00:56, kalman wrote: ...
6 years, 1 month ago (2014-11-11 21:32:47 UTC) #18
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/180001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/180001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode162 chrome/test/data/extensions/api_test/messaging/connect/test.js:162: ); On 2014/11/11 21:32:47, robwu wrote: > On 2014/11/11 ...
6 years, 1 month ago (2014-11-11 22:01:58 UTC) #19
robwu
https://codereview.chromium.org/709933002/diff/180001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/180001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode162 chrome/test/data/extensions/api_test/messaging/connect/test.js:162: ); On 2014/11/11 22:01:58, kalman wrote: > On 2014/11/11 ...
6 years, 1 month ago (2014-11-11 22:20:50 UTC) #20
not at google - send to devlin
https://codereview.chromium.org/709933002/diff/260001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/260001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode134 chrome/test/data/extensions/api_test/messaging/connect/test.js:134: chrome.test.assertEq({ Test is better now, but I think you ...
6 years, 1 month ago (2014-11-11 22:23:01 UTC) #21
robwu
https://codereview.chromium.org/709933002/diff/260001/chrome/test/data/extensions/api_test/messaging/connect/test.js File chrome/test/data/extensions/api_test/messaging/connect/test.js (right): https://codereview.chromium.org/709933002/diff/260001/chrome/test/data/extensions/api_test/messaging/connect/test.js#newcode134 chrome/test/data/extensions/api_test/messaging/connect/test.js:134: chrome.test.assertEq({ On 2014/11/11 22:23:00, kalman wrote: > Test is ...
6 years, 1 month ago (2014-11-11 22:44:38 UTC) #22
not at google - send to devlin
lgtm https://codereview.chromium.org/709933002/diff/340001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/340001/chrome/browser/extensions/api/messaging/message_service.cc#newcode154 chrome/browser/extensions/api/messaging/message_service.cc:154: this->source_tab.reset(source_tab.Pass()); you can just use = (not .reset()) ...
6 years, 1 month ago (2014-11-12 01:02:46 UTC) #24
robwu
https://codereview.chromium.org/709933002/diff/340001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/340001/chrome/browser/extensions/api/messaging/message_service.cc#newcode154 chrome/browser/extensions/api/messaging/message_service.cc:154: this->source_tab.reset(source_tab.Pass()); On 2014/11/12 01:02:46, kalman wrote: > you can ...
6 years, 1 month ago (2014-11-12 10:00:21 UTC) #25
robwu
Avi: PTAL at chrome/browser/tab_contents/tab_util.{h,cc} Ben: I have moved the IPC from RenderView to RenderFrame to ...
6 years, 1 month ago (2014-11-13 12:01:16 UTC) #36
Avi (use Gerrit)
tab util LGTM
6 years, 1 month ago (2014-11-13 15:56:51 UTC) #37
Charlie Reis
On 2014/11/13 12:01:16, robwu wrote: > Avi: PTAL at chrome/browser/tab_contents/tab_util.{h,cc} > > Ben: I have ...
6 years, 1 month ago (2014-11-13 19:47:05 UTC) #38
robwu
https://codereview.chromium.org/709933002/diff/460001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/460001/chrome/browser/extensions/api/messaging/message_service.cc#newcode348 chrome/browser/extensions/api/messaging/message_service.cc:348: // Main frame's frameId is 0. On 2014/11/13 19:47:05, ...
6 years, 1 month ago (2014-11-13 22:35:30 UTC) #40
robwu
On 2014/11/13 12:01:16, robwu wrote: > Ben: I have moved the IPC from RenderView to ...
6 years, 1 month ago (2014-11-18 22:27:21 UTC) #41
not at google - send to devlin
Still looks fine, I'm not too familiar with the intricacies of view vs frame (probably ...
6 years, 1 month ago (2014-11-18 22:38:40 UTC) #42
Charlie Reis
https://codereview.chromium.org/709933002/diff/460001/chrome/browser/extensions/api/messaging/message_service.cc File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/709933002/diff/460001/chrome/browser/extensions/api/messaging/message_service.cc#newcode348 chrome/browser/extensions/api/messaging/message_service.cc:348: // Main frame's frameId is 0. On 2014/11/13 22:35:30, ...
6 years, 1 month ago (2014-11-18 23:08:58 UTC) #43
robwu
On 2014/11/18 23:08:58, Charlie Reis wrote: > https://codereview.chromium.org/709933002/diff/460001/chrome/browser/extensions/api/messaging/message_service.cc > File chrome/browser/extensions/api/messaging/message_service.cc (right): > > https://codereview.chromium.org/709933002/diff/460001/chrome/browser/extensions/api/messaging/message_service.cc#newcode348 ...
6 years, 1 month ago (2014-11-18 23:15:17 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/709933002/460001
6 years, 1 month ago (2014-11-18 23:17:08 UTC) #46
commit-bot: I haz the power
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/88025)
6 years, 1 month ago (2014-11-19 00:00:31 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/709933002/480001
6 years, 1 month ago (2014-11-19 11:47:49 UTC) #50
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel/builds/13374)
6 years, 1 month ago (2014-11-19 13:49:06 UTC) #52
robwu
kalman@ Some tests failed because sometimes tab.status == 'loading' when a frame sends a message. ...
6 years, 1 month ago (2014-11-20 23:32:27 UTC) #53
not at google - send to devlin
ok that looks fine. A PITA to try to wait some more until the tab ...
6 years, 1 month ago (2014-11-20 23:51:38 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/709933002/500001
6 years, 1 month ago (2014-11-20 23:59:25 UTC) #56
commit-bot: I haz the power
Committed patchset #14 (id:500001)
6 years, 1 month ago (2014-11-21 02:04:56 UTC) #57
commit-bot: I haz the power
6 years, 1 month ago (2014-11-21 02:05:44 UTC) #58
Message was sent while issue was closed.
Patchset 14 (id:??) landed as
https://crrev.com/248d6a8456fdf67fa11be00e35105642f1871a9b
Cr-Commit-Position: refs/heads/master@{#305148}

Powered by Google App Engine
This is Rietveld 408576698