Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by adrian.belgun Modified:
4 years, 5 months ago Reviewers:
bartfab (slow), bbudge, robert.bradford, dcheng, dskaram, emaxx, Kevin Cernekee CC:
chromium-reviews, mlamouri+watch-content_chromium.org, extensions-reviews_chromium.org, jam, darin-cc_chromium.org, oshima+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@vpn-nacl-sdk Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionppapi: PPB_VpnProvider: Implement Resource Host
BUG=506490
Committed: https://crrev.com/8a1558d86fa478c65aba0b268129a9d8f2d3a7af
Cr-Commit-Position: refs/heads/master@{#403909}
Patch Set 1 #Patch Set 2 : Cleanup. #Patch Set 3 : Sync to new API. #Patch Set 4 : Adjust for simpler API. #Patch Set 5 : Minor fix #Patch Set 6 : Performance Optimization: Use shared memory #Patch Set 7 : Rebase #Patch Set 8 : Implemented delegate model to remove .DEPS workarounds. #Patch Set 9 : Split patch #Patch Set 10 : Add permission check #Patch Set 11 : Format #Patch Set 12 : Cleanup #Patch Set 13 : Fix component build #Patch Set 14 : Fix comments #Patch Set 15 : Split patch #Patch Set 16 : Sync #Patch Set 17 : Sync #Patch Set 18 : Sync #Patch Set 19 : Sync #Patch Set 20 : Sync #Patch Set 21 : Sync #Patch Set 22 : Sync #
Total comments: 9
Patch Set 23 : Add packet size range checking. #
Total comments: 28
Patch Set 24 : Respond to reviews. #
Total comments: 7
Patch Set 25 : Respond to reviews. #
Total comments: 19
Patch Set 26 : Add DISALLOW_COPY_AND_ASSIGN #Patch Set 27 : Respond to reviews. #Patch Set 28 : Use uint32_t consistently for id. #Patch Set 29 : Fix sign compare error. #
Total comments: 24
Patch Set 30 : Respond to reviews. #Patch Set 31 : Respond to reviews. #
Total comments: 2
Patch Set 32 : Add DCHECKs. #
Created: 4 years, 5 months ago
Messages
Total messages: 51 (18 generated)
Re-based old PPAPI VPN Provider proposal and split it into three separate reviews. ppapi: https://codereview.chromium.org/1726303003/ naclsdk: https://codereview.chromium.org/1731273002/ browser: https://codereview.chromium.org/1735473002/
bartfab@chromium.org changed reviewers: + cernekee@chromium.org, emaxx@chromium.org - kaliamoorthi@chromium.org
adrian.belgun@intel.com changed reviewers: + kaliamoorthi@chromium.org - cernekee@chromium.org, emaxx@chromium.org
This patch contains the browser part. Unfortunately this is, this approach contains .DEPS workarounds that were good enough for the PoC, but it's not production ready. What would be the best approach to move packets and messages between the PPAPI resource host and the shill DBUS client? My best guess would be a new instance of ShillThirdPartyVpnObserver specially for the PPAPI path, but that would contain a lot of code duplication with the VPN Service from the extension. I really appreciate any help with solving this architecture problem.
+Kevin to ensure any changes here don't conflict with the robustness improvements currently in flight. David Karam | Product Manager - Chrome for Work <https://www.google.com/work/chrome/> | dskaram@google.com | +49 172 280 8110 -- Google Germany GmbH Erika-Mann-Strasse 33 80636 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle On Wed, Feb 24, 2016 at 3:14 PM, <adrian.belgun@intel.com> wrote: > This patch contains the browser part. Unfortunately this is, this approach > contains .DEPS workarounds that were good enough for the PoC, but it's not > production ready. > > What would be the best approach to move packets and messages between the > PPAPI > resource host and the shill DBUS client? > > My best guess would be a new instance of ShillThirdPartyVpnObserver > specially > for the PPAPI path, but that would contain a lot of code duplication with > the > VPN Service from the extension. > > I really appreciate any help with solving this architecture problem. > > > https://codereview.chromium.org/1735473002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== ppapi: PPB_VpnProvider: Browser implementation. BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Browser implementation. BUG=506490 ==========
adrian.belgun@intel.com changed reviewers: + cernekee@chromium.org, emaxx@chromium.org - kaliamoorthi@chromium.org
Description was changed from ========== ppapi: PPB_VpnProvider: Browser implementation. BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Browser Resource Host BUG=506490 ==========
Description was changed from ========== ppapi: PPB_VpnProvider: Browser Resource Host BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Implement Resource Host BUG=506490 ==========
https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:29: class PepperVpnProviderMessageFilterChromeOS:: Leave ChromeOS off the name. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:166: std::vector<char> packet(packet_pointer, packet_pointer + packet_size); This is an attack surface, so you should sanity check packet_size against some maximum value. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:184: size_t size = kMaxQueuedPackets * kMaxPacketSizeInBytes; You control the math but perhaps a DCHECK to make sure someone doesn't change the constants in a way that causes problems. Or better yet, since the product of these two is used so often, define it as another constant with the others. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:20: class CONTENT_EXPORT PepperVpnProviderMessageFilterChromeOS I don't see a reason for including ChromeOS as part of the name (here, and in the file names.) Perhaps a comment explaining that it's only available on ChromeOS.
Uploaded new patch set per review. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:29: class PepperVpnProviderMessageFilterChromeOS:: On 2016/06/21 23:58:38, bbudge wrote: > Leave ChromeOS off the name. Done. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:166: std::vector<char> packet(packet_pointer, packet_pointer + packet_size); On 2016/06/21 23:58:38, bbudge wrote: > This is an attack surface, so you should sanity check packet_size against some > maximum value. Done. Added checking in both SendPacket and OnPacketReceived, in both Resource and Resource Host. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:184: size_t size = kMaxQueuedPackets * kMaxPacketSizeInBytes; On 2016/06/21 23:58:38, bbudge wrote: > You control the math but perhaps a DCHECK to make sure someone doesn't change > the constants in a way that causes problems. > > Or better yet, since the product of these two is used so often, define it as > another constant with the others. Done. Removed computation. Added new precalculated constant. https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:20: class CONTENT_EXPORT PepperVpnProviderMessageFilterChromeOS On 2016/06/21 23:58:38, bbudge wrote: > I don't see a reason for including ChromeOS as part of the name (here, and in > the file names.) > > Perhaps a comment explaining that it's only available on ChromeOS. The 'ChromeOS' in the class name is indeed redundant. But the "_chromeos" in the file name is needed to further ensure that this is build only on Chrome OS. See the GYP documentation here: https://gyp.gsrc.io/docs/UserDocumentation.md#Add-a-platform_specific-source-...
https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/420001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:20: class CONTENT_EXPORT PepperVpnProviderMessageFilterChromeOS On 2016/06/22 18:21:59, adrian.belgun wrote: > On 2016/06/21 23:58:38, bbudge wrote: > > I don't see a reason for including ChromeOS as part of the name (here, and in > > the file names.) > > > > Perhaps a comment explaining that it's only available on ChromeOS. > > The 'ChromeOS' in the class name is indeed redundant. But the "_chromeos" in the > file name is needed to further ensure that this is build only on Chrome OS. > See the GYP documentation here: > https://gyp.gsrc.io/docs/UserDocumentation.md#Add-a-platform_specific-source-... > OK https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:30: class PepperVpnProviderMessageFilter::PepperVpnProviderResourceHostProxyImpl Standalone class in anonymous namespace. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:82: NOTREACHED(); return here. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:85: extension_id_ = host->GetDocumentURLForInstance(instance); This should be named document_url_ as it's not really the extension id. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:134: const std::string& configuration_name) { Is it OK to call Bind multiple times? It seems undesirable, so maybe check bound_ and return PP_ERROR_FAILED on such calls. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:177: FailureCallback failure) { nit: s/success/successCallback or successCb, ditto for failure. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:191: std::unique_ptr<base::SharedMemory> send_buffer(new base::SharedMemory); recv_buffer? https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:234: receive_packet_buffer_->GetHandle(), kBufferSize)); For the failure case, should you return the shm handles? https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:259: const std::string& error_message) { It seems undesirable to drop the error information. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:308: int32_t PepperVpnProviderMessageFilter::OnPacketReceivedReply( Usually, methods in the .cc file are in the same order as methods in the .h file. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:36: class PepperVpnProviderResourceHostProxyImpl; Since this is not used anywhere in the header, I think it can be moved into the .cc file and implemented as a stand alone class in an anonymous namespace. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:38: using StringCallback = base::Callback<void(const std::string& result)>; nit: MessageCallback or ResultCallback NOTE: this doesn't appear to be used. https://codereview.chromium.org/1735473002/diff/440001/ppapi/proxy/vpn_provid... File ppapi/proxy/vpn_provider_resource.cc (right): https://codereview.chromium.org/1735473002/diff/440001/ppapi/proxy/vpn_provid... ppapi/proxy/vpn_provider_resource.cc:108: return PP_ERROR_MESSAGE_TOO_BIG; It's fine to check this to help plugin writers, but it doesn't add to security as malicious plugins can bypass it. https://codereview.chromium.org/1735473002/diff/440001/ppapi/proxy/vpn_provid... ppapi/proxy/vpn_provider_resource.cc:159: if (!bound_ || (packet_size > receive_packet_buffer_->GetMaxPacketSize())) { packet_size is coming from the browser so this should never happen. Perhaps a DCHECK? https://codereview.chromium.org/1735473002/diff/440001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/440001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:26: uint32_t GetMaxPacketSize(); implement inline
Uploaded new patch set per review. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:30: class PepperVpnProviderMessageFilter::PepperVpnProviderResourceHostProxyImpl On 2016/06/23 18:10:22, bbudge wrote: > Standalone class in anonymous namespace. Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:82: NOTREACHED(); On 2016/06/23 18:10:22, bbudge wrote: > return here. Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:85: extension_id_ = host->GetDocumentURLForInstance(instance); On 2016/06/23 18:10:22, bbudge wrote: > This should be named document_url_ as it's not really the extension id. Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:134: const std::string& configuration_name) { On 2016/06/23 18:10:22, bbudge wrote: > Is it OK to call Bind multiple times? It seems undesirable, so maybe check > bound_ and return PP_ERROR_FAILED on such calls. Yes it is. It's expected behavior for supporting reconnects. Considering the typical usage as described in the IDL https://cs.chromium.org/chromium/src/ppapi/api/ppb_vpn_provider.idl a plugin must call Bind() each time the connection connects to explicitly route packets through NaCl. The alternative would be that for each reconnect the PPB_VpnProvider instance to be recreated, which would be cumbersome. The following sequence must work: Create -> (Connect -> Bind -> Disconnect)* -> Destroy. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:177: FailureCallback failure) { On 2016/06/23 18:10:22, bbudge wrote: > nit: s/success/successCallback or successCb, ditto for failure. Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:191: std::unique_ptr<base::SharedMemory> send_buffer(new base::SharedMemory); On 2016/06/23 18:10:22, bbudge wrote: > recv_buffer? Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:234: receive_packet_buffer_->GetHandle(), kBufferSize)); On 2016/06/23 18:10:22, bbudge wrote: > For the failure case, should you return the shm handles? Done. Moved this code to OnBindSuccess(). https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:259: const std::string& error_message) { On 2016/06/23 18:10:22, bbudge wrote: > It seems undesirable to drop the error information. Acknowledged. Added logs for developers for plugin developers for debugging. Would rather not unnecessary bloat PpapiPluginMsg_VpnProvider_SendPacketReply because that is on the performance-sensitive path. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:308: int32_t PepperVpnProviderMessageFilter::OnPacketReceivedReply( On 2016/06/23 18:10:22, bbudge wrote: > Usually, methods in the .cc file are in the same order as methods in the .h > file. Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:36: class PepperVpnProviderResourceHostProxyImpl; On 2016/06/23 18:10:22, bbudge wrote: > Since this is not used anywhere in the header, I think it can be moved into the > .cc file and implemented as a stand alone class in an anonymous namespace. Done. https://codereview.chromium.org/1735473002/diff/440001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:38: using StringCallback = base::Callback<void(const std::string& result)>; On 2016/06/23 18:10:23, bbudge wrote: > nit: MessageCallback or ResultCallback > NOTE: this doesn't appear to be used. Done. Removed. https://codereview.chromium.org/1735473002/diff/440001/ppapi/proxy/vpn_provid... File ppapi/proxy/vpn_provider_resource.cc (right): https://codereview.chromium.org/1735473002/diff/440001/ppapi/proxy/vpn_provid... ppapi/proxy/vpn_provider_resource.cc:108: return PP_ERROR_MESSAGE_TOO_BIG; On 2016/06/23 18:10:23, bbudge wrote: > It's fine to check this to help plugin writers, but it doesn't add to security > as malicious plugins can bypass it. How can it be bypassed? When the plugin will call SendPacket() with bigger messages, the message will be rejected before being written to the buffer. https://codereview.chromium.org/1735473002/diff/440001/ppapi/proxy/vpn_provid... ppapi/proxy/vpn_provider_resource.cc:159: if (!bound_ || (packet_size > receive_packet_buffer_->GetMaxPacketSize())) { On 2016/06/23 18:10:23, bbudge wrote: > packet_size is coming from the browser so this should never happen. Perhaps a > DCHECK? Done. https://codereview.chromium.org/1735473002/diff/440001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/440001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:26: uint32_t GetMaxPacketSize(); On 2016/06/23 18:10:23, bbudge wrote: > implement inline Done.
https://codereview.chromium.org/1735473002/diff/460001/content/browser/render... File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/1735473002/diff/460001/content/browser/render... content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:27: #include "content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h" #if defined(OS_CHROMEOS) ? https://codereview.chromium.org/1735473002/diff/460001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1735473002/diff/460001/content/content_browse... content/content_browser.gypi:1866: 'browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h', You probably need a similar change in a BUILD.gn file. https://codereview.chromium.org/1735473002/diff/460001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/460001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:26: uint32_t GetMaxPacketSize() { return packet_size_; } nit: maybe s/packet_size_/max_packet_size_ ?
Uploaded new patch set. https://codereview.chromium.org/1735473002/diff/460001/content/browser/render... File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/1735473002/diff/460001/content/browser/render... content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:27: #include "content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h" On 2016/06/27 22:25:07, bbudge wrote: > #if defined(OS_CHROMEOS) ? Done. https://codereview.chromium.org/1735473002/diff/460001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1735473002/diff/460001/content/content_browse... content/content_browser.gypi:1866: 'browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h', On 2016/06/27 22:25:07, bbudge wrote: > You probably need a similar change in a BUILD.gn file. The BUILD.gn uses this file to generate the GN at run time. https://cs.chromium.org/chromium/src/content/browser/browser.gni?l=10 https://cs.chromium.org/chromium/src/content/browser/BUILD.gn?l=7 https://cs.chromium.org/chromium/src/content/browser/BUILD.gn?l=256 Thus, no duplication is needed. https://codereview.chromium.org/1735473002/diff/460001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/460001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:26: uint32_t GetMaxPacketSize() { return packet_size_; } On 2016/06/27 22:25:07, bbudge wrote: > nit: maybe s/packet_size_/max_packet_size_ ? Done.
The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
adrian.belgun@intel.com changed reviewers: + dcheng@chromium.org
dcheng@: Please review changes in ppapi/proxy/ppapi_messages.h. Thank you!
LGTM w/some comments https://codereview.chromium.org/1735473002/diff/460001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1735473002/diff/460001/content/content_browse... content/content_browser.gypi:1866: 'browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h', On 2016/06/28 07:02:20, adrian.belgun wrote: > On 2016/06/27 22:25:07, bbudge wrote: > > You probably need a similar change in a BUILD.gn file. > > The BUILD.gn uses this file to generate the GN at run time. > https://cs.chromium.org/chromium/src/content/browser/browser.gni?l=10 > https://cs.chromium.org/chromium/src/content/browser/BUILD.gn?l=7 > https://cs.chromium.org/chromium/src/content/browser/BUILD.gn?l=256 > > Thus, no duplication is needed. OK thanks for explaining that. https://codereview.chromium.org/1735473002/diff/480001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/480001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:32: std::vector<bool> available_; I didn't notice before but you should probably disallow copy and assignment.
Thanks for the review. Uploaded new patch set. https://codereview.chromium.org/1735473002/diff/480001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/480001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:32: std::vector<bool> available_; On 2016/06/29 15:00:40, bbudge wrote: > I didn't notice before but you should probably disallow copy and assignment. Done.
https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:49: NOTREACHED(); Having both NOTREACHED and the handling code ("return" in the next line) is unrecommended practice according to our style guide: https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:133: if (recv_packet_buffer_.get() && recv_packet_buffer_->GetAvailable(&id)) { nit: Calling the get method is unnecessary to perform the null/non-null check. unique_ptr already has operator bool. (This occurs in this file several times.) https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:149: } Shouldn't send_packet_buffer_ and recv_packet_buffer_ be cleared too? https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:164: configuration_id_ = configuration_id; Is it possible for this OnBind method to be called twice, without unbinding in between? In that case, for example, this configuration_id_ member would be just overwritten - is it the expected behavior? https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:164: configuration_id_ = configuration_id; Is it possible for this OnBind method to be called twice, without unbinding in between? In that case, for example, this configuration_id_ member would be just overwritten - is it the expected behavior? https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:215: if (!send_buffer.get() || !send_buffer->CreateAndMapAnonymous(kBufferSize)) Is there any evidence that operator new may return NULL? I've never seen such checks in the Chrome code base. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:322: const void* packet_pointer = &packet.front(); Can the packet be an empty vector? Then it would be incorrect to take the address of the first element here. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:16: nit: Please include all required header files. E.g., looking at some first members of the class below, the following ones are missing: "ppapi/c/pp_instance.h", "base/memory/ref_counted.h", "base/task_runner.h", "ipc/ipc_message.h", <stdint.h>, "ppapi/host/host_message_context.h", <vector>, etc. https://codereview.chromium.org/1735473002/diff/480001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1735473002/diff/480001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:2046: int32_t /* id */) Is there a reason to make the id arguments signed at one places and unsigned at others?
Thanks for the review! Uploaded new patch set with suggested changes. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:49: NOTREACHED(); On 2016/06/29 16:36:39, emaxx wrote: > Having both NOTREACHED and the handling code ("return" in the next line) is > unrecommended practice according to our style guide: > https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... Done. Removed NOTREACHED(). https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:133: if (recv_packet_buffer_.get() && recv_packet_buffer_->GetAvailable(&id)) { On 2016/06/29 16:36:39, emaxx wrote: > nit: Calling the get method is unnecessary to perform the null/non-null check. > unique_ptr already has operator bool. (This occurs in this file several times.) Done. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:149: } On 2016/06/29 16:36:39, emaxx wrote: > Shouldn't send_packet_buffer_ and recv_packet_buffer_ be cleared too? Done. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:164: configuration_id_ = configuration_id; On 2016/06/29 16:36:39, emaxx wrote: > Is it possible for this OnBind method to be called twice, without unbinding in > between? > In that case, for example, this configuration_id_ member would be just > overwritten - is it the expected behavior? Yes. Only the active configuration can be bound. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:215: if (!send_buffer.get() || !send_buffer->CreateAndMapAnonymous(kBufferSize)) On 2016/06/29 16:36:39, emaxx wrote: > Is there any evidence that operator new may return NULL? I've never seen such > checks in the Chrome code base. Done. Cleaned-up the code. Now only checking CreateAndMapAnonymous(). https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:322: const void* packet_pointer = &packet.front(); On 2016/06/29 16:36:39, emaxx wrote: > Can the packet be an empty vector? > Then it would be incorrect to take the address of the first element here. Made sure that this is not called with empty vectors. Added a DCHECK here to further enforce this. https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h (right): https://codereview.chromium.org/1735473002/diff/480001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.h:16: On 2016/06/29 16:36:39, emaxx wrote: > nit: Please include all required header files. > E.g., looking at some first members of the class below, the following ones are > missing: "ppapi/c/pp_instance.h", "base/memory/ref_counted.h", > "base/task_runner.h", "ipc/ipc_message.h", <stdint.h>, > "ppapi/host/host_message_context.h", <vector>, etc. Done. Added remaining headers. https://codereview.chromium.org/1735473002/diff/480001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1735473002/diff/480001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:2046: int32_t /* id */) On 2016/06/29 16:36:39, emaxx wrote: > Is there a reason to make the id arguments signed at one places and unsigned at > others? Fixed.
The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:169: return std::unique_ptr<ResourceHost>(new MessageFilterHost( Nit: base::MakeUnique<MessageFilterHost(...) https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:170: host_->GetPpapiHost(), instance, resource, vpn_provider)); Nit: std::move(vpn_provider) https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:20: const size_t kMaxQueuedPackets = 128; At least to me, it wasn't immediately obvious that kMaxQueuedPackets has nothing to do with std::queue<...> received_packets_. Not sure this comment could be clarified though, perhaps I am just not familiar enough with Pepper? https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:27: PepperVpnProviderResourceHostProxyImpl( Nit: explicit https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:68: &render_frame_id)) Sorry if this is a dumb question. Presumably this is called on the UI thread. What is the scenario where this function call succeeds (e.g. we find it in the instance map) but the process host / browser context lookups below return null? https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:101: return NULL; Nit: nullptr https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:212: send_packet_buffer_ = base::WrapUnique(new ppapi::VpnProviderSharedBuffer( Nit: base::MakeUnique here and below. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:216: if (!recv_packet_buffer_) { What's the scenario where one of these, but not the other would be initialized? If there's some logical guarantee that these are either always both set or both unset, we can simplify this code a bit by constructing it into local unique_ptrs and moving them into members upon success (perhaps we could even do this inside OnBindSuccess so we can omit more manual cleanup in OnBindFailure): then we don't need the manual reset() on line 219. https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:2034: // VpnProvider Plugin -> Browser Messages Are these comments accurate? i see PpapiHostMsg and PpapiPluginMsg interleaved here. https://codereview.chromium.org/1735473002/diff/560001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/560001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:26: uint32_t GetMaxPacketSize() { return max_packet_size_; } Nit: for simple accessors, the usual chromium convention is to name it_like_this (it can also be made const)
Thank you for reviewing. Uploaded new patch set with changes as per review. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:169: return std::unique_ptr<ResourceHost>(new MessageFilterHost( On 2016/07/04 04:24:06, dcheng wrote: > Nit: base::MakeUnique<MessageFilterHost(...) Done. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:170: host_->GetPpapiHost(), instance, resource, vpn_provider)); On 2016/07/04 04:24:06, dcheng wrote: > Nit: std::move(vpn_provider) Done. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:20: const size_t kMaxQueuedPackets = 128; On 2016/07/04 04:24:07, dcheng wrote: > At least to me, it wasn't immediately obvious that kMaxQueuedPackets has nothing > to do with std::queue<...> received_packets_. Not sure this comment could be > clarified though, perhaps I am just not familiar enough with Pepper? Renamed constants to clarify their relation to the shared buffer, not the queue. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:27: PepperVpnProviderResourceHostProxyImpl( On 2016/07/04 04:24:06, dcheng wrote: > Nit: explicit Done. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:68: &render_frame_id)) On 2016/07/04 04:24:06, dcheng wrote: > Sorry if this is a dumb question. Presumably this is called on the UI thread. > What is the scenario where this function call succeeds (e.g. we find it in the > instance map) but the process host / browser context lookups below return null? Neither renderer_process_host or browser_context should be NULL at this point. While experimenting with this I have not encountered that use-case. While the error checking on this may be superfluous, it sure isn't a hindrance to performance and can only increase robustness for that corner case. Also, looking at how this is used, those two are usually NULL-checked. https://cs.chromium.org/chromium/src/content/browser/renderer_host/pepper/pep... https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:101: return NULL; On 2016/07/04 04:24:07, dcheng wrote: > Nit: nullptr Done. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:212: send_packet_buffer_ = base::WrapUnique(new ppapi::VpnProviderSharedBuffer( On 2016/07/04 04:24:07, dcheng wrote: > Nit: base::MakeUnique here and below. Done. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:216: if (!recv_packet_buffer_) { On 2016/07/04 04:24:07, dcheng wrote: > What's the scenario where one of these, but not the other would be initialized? > > If there's some logical guarantee that these are either always both set or both > unset, we can simplify this code a bit by constructing it into local unique_ptrs > and moving them into members upon success (perhaps we could even do this inside > OnBindSuccess so we can omit more manual cleanup in OnBindFailure): then we > don't need the manual reset() on line 219. I would prefer that allocation testing is done here, not in the success handler. If CreateAndMapAnonymous(kBufferSize) fails for some reason, we would need to unbind in VpnService, thus making the flow a bit convoluted. It's much simpler to test the shm allocation here. Also, unified buffer allocation. https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:2034: // VpnProvider Plugin -> Browser Messages On 2016/07/04 04:24:07, dcheng wrote: > Are these comments accurate? i see PpapiHostMsg and PpapiPluginMsg interleaved > here. Initially it was split into 4 parts - Plugin -> Browser Messages - Browser -> Plugin Replies - Browser -> Plugin Messages - Plugin -> Browser Replies https://codereview.chromium.org/1931513002/diff/180001/ppapi/proxy/ppapi_mess... An earlier reviewer recommended that "the more common pattern is to group messages with their reply message. (...) Then you would just have plugin->browser and browser->plugin groups." Thus leaving only two groups. What do you recommend? https://codereview.chromium.org/1735473002/diff/560001/ppapi/shared_impl/vpn_... File ppapi/shared_impl/vpn_provider_util.h (right): https://codereview.chromium.org/1735473002/diff/560001/ppapi/shared_impl/vpn_... ppapi/shared_impl/vpn_provider_util.h:26: uint32_t GetMaxPacketSize() { return max_packet_size_; } On 2016/07/04 04:24:07, dcheng wrote: > Nit: for simple accessors, the usual chromium convention is to name it_like_this > (it can also be made const) Done.
https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:68: &render_frame_id)) On 2016/07/04 14:28:32, adrian.belgun wrote: > On 2016/07/04 04:24:06, dcheng wrote: > > Sorry if this is a dumb question. Presumably this is called on the UI thread. > > What is the scenario where this function call succeeds (e.g. we find it in the > > instance map) but the process host / browser context lookups below return > null? > > Neither renderer_process_host or browser_context should be NULL at this point. > While experimenting with this I have not encountered that use-case. > While the error checking on this may be superfluous, it sure isn't a hindrance > to performance and can only increase robustness for that corner case. > Also, looking at how this is used, those two are usually NULL-checked. > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/pepper/pep... There may not be a large perf cost, but there is a cost to the code reader: now the reader needs to figure out if there is an exceptional case where this can happen, and how to handle it. It also means we need to try to gracefully handle cases that should never happen in practice, resulting in more lines of unused code. There are a few other ResourceMessageFilters that call this function, and they don't bother checking the result, or they guard it with NOTREACHED(). That's probably the pattern we should be using here. https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:2034: // VpnProvider Plugin -> Browser Messages On 2016/07/04 14:28:32, adrian.belgun wrote: > On 2016/07/04 04:24:07, dcheng wrote: > > Are these comments accurate? i see PpapiHostMsg and PpapiPluginMsg interleaved > > here. > > Initially it was split into 4 parts > - Plugin -> Browser Messages > - Browser -> Plugin Replies > - Browser -> Plugin Messages > - Plugin -> Browser Replies > https://codereview.chromium.org/1931513002/diff/180001/ppapi/proxy/ppapi_mess... > > An earlier reviewer recommended that "the more common pattern is to group > messages with their reply message. (...) Then you would just have > plugin->browser and browser->plugin groups." Thus leaving only two groups. > > What do you recommend? That's fine, but that makes this comment not entirely accurate. Maybe model it after something like https://cs.chromium.org/chromium/src/ppapi/proxy/ppapi_messages.h?rcl=0&l=2484
Uploaded new patch set with the requested code cleanup and updated comments. https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/560001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:68: &render_frame_id)) On 2016/07/05 07:12:48, dcheng wrote: > On 2016/07/04 14:28:32, adrian.belgun wrote: > > On 2016/07/04 04:24:06, dcheng wrote: > > > Sorry if this is a dumb question. Presumably this is called on the UI > thread. > > > What is the scenario where this function call succeeds (e.g. we find it in > the > > > instance map) but the process host / browser context lookups below return > > null? > > > > Neither renderer_process_host or browser_context should be NULL at this point. > > While experimenting with this I have not encountered that use-case. > > While the error checking on this may be superfluous, it sure isn't a hindrance > > to performance and can only increase robustness for that corner case. > > Also, looking at how this is used, those two are usually NULL-checked. > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/pepper/pep... > > There may not be a large perf cost, but there is a cost to the code reader: now > the reader needs to figure out if there is an exceptional case where this can > happen, and how to handle it. It also means we need to try to gracefully handle > cases that should never happen in practice, resulting in more lines of unused > code. > > There are a few other ResourceMessageFilters that call this function, and they > don't bother checking the result, or they guard it with NOTREACHED(). That's > probably the pattern we should be using here. Cleaned up the code here as follows: - Removed GetRenderFrameIDsForInstance check. (On failure render_process_id will be 0, thus RenderProcessHost::FromID will return NULL). - Removed browser_context check. (nullptr check eventually happens in KeyedServiceFactory::GetServiceForContext). The only needed check is for RenderProcessHost::FromID. https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1735473002/diff/560001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:2034: // VpnProvider Plugin -> Browser Messages On 2016/07/05 07:12:48, dcheng wrote: > On 2016/07/04 14:28:32, adrian.belgun wrote: > > On 2016/07/04 04:24:07, dcheng wrote: > > > Are these comments accurate? i see PpapiHostMsg and PpapiPluginMsg > interleaved > > > here. > > > > Initially it was split into 4 parts > > - Plugin -> Browser Messages > > - Browser -> Plugin Replies > > - Browser -> Plugin Messages > > - Plugin -> Browser Replies > > > https://codereview.chromium.org/1931513002/diff/180001/ppapi/proxy/ppapi_mess... > > > > An earlier reviewer recommended that "the more common pattern is to group > > messages with their reply message. (...) Then you would just have > > plugin->browser and browser->plugin groups." Thus leaving only two groups. > > > > What do you recommend? > > That's fine, but that makes this comment not entirely accurate. Maybe model it > after something like > https://cs.chromium.org/chromium/src/ppapi/proxy/ppapi_messages.h?rcl=0&l=2484 Done.
LGTM with a nit https://codereview.chromium.org/1735473002/diff/600001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/600001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:73: if (render_process_host) My (very strong) preference is to just DCHECK() that the result of GetRenderFrameIDsForInstance is true. Then we can skip this check. This if () statement should *always* be true, so having it structured like this still implies that this can somehow fail.
Uploaded new patch set with changes as per review. https://codereview.chromium.org/1735473002/diff/600001/content/browser/render... File content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc (right): https://codereview.chromium.org/1735473002/diff/600001/content/browser/render... content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc:73: if (render_process_host) On 2016/07/06 01:55:52, dcheng wrote: > My (very strong) preference is to just DCHECK() that the result of > GetRenderFrameIDsForInstance is true. Then we can skip this check. This if () > statement should *always* be true, so having it structured like this still > implies that this can somehow fail. Done. Added two DCHECKs (one for GetRenderFrameIDsForInstance and one for FromID). Should now be OK.
The CQ bit was checked by adrian.belgun@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, emaxx@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1735473002/#ps620001 (title: "Add DCHECKs.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ppapi: PPB_VpnProvider: Implement Resource Host BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Implement Resource Host BUG=506490 ==========
Message was sent while issue was closed.
Committed patchset #32 (id:620001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== ppapi: PPB_VpnProvider: Implement Resource Host BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Implement Resource Host BUG=506490 Committed: https://crrev.com/8a1558d86fa478c65aba0b268129a9d8f2d3a7af Cr-Commit-Position: refs/heads/master@{#403909} ==========
Message was sent while issue was closed.
Patchset 32 (id:??) landed as https://crrev.com/8a1558d86fa478c65aba0b268129a9d8f2d3a7af Cr-Commit-Position: refs/heads/master@{#403909}
Message was sent while issue was closed.
A revert of this CL (patchset #32 id:620001) has been created in https://codereview.chromium.org/2129653002/ by brucedawson@chromium.org. The reason for reverting is: Speculative revert to try to fix failing Android tests. https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2... android_webview_test_apk on Android android_webview_test_apk on Android failed 22 failures: org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllDouble with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextForward with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testClearMatches with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllFinds org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testClearFindNext org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllEmptyNext org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllDoubleNext with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextForward org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextBig org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextBig with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllDouble org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextBackward org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testClearFindNext with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllDoubleNext org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllEmptyNext with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextFirst org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindEmptyNext org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextFirst with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindNextBackward with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindEmptyNext with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testFindAllFinds with {--webview-sandboxed-renderer} org.chromium.android_webview.test.WebViewAsynchronousFindApisTest#testClearMatches. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
