Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7029032:
Port hello_world (C++) to use postMessage. (Closed)
|
Created:
9 years, 7 months ago by David Springer Modified:
9 years, 7 months ago CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionPort hello_world (C++) to use postMessage.
BUG=none
TEST=manual; build and run this example in an M13 browser version 85624 or
later.
Committed: http://code.google.com/p/nativeclient-sdk/source/detail?r=860
Patch Set 1 #
Total comments: 16
Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : '' #
Messages
Total messages: 13 (0 generated)
http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... File examples/hello_world/hello_world.cc (right): http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:40: /// Separator charater for the reverseText method. charater->character http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:56: pp::Var MarshallReverseText(const std::string& text) { Did you update the unit test? For what it's worth, this almost seems silly to have this function now... although we might have something similar for more complex examples. Thoughts? http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:79: /// calls the appropriate function. Posts the resutl back to the browser resutl->result http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:96: strlen(kReverseTextMethodId)) != std::string::npos) { You don't really need the latter 2 arguments, since kReverseTextMethodId is null-terminated. You also may just want to see if find returns 0 to see if it's at the start of the string. It's not important, but your current code would match, e.g. "Never gonna give you up, never gonna: reverseText"
nits http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... File examples/hello_world/hello_world.cc (right): http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:24: #include <ppapi/cpp/dev/scriptable_object_deprecated.h> Do we need this header? http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:81: /// @param[in] var_message The message posted by the browser. It would be nice to provide example of the possible content of the message, like: "FortyTwo" + "ReverseString:Hello world"
LGTM http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... File examples/hello_world/hello_world.html (right): http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.html:19: // Add a message handler that accespts messages coming from the NaCl accespts -> accepts
http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... File examples/hello_world/hello_world.cc (right): http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:24: #include <ppapi/cpp/dev/scriptable_object_deprecated.h> On 2011/05/18 15:45:19, garianov wrote: > Do we need this header? Uh no. Gone. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:40: /// Separator charater for the reverseText method. On 2011/05/18 15:43:56, dmichael1 wrote: > charater->character Done. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:56: pp::Var MarshallReverseText(const std::string& text) { On 2011/05/18 15:43:56, dmichael1 wrote: > Did you update the unit test? For what it's worth, this almost seems silly to > have this function now... although we might have something similar for more > complex examples. Thoughts? Oh - whoops. I spaced that. Done. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:79: /// calls the appropriate function. Posts the resutl back to the browser On 2011/05/18 15:43:56, dmichael1 wrote: > resutl->result Done. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:81: /// @param[in] var_message The message posted by the browser. On 2011/05/18 15:45:19, garianov wrote: > It would be nice to provide example of the possible content of the message, > like: "FortyTwo" + "ReverseString:Hello world" Done. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:96: strlen(kReverseTextMethodId)) != std::string::npos) { On 2011/05/18 15:43:56, dmichael1 wrote: > You don't really need the latter 2 arguments, since kReverseTextMethodId is > null-terminated. You also may just want to see if find returns 0 to see if it's > at the start of the string. It's not important, but your current code would > match, e.g. "Never gonna give you up, never gonna: reverseText" Done. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... File examples/hello_world/hello_world.html (right): http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.html:19: // Add a message handler that accespts messages coming from the NaCl On 2011/05/18 16:12:22, Matt Ball wrote: > accespts -> accepts Done.
http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... File examples/hello_world/hello_world.cc (right): http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:25: #include <ppapi/cpp/var.h> Whoops, just noticed... if we're using "" for ppapi headers now, these chould change and move down. http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... examples/hello_world/hello_world.cc:56: pp::Var MarshallReverseText(const std::string& text) { On 2011/05/18 18:01:42, David Springer wrote: > On 2011/05/18 15:43:56, dmichael1 wrote: > > Did you update the unit test? For what it's worth, this almost seems silly to > > have this function now... although we might have something similar for more > > complex examples. Thoughts? > > Oh - whoops. I spaced that. Done. Could you add it to the CL please? And... did the build succeed before the change? That seems bad, if you didn't find out from compilation.
On Wed, May 18, 2011 at 12:46 PM, <dmichael@google.com> wrote: > > > http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... > examples/hello_world/hello_world.cc:56: pp::Var > MarshallReverseText(const std::string& text) { > On 2011/05/18 18:01:42, David Springer wrote: > >> On 2011/05/18 15:43:56, dmichael1 wrote: >> > Did you update the unit test? For what it's worth, this almost >> > seems silly to > >> > have this function now... although we might have something similar >> > for more > >> > complex examples. Thoughts? >> > > Oh - whoops. I spaced that. Done. >> > Could you add it to the CL please? And... did the build succeed before > the change? That seems bad, if you didn't find out from compilation. > > Along those lines, this might also be a good time to add a unit test to build_tools/tests/installer_test.py that runs the hello_world sel_ldr unit test > > http://codereview.chromium.org/7029032/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
On 2011/05/18 18:50:12, Matt Ball wrote: > On Wed, May 18, 2011 at 12:46 PM, <mailto:dmichael@google.com> wrote: > > > > > > > > http://codereview.chromium.org/7029032/diff/1/examples/hello_world/hello_worl... > > examples/hello_world/hello_world.cc:56: pp::Var > > MarshallReverseText(const std::string& text) { > > On 2011/05/18 18:01:42, David Springer wrote: > > > >> On 2011/05/18 15:43:56, dmichael1 wrote: > >> > Did you update the unit test? For what it's worth, this almost > >> > > seems silly to > > > >> > have this function now... although we might have something similar > >> > > for more > > > >> > complex examples. Thoughts? > >> > > > > Oh - whoops. I spaced that. Done. > >> > > Could you add it to the CL please? And... did the build succeed before > > the change? That seems bad, if you didn't find out from compilation. > > > > Along those lines, this might also be a good time to add a unit test to > build_tools/tests/installer_test.py that runs the hello_world sel_ldr unit > test > No changes were necessary - the tests build and run fine as they are. > > > > > http://codereview.chromium.org/7029032/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Native-Client-Reviews" group. > To post to this group, send email to mailto:native-client-reviews@googlegroups.com. > To unsubscribe from this group, send email to > mailto:native-client-reviews+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/native-client-reviews?hl=en. >
Ah, right, I saw the changed signature on 'Marshall...' and thought the test had to change, but it's calling the underlying methods instead. Sorry for my confusion. aside from the nit about <> vs "" includes, LGTM
one nit http://codereview.chromium.org/7029032/diff/5001/examples/hello_world/hello_w... File examples/hello_world/hello_world.cc (right): http://codereview.chromium.org/7029032/diff/5001/examples/hello_world/hello_w... examples/hello_world/hello_world.cc:28: #include <algorithm> // for reverse Actually, this header is not user here either. It should be included by helper_functions
Issues addressed. Also, change #include <ppapi> to #include "ppapi" per new guidelines. PTAL. Thanks! http://codereview.chromium.org/7029032/diff/5001/examples/hello_world/hello_w... File examples/hello_world/hello_world.cc (right): http://codereview.chromium.org/7029032/diff/5001/examples/hello_world/hello_w... examples/hello_world/hello_world.cc:28: #include <algorithm> // for reverse On 2011/05/18 19:43:36, mlinck wrote: > Actually, this header is not user here either. It should be included by > helper_functions Done.
LGTM On 2011/05/18 19:56:30, David Springer wrote: > Issues addressed. Also, change #include <ppapi> to #include "ppapi" per new > guidelines. > > PTAL. > > Thanks! > > http://codereview.chromium.org/7029032/diff/5001/examples/hello_world/hello_w... > File examples/hello_world/hello_world.cc (right): > > http://codereview.chromium.org/7029032/diff/5001/examples/hello_world/hello_w... > examples/hello_world/hello_world.cc:28: #include <algorithm> // for reverse > On 2011/05/18 19:43:36, mlinck wrote: > > Actually, this header is not user here either. It should be included by > > helper_functions > > Done.
Thanks for the reviews! Committed as r860 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
