Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1731273002:
ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. (Closed)
|
Created:
4 years, 10 months ago by adrian.belgun Modified:
4 years, 5 months ago CC:
chromium-reviews, binji+watch_chromium.org, Sam Clegg Base URL:
https://chromium.googlesource.com/chromium/src.git@vpn-ppapi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionppapi: PPB_VpnProvider: Add a simple NaCl SDK example.
Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>;
BUG=506490
Committed: https://crrev.com/bb921dfc13aad262dfff7270a8949b6032c6fb23
Cr-Commit-Position: refs/heads/master@{#406516}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Sync to new API. #Patch Set 3 : New simplied API. Full connection workflow. #
Total comments: 40
Patch Set 4 : Respond to reviews. #Messages
Total messages: 35 (14 generated)
adrian.belgun@intel.com changed reviewers: + bartfab@chromium.org, dskaram@chromium.org, robert.bradford@intel.com
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/
adrian.belgun@intel.com changed reviewers: + sbc@chromium.org
bartfab@chromium.org changed reviewers: + cernekee@chromium.org, emaxx@chromium.org
Thanks for adding this. Can you add "CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk" to your CL description and do a CQ dry run? https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc (right): https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:243: return new HelloTutorialModule(); Rename the module and instance to match the name of the example?
Description was changed from ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk BUG=506490 ==========
binji@chromium.org changed reviewers: + binji@chromium.org
https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... File native_client_sdk/src/examples/api/vpn_provider/example.js (right): https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/example.js:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. 2016, and remove (c) https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/example.js:19: function load_me() { nit: loadMe (and unloadMe below) https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... File native_client_sdk/src/examples/api/vpn_provider/index.html (right): https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/index.html:4: Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/index.html:15: <!-- remove this, we only include this message in getting_started. https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/index.html:25: <body data-name="vpn_provider" this should be <body {{atrs}}>, take a look at some the other examples in this directory. https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/index.html:30: <!-- remove this comment https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/index.html:43: <button id="load" onclick="load_me()" >Create</button> Add documentation to the page explaining what it is doing. https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc (right): https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. 2016, remove (c) https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:5: #include <time.h> nit: sort alphabetically, and separate libc includes from ppapi includes by one newline https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:32: std::cerr << "VPN Provider NaCL: " nit: NaCl (lowercase l) here and elsewhere https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:35: // Initial Callaback registration sp: callback https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:37: &HelloTutorialInstance::GetPlatformMessageCompletionCallback)); These names are a bit verbose, maybe just "OnGetPlatformMessage", etc.? https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:50: std::cerr << "VPN Provider NaCL: " It's nicer if these messages are sent to the page. That said, it's more work, and not all of our examples do it. https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:54: vpn_ = nullptr; prefer NULL, one of our compilers is quite old (~2008) https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:122: << dict.Get(pp::Var("id")).AsString() << ", " you can just write dict.Get("id").AsString(), there's an implicit conversion from const char* -> pp::Var https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:130: exclusionList.push_back(std::string("63.145.213.129/32")); same here, use implicit conversion to std::string https://codereview.chromium.org/1731273002/diff/1/native_client_sdk/src/examp... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:135: inclusionList.push_back("63.145.212.128/25"); what do these IP addresses point to? Is this usable by any user of the SDK?
Uploaded a new patch set. Please review. This is a complete rewrite of the SDK example. With the new API the configuration creation and configuration is handled by JS, NaCl only handling the actual packet transfer.
Description was changed from ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk BUG=506490 ==========
On 2016/07/15 14:28:57, adrian.belgun wrote: > Uploaded a new patch set. Please review. > > This is a complete rewrite of the SDK example. With the new API the > configuration creation and configuration is handled by JS, NaCl only handling > the actual packet transfer. Doe it make sense to have two separate examples? The JS part of that API is still alive right? Or perhaps stick to one example, keep the JS packet transfer functions in the code but remove their public documentation? The one downside is that we'd be forcing all app developers to use NaCl, but how bad can that be if there is an example to get them started? Besides any reasonable VPN implementation will have a NaCl component anyway for the crypto stuff.
On 2016/07/15 14:48:14, dskaram wrote: > On 2016/07/15 14:28:57, adrian.belgun wrote: > > Uploaded a new patch set. Please review. > > > > This is a complete rewrite of the SDK example. With the new API the > > configuration creation and configuration is handled by JS, NaCl only handling > > the actual packet transfer. > > Doe it make sense to have two separate examples? The JS part of that API is > still alive right? > > Or perhaps stick to one example, keep the JS packet transfer functions in the > code but remove their public documentation? The one downside is that we'd be > forcing all app developers to use NaCl, but how bad can that be if there is an > example to get them started? Besides any reasonable VPN implementation will have > a NaCl component anyway for the crypto stuff. Not sure about what you mean with "two separate examples". Is there another that uses just the JS API? It's best not to remove the existing JS API path, even just from the documentation. I think that we should just say on the JS API page something like "if you want better performance, consider using the NaCl API". It would be clearer for extension writers to have as a stating point a full blown JS API as present and then for them to migrate to using this API if they need the performance.
https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/example.js (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:6: var configName = "Mock configuration"; nit: Google JS style guide suggests to prefer the single quotes over the double quotes. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:10: vpnParams = { nit: add "var". https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:26: // Create a VPN configurations using the createConfig method. nit: s/configurations/configuration/. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:52: ); nit: the closing bracket usually stays on the same line as the last argument. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:74: common.naclModule.postMessage({cmd: 'connected'}); nit: Please correct the indentation here and in line 80. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:102: chrome.vpnProvider.onPacketReceived.addListener(function(data) { Is it correct that this JavaScript event should never occur in this example? Then it makes sense to show this explicitly (e.g. by logging an error message or by throwing an exception). https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:103: wlog("onPacketRecevied - JS"); nit: s/onPacketRecevied/onPacketReceived/ https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:112: " data='" + data + "'"); nit: the data argument is not a string, so it should be serialized differently (e.g. through JSON.stringify) and it shouldn't be surrounded with quotes. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:112: " data='" + data + "'"); nit: please fix the indentation. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:23: // Handles On Click messages from Javascript The comment looks to be a bit confusing, as the messages are sent to the NaCl module in a bunch of other scenarios except the user clicks. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:49: std::string command = cmd.AsString(); nit: #include <string> https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:39: // Initial Callaback registration nit: s/Callaback/callback/ https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:76: dict.Set("cmd", "bindSucces"); nit: s/bindSucces/bindSuccess/ (I'm wondering how the example worked correctly with this incompatibility - because the JS side would never report to Chrome that the state should be changed to "connected".) https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:91: * message.Set(pp::Var("operation"), pp::Var("write")); nit: not sure whether you put conversions to pp::Var for clarity, but if not - they are not required. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h:26: void InitOnThread(int32_t result); nit: include <stdint.h> https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h:45: std::string config_name_; nit: #include <string>
https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/index.html (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/index.html:18: <h2>Status: <code id="statusField">NO-STATUS</code></h2> We usually try to have a brief description of what the example is doing (and how to use it) here. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:65: pp::Var id = dict.Get("id"); duplicated 3 times, probably worth making a function. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:28: factory_.NewCallback(&VpnProviderHelper::BindOnThread, config, config)); should be "...config, name"? https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h:16: // Helper class that keeps VpnPacket handling in it's own thread. nit: its
Thank you for reviewing this! Uploaded new patch set with the requested changes. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/example.js (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:6: var configName = "Mock configuration"; On 2016/07/18 13:46:58, emaxx wrote: > nit: Google JS style guide suggests to prefer the single quotes over the double > quotes. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:10: vpnParams = { On 2016/07/18 13:46:58, emaxx wrote: > nit: add "var". Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:26: // Create a VPN configurations using the createConfig method. On 2016/07/18 13:46:58, emaxx wrote: > nit: s/configurations/configuration/. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:52: ); On 2016/07/18 13:46:58, emaxx wrote: > nit: the closing bracket usually stays on the same line as the last argument. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:74: common.naclModule.postMessage({cmd: 'connected'}); On 2016/07/18 13:46:58, emaxx wrote: > nit: Please correct the indentation here and in line 80. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:102: chrome.vpnProvider.onPacketReceived.addListener(function(data) { On 2016/07/18 13:46:58, emaxx wrote: > Is it correct that this JavaScript event should never occur in this example? > Then it makes sense to show this explicitly (e.g. by logging an error message or > by throwing an exception). Yes. That's what I'm doing here. It's logging to the HTML file. Added extra logging - also to the console. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:103: wlog("onPacketRecevied - JS"); On 2016/07/18 13:46:58, emaxx wrote: > nit: s/onPacketRecevied/onPacketReceived/ Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:112: " data='" + data + "'"); On 2016/07/18 13:46:58, emaxx wrote: > nit: please fix the indentation. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/example.js:112: " data='" + data + "'"); On 2016/07/18 13:46:58, emaxx wrote: > nit: the data argument is not a string, so it should be serialized differently > (e.g. through JSON.stringify) and it shouldn't be surrounded with quotes. Acknowledged. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/index.html (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/index.html:18: <h2>Status: <code id="statusField">NO-STATUS</code></h2> On 2016/07/18 18:46:28, binji wrote: > We usually try to have a brief description of what the example is doing (and how > to use it) here. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:23: // Handles On Click messages from Javascript On 2016/07/18 13:46:58, emaxx wrote: > The comment looks to be a bit confusing, as the messages are sent to the NaCl > module in a bunch of other scenarios except the user clicks. Done. Was a leftover from an older version. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:49: std::string command = cmd.AsString(); On 2016/07/18 13:46:58, emaxx wrote: > nit: #include <string> Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider.cc:65: pp::Var id = dict.Get("id"); On 2016/07/18 18:46:28, binji wrote: > duplicated 3 times, probably worth making a function. Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:28: factory_.NewCallback(&VpnProviderHelper::BindOnThread, config, config)); On 2016/07/18 18:46:28, binji wrote: > should be "...config, name"? Done. It worked as written because currently the configuration ID is the same as the configuration name. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:39: // Initial Callaback registration On 2016/07/18 13:46:58, emaxx wrote: > nit: s/Callaback/callback/ Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:76: dict.Set("cmd", "bindSucces"); On 2016/07/18 13:46:58, emaxx wrote: > nit: s/bindSucces/bindSuccess/ > (I'm wondering how the example worked correctly with this incompatibility - > because the JS side would never report to Chrome that the state should be > changed to "connected".) Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.cc:91: * message.Set(pp::Var("operation"), pp::Var("write")); On 2016/07/18 13:46:58, emaxx wrote: > nit: not sure whether you put conversions to pp::Var for clarity, but if not - > they are not required. Done. Initially there were for clarity, removed them from the rest of the code, but forgot to remove them from the comments. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h (right): https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h:16: // Helper class that keeps VpnPacket handling in it's own thread. On 2016/07/18 18:46:28, binji wrote: > nit: its Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h:26: void InitOnThread(int32_t result); On 2016/07/18 13:46:58, emaxx wrote: > nit: include <stdint.h> Done. https://codereview.chromium.org/1731273002/diff/30001/native_client_sdk/src/e... native_client_sdk/src/examples/api/vpn_provider/vpn_provider_helper.h:45: std::string config_name_; On 2016/07/18 13:46:58, emaxx wrote: > nit: #include <string> Done.
lgtm
The CQ bit was checked by binji@chromium.org 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_nacl_sdk on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_nacl_sdk/buil...)
hm, looks like the NaCl SDK trybots aren't working properly. lgtm
On 2016/07/19 19:00:12, binji wrote: > hm, looks like the NaCl SDK trybots aren't working properly. Thanks for the review! What should I do in this case? Should I just try the dry run again at a later date in hope that the bots would have been fixed by then?
On 2016/07/19 20:37:32, adrian.belgun wrote: > On 2016/07/19 19:00:12, binji wrote: > > hm, looks like the NaCl SDK trybots aren't working properly. > > Thanks for the review! What should I do in this case? > Should I just try the dry run again at a later date in hope that the bots would > have been fixed by then? I opened: https://bugs.chromium.org/p/chromium/issues/detail?id=629626 It looks like the chrome binary can't be found for the "Run Tests" step. Compilation and unittests that don't require chrome binary pass fine.
On 2016/07/19 21:13:07, robert.bradford wrote: > On 2016/07/19 20:37:32, adrian.belgun wrote: > > On 2016/07/19 19:00:12, binji wrote: > > > hm, looks like the NaCl SDK trybots aren't working properly. > > > > Thanks for the review! What should I do in this case? > > Should I just try the dry run again at a later date in hope that the bots > would > > have been fixed by then? > > I opened: https://bugs.chromium.org/p/chromium/issues/detail?id=629626 > > It looks like the chrome binary can't be found for the "Run Tests" step. > Compilation and unittests that don't require chrome binary pass fine. Right. I think it's OK to land without waiting, all those tests do is load the example to ensure that it doesn't crash immediately.
Description was changed from ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> BUG=506490 ==========
The CQ bit was checked by robert.bradford@intel.com
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: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> BUG=506490 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
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: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> BUG=506490 ========== to ========== ppapi: PPB_VpnProvider: Add a simple NaCl SDK example. Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com> BUG=506490 Committed: https://crrev.com/bb921dfc13aad262dfff7270a8949b6032c6fb23 Cr-Commit-Position: refs/heads/master@{#406516} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bb921dfc13aad262dfff7270a8949b6032c6fb23 Cr-Commit-Position: refs/heads/master@{#406516} |
