Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23523064:
Porting the C++ tutorial to PNaCl (Closed)
|
Created:
7 years, 3 months ago by eliben Modified:
7 years, 3 months ago CC:
chromium-reviews, native-client-reviews_googlegroups.com, binji, Sam Clegg Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPort the NaCl C++ tutorial (devguide/tutorial.rst) to discuss PNaCl and the new SDK getting_started/ code.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3634
R=binji@chromium.org, jvoung@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224459
Patch Set 1 #Patch Set 2 : More stuff done #Patch Set 3 : moar #Patch Set 4 : Ready for first review #
Total comments: 26
Patch Set 5 : After Ben's and Jan's first review #Patch Set 6 : Fix for Jan's 2nd review #
Total comments: 1
Patch Set 7 : Fix for Ben's 2nd review #
Messages
Total messages: 13 (0 generated)
On 2013/09/19 20:42:51, eliben wrote: Please look at the embedded TODOs - some ask for advice
https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... File native_client_sdk/src/doc/devguide/tutorial.rst (right): https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:39: JavaScript and the Native Clinet module (C/C++ code). Both sides can initiate Clinet -> Client https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:48: TODO: would it be better to compare to web-worker communication? The message-system doc does compare it to web-worker PostMessage communication. It might help to briefly mention that here too, so that this is a bit more self-contained. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:78: latest one. Latest one is canary which might churn more? Or do we recommend the "stable" one, or the one matching the version of chrome version that you want to target? https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:162: Assuming the local server was started according to the instructions in :ref:`Step 2 80 col https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:179: HelloTutorialModule = document.getElementById('hello_tutorial'); should this JS have "var" https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:235: <https://developers.google.com/native-client/peppercpp/classpp_1_1_instance.ht... hope the hashes are stable =) https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:257: using. Do we have docs on what the chrome console prints when NaCl doesn't load, or other ways to check for error messages? Pointer to the JS load-events (like lastError). Or, pointer to debugging notes?
https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... File native_client_sdk/src/doc/devguide/tutorial.rst (right): https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:48: TODO: would it be better to compare to web-worker communication? I think this is better. It is actually very similar to window.postMessage (https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage) but I doubt that would help the average developer reading this doc. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:78: latest one. I'm not sure this is the best advice. Shouldn't we suggest to use the "stable" one? https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:142: except correctly initializing itself. The JavaScript code waits for the Native correctly initialize https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:165: successfully and the Status text should change from "LOADING..." to "SUCCESS". Probably need a troubleshooting. What happens if I don't see success? * Are you running Chrome 31? * Do you have a .nmf file? * Do you have a .pexe? * Are they being loaded? (Check the devtools) Ah, I see it is below. Still might be nice to call something out here. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:179: HelloTutorialModule = document.getElementById('hello_tutorial'); On 2013/09/19 21:46:35, jvoung (cr) wrote: > should this JS have "var" Nope, it's global. :) https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:192: * Implement the ``HandleMessage()`` method of the module instance. Actually I think C++ people prefer "member function" or "function". Java people prefer "method". :)
https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... File native_client_sdk/src/doc/devguide/tutorial.rst (right): https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:39: JavaScript and the Native Clinet module (C/C++ code). Both sides can initiate On 2013/09/19 21:46:35, jvoung (cr) wrote: > Clinet -> Client Dnoe https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:48: TODO: would it be better to compare to web-worker communication? On 2013/09/19 22:06:50, binji wrote: > I think this is better. It is actually very similar to window.postMessage > (https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage) but I > doubt that would help the average developer reading this doc. I'm sorry, too much context to disambiguate "this". What is better :) ? https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:78: latest one. On 2013/09/19 22:06:50, binji wrote: > I'm not sure this is the best advice. Shouldn't we suggest to use the "stable" > one? Yes. How do I say "latest stable" one? The one with the latest number? https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:142: except correctly initializing itself. The JavaScript code waits for the Native On 2013/09/19 22:06:50, binji wrote: > correctly initialize Done. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:162: Assuming the local server was started according to the instructions in :ref:`Step 2 On 2013/09/19 21:46:35, jvoung (cr) wrote: > 80 col Done. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:165: successfully and the Status text should change from "LOADING..." to "SUCCESS". On 2013/09/19 22:06:50, binji wrote: > Probably need a troubleshooting. What happens if I don't see success? > * Are you running Chrome 31? > * Do you have a .nmf file? > * Do you have a .pexe? > * Are they being loaded? (Check the devtools) > > Ah, I see it is below. Still might be nice to call something out here. OK, will add a link to Troubleshooting https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:179: HelloTutorialModule = document.getElementById('hello_tutorial'); On 2013/09/19 22:06:50, binji wrote: > On 2013/09/19 21:46:35, jvoung (cr) wrote: > > should this JS have "var" > > Nope, it's global. :) F* JS https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:192: * Implement the ``HandleMessage()`` method of the module instance. On 2013/09/19 22:06:50, binji wrote: > Actually I think C++ people prefer "member function" or "function". Java people > prefer "method". :) good point. will change to member function to be more explicit. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:235: <https://developers.google.com/native-client/peppercpp/classpp_1_1_instance.ht... On 2013/09/19 21:46:35, jvoung (cr) wrote: > hope the hashes are stable =) I'm sure they are ;-) https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:257: using. On 2013/09/19 21:46:35, jvoung (cr) wrote: > Do we have docs on what the chrome console prints when NaCl doesn't load, No. We should. Now who would be the best person to do that .... ;-) > or > other ways to check for error messages? Pointer to the JS load-events (like > lastError). Or, pointer to debugging notes? What kind of debugging do you mean here?
https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... File native_client_sdk/src/doc/devguide/tutorial.rst (right): https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:257: using. On 2013/09/19 23:19:21, eliben wrote: > On 2013/09/19 21:46:35, jvoung (cr) wrote: > > Do we have docs on what the chrome console prints when NaCl doesn't load, > > No. We should. Now who would be the best person to do that .... ;-) I hear that ncbray has sent many emails describing how to do that ;-) Some of the errors are not as big of a problem with PNaCl (did you deploy through the open web, or webstore? If open web, are you using --enable-nacl?), etc. Otherwise, a quick note on checking the JS console with ctrl-shift-J would be helpful. Also, there is some description of lastError here: https://developers.google.com/native-client/devguide/coding/progress-events under the "lastError" section. Developers could add JS code that console.log()s the lastError value. > > > or > > other ways to check for error messages? Pointer to the JS load-events (like > > lastError). Or, pointer to debugging notes? > > What kind of debugging do you mean here? I meant debugging the app itself, like with printf or gdb: https://developers.google.com/native-client/devguide/devcycle/debugging Depends on whether the console message says "NaCl module crashed" or something else. If it crashed, it might be worth debugging the app itself. Otherwise, gotta check the chrome version, the servers or something.
On 2013/09/19 23:29:52, jvoung (cr) wrote: > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > File native_client_sdk/src/doc/devguide/tutorial.rst (right): > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > native_client_sdk/src/doc/devguide/tutorial.rst:257: using. > On 2013/09/19 23:19:21, eliben wrote: > > On 2013/09/19 21:46:35, jvoung (cr) wrote: > > > Do we have docs on what the chrome console prints when NaCl doesn't load, > > > > No. We should. Now who would be the best person to do that .... ;-) > > > I hear that ncbray has sent many emails describing how to do that ;-) > Some of the errors are not as big of a problem with PNaCl (did you deploy > through the open web, or webstore? If open web, are you using --enable-nacl?), > etc. Otherwise, a quick note on checking the JS console > with ctrl-shift-J would be helpful. > > Also, there is some description of lastError here: > https://developers.google.com/native-client/devguide/coding/progress-events > under the "lastError" section. Developers could add JS code that console.log()s > the lastError value. > > > > > > > or > > > other ways to check for error messages? Pointer to the JS load-events (like > > > lastError). Or, pointer to debugging notes? > > > > What kind of debugging do you mean here? > > I meant debugging the app itself, like with printf or gdb: > > https://developers.google.com/native-client/devguide/devcycle/debugging > > Depends on whether the console message says "NaCl module crashed" or something > else. If it crashed, it might be worth debugging the app itself. Otherwise, > gotta check the chrome version, the servers or something. PTAL
Thanks, responses to my comments lgtm
lgtm, thanks https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... File native_client_sdk/src/doc/devguide/tutorial.rst (right): https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:48: TODO: would it be better to compare to web-worker communication? On 2013/09/19 23:19:21, eliben wrote: > On 2013/09/19 22:06:50, binji wrote: > > I think this is better. It is actually very similar to window.postMessage > > (https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage) but I > > doubt that would help the average developer reading this doc. > > I'm sorry, too much context to disambiguate "this". What is better :) ? Apologies, I think the way you have written it now is better than referencing web-workers. Though if we do it elsewhere in the documentation, maybe it's best to be consistent. https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... native_client_sdk/src/doc/devguide/tutorial.rst:78: latest one. On 2013/09/19 23:19:21, eliben wrote: > On 2013/09/19 22:06:50, binji wrote: > > I'm not sure this is the best advice. Shouldn't we suggest to use the "stable" > > one? > > Yes. How do I say "latest stable" one? The one with the latest number? No, when you run "nacl_sdk list" it will display something like this: $ ./naclsdk list Bundles: I: installed *: update available I sdk_tools (stable) vs_addin (dev) I pepper_25 (post_stable) pepper_26 (post_stable) pepper_27 (post_stable) pepper_28 (post_stable) I* pepper_29 (stable) pepper_30 (dev) I* pepper_canary (canary) So referencing "stable" is here is probably OK. I believe we explain this in more detail in the download/install doc. https://codereview.chromium.org/23523064/diff/22001/native_client_sdk/src/doc... File native_client_sdk/src/doc/devguide/tutorial.rst (right): https://codereview.chromium.org/23523064/diff/22001/native_client_sdk/src/doc... native_client_sdk/src/doc/devguide/tutorial.rst:264: ``Tools`` menu or by pressing ``Shift+Ctrl+J``). Examine it for clues about what The command is different on Mac as well (Shift+Cmd+J, I think).
On 2013/09/20 17:14:44, binji wrote: > lgtm, thanks > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > File native_client_sdk/src/doc/devguide/tutorial.rst (right): > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > native_client_sdk/src/doc/devguide/tutorial.rst:48: TODO: would it be better to > compare to web-worker communication? > On 2013/09/19 23:19:21, eliben wrote: > > On 2013/09/19 22:06:50, binji wrote: > > > I think this is better. It is actually very similar to window.postMessage > > > (https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage) but I > > > doubt that would help the average developer reading this doc. > > > > I'm sorry, too much context to disambiguate "this". What is better :) ? > > Apologies, I think the way you have written it now is better than referencing > web-workers. Though if we do it elsewhere in the documentation, maybe it's best > to be consistent. > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > native_client_sdk/src/doc/devguide/tutorial.rst:78: latest one. > On 2013/09/19 23:19:21, eliben wrote: > > On 2013/09/19 22:06:50, binji wrote: > > > I'm not sure this is the best advice. Shouldn't we suggest to use the > "stable" > > > one? > > > > Yes. How do I say "latest stable" one? The one with the latest number? > > No, when you run "nacl_sdk list" it will display something like this: > $ ./naclsdk list > Bundles: > I: installed > *: update available > > I sdk_tools (stable) > vs_addin (dev) > I pepper_25 (post_stable) > pepper_26 (post_stable) > pepper_27 (post_stable) > pepper_28 (post_stable) > I* pepper_29 (stable) > pepper_30 (dev) > I* pepper_canary (canary) > > So referencing "stable" is here is probably OK. I believe we explain this in > more detail in the download/install doc. Thanks, PTAL rephrasing. > > https://codereview.chromium.org/23523064/diff/22001/native_client_sdk/src/doc... > File native_client_sdk/src/doc/devguide/tutorial.rst (right): > > https://codereview.chromium.org/23523064/diff/22001/native_client_sdk/src/doc... > native_client_sdk/src/doc/devguide/tutorial.rst:264: ``Tools`` menu or by > pressing ``Shift+Ctrl+J``). Examine it for clues about what > The command is different on Mac as well (Shift+Cmd+J, I think). OK, thanks. I'll remove the key reference, this isn't a Chrome developer tutorial :)
On 2013/09/20 17:14:44, binji wrote: > lgtm, thanks > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > File native_client_sdk/src/doc/devguide/tutorial.rst (right): > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > native_client_sdk/src/doc/devguide/tutorial.rst:48: TODO: would it be better to > compare to web-worker communication? > On 2013/09/19 23:19:21, eliben wrote: > > On 2013/09/19 22:06:50, binji wrote: > > > I think this is better. It is actually very similar to window.postMessage > > > (https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage) but I > > > doubt that would help the average developer reading this doc. > > > > I'm sorry, too much context to disambiguate "this". What is better :) ? > > Apologies, I think the way you have written it now is better than referencing > web-workers. Though if we do it elsewhere in the documentation, maybe it's best > to be consistent. > > https://codereview.chromium.org/23523064/diff/8001/native_client_sdk/src/doc/... > native_client_sdk/src/doc/devguide/tutorial.rst:78: latest one. > On 2013/09/19 23:19:21, eliben wrote: > > On 2013/09/19 22:06:50, binji wrote: > > > I'm not sure this is the best advice. Shouldn't we suggest to use the > "stable" > > > one? > > > > Yes. How do I say "latest stable" one? The one with the latest number? > > No, when you run "nacl_sdk list" it will display something like this: > $ ./naclsdk list > Bundles: > I: installed > *: update available > > I sdk_tools (stable) > vs_addin (dev) > I pepper_25 (post_stable) > pepper_26 (post_stable) > pepper_27 (post_stable) > pepper_28 (post_stable) > I* pepper_29 (stable) > pepper_30 (dev) > I* pepper_canary (canary) > > So referencing "stable" is here is probably OK. I believe we explain this in > more detail in the download/install doc. Thanks, PTAL rephrasing. > > https://codereview.chromium.org/23523064/diff/22001/native_client_sdk/src/doc... > File native_client_sdk/src/doc/devguide/tutorial.rst (right): > > https://codereview.chromium.org/23523064/diff/22001/native_client_sdk/src/doc... > native_client_sdk/src/doc/devguide/tutorial.rst:264: ``Tools`` menu or by > pressing ``Shift+Ctrl+J``). Examine it for clues about what > The command is different on Mac as well (Shift+Cmd+J, I think). OK, thanks. I'll remove the key reference, this isn't a Chrome developer tutorial :)
ok, lgtm.
Message was sent while issue was closed.
Committed patchset #7 manually as r224459 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
