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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 131413009: Prototype: Use Chromium IPC for plugin LOAD_MODULE. (Closed)

Created:
6 years, 11 months ago by teravest
Modified:
5 years, 10 months ago
CC:
chromium-reviews, yusukes+watch_chromium.org, raymes+watch_chromium.org, teravest+watch_chromium.org, yzshen+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org
Visibility:
Public.

Description

Prototype: Use Chromium IPC for plugin LOAD_MODULE. This depends on a native client change: https://codereview.chromium.org/141413007/ BUG=333950

Patch Set 1 #

Patch Set 2 : Rebased, some FIXMEs cleaned up #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+665 lines, -221 lines) Patch
M components/nacl.gyp View 2 chunks +4 lines, -0 lines 0 comments Download
M components/nacl/browser/nacl_process_host.h View 3 chunks +6 lines, -1 line 1 comment Download
M components/nacl/browser/nacl_process_host.cc View 5 chunks +38 lines, -12 lines 4 comments Download
M components/nacl/common/nacl_host_messages.h View 2 chunks +5 lines, -1 line 0 comments Download
M components/nacl/common/nacl_messages.h View 1 2 chunks +13 lines, -0 lines 1 comment Download
M components/nacl/common/nacl_types.h View 1 chunk +4 lines, -2 lines 0 comments Download
M components/nacl/common/nacl_types.cc View 1 chunk +6 lines, -3 lines 0 comments Download
M components/nacl/loader/nacl_listener.h View 4 chunks +9 lines, -1 line 0 comments Download
M components/nacl/loader/nacl_listener.cc View 1 6 chunks +26 lines, -2 lines 0 comments Download
A components/nacl/loader/nacl_trusted_listener.h View 1 1 chunk +51 lines, -0 lines 0 comments Download
A components/nacl/loader/nacl_trusted_listener.cc View 1 1 chunk +89 lines, -0 lines 0 comments Download
M components/nacl/renderer/DEPS View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/nacl/renderer/ppb_nacl_private_impl.cc View 1 8 chunks +87 lines, -19 lines 0 comments Download
A components/nacl/renderer/trusted_plugin_channel.h View 1 1 chunk +37 lines, -0 lines 0 comments Download
A components/nacl/renderer/trusted_plugin_channel.cc View 1 1 chunk +62 lines, -0 lines 0 comments Download
M ppapi/api/private/ppb_nacl_private.idl View 1 2 chunks +19 lines, -10 lines 0 comments Download
M ppapi/c/private/ppb_nacl_private.h View 1 3 chunks +17 lines, -11 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/nacl_entry_points.h View 1 2 chunks +4 lines, -2 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/plugin.h View 1 4 chunks +14 lines, -8 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/plugin.cc View 1 6 chunks +88 lines, -63 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.h View 1 2 chunks +3 lines, -2 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc View 1 3 chunks +26 lines, -25 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/service_runtime.h View 1 2 chunks +4 lines, -1 line 0 comments Download
M ppapi/native_client/src/trusted/plugin/service_runtime.cc View 1 4 chunks +36 lines, -54 lines 0 comments Download
M ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c View 1 4 chunks +16 lines, -4 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
dmichael (off chromium)
https://codereview.chromium.org/131413009/diff/30001/components/nacl/browser/nacl_process_host.cc File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/131413009/diff/30001/components/nacl/browser/nacl_process_host.cc#newcode864 components/nacl/browser/nacl_process_host.cc:864: ReplyToRenderer(); This looks iffy... you do ReplyToRenderer whenever the ...
6 years, 10 months ago (2014-02-06 18:18:47 UTC) #1
teravest1
https://codereview.chromium.org/131413009/diff/30001/components/nacl/browser/nacl_process_host.cc File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/131413009/diff/30001/components/nacl/browser/nacl_process_host.cc#newcode864 components/nacl/browser/nacl_process_host.cc:864: ReplyToRenderer(); I do wait until both channels have been ...
6 years, 10 months ago (2014-02-06 18:21:14 UTC) #2
dmichael (off chromium)
6 years, 10 months ago (2014-02-06 18:24:21 UTC) #3 
(Still looking at this, mostly to get up to speed...  last set of comments was
mostly because I thought I spotted an important problem)

https://codereview.chromium.org/131413009/diff/30001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/131413009/diff/30001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:864: ReplyToRenderer();
On 2014/02/06 18:21:15, teravest1 wrote:
> I do wait until both channels have been created before sending the reply.
> 
> OnTrustedPluginChannelCreated
>   sets nacl_renderer_channel_handle_
>   and checks that ppapi_renderer_channel_handle_ is valid before sending.
> 
> OnPpapiRendererChannelCreated
>   sets ppapi_renderer_channel_handle_
>   and checks that nacl_renderer_channel_handle_ is valid before sending.
> 
> Am I misunderstanding you?
> 
> Regardless, the race issue I was discussing earlier doesn't happen with this
> patch.
D'oh, okay, I see that now. Sorry. I should either drink more coffee or less
alcohol.

https://codereview.chromium.org/131413009/diff/30001/components/nacl/common/n...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/131413009/diff/30001/components/nacl/common/n...
components/nacl/common/nacl_messages.h:105: IPC::PlatformFileForTransit /* fd
*/,
It would be good to have a name or comment explaining what the fd represents.

Powered by Google App Engine
This is Rietveld 408576698