Support GLBIC example.

Support GLBIC example in SDK

This CL added two new examples for the NaCl SDK.  This only affects SDK bots since
the code is completely contained within native_client_sdk subtree.

Fix incorrectly placed ppapi headers.
Remove TODOs and cleanup create_nmf.py.
Add hello_world_glibc example
Add dlopen example.

BUG=111224
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120117

[Brad Chen, 2012-01-28 01:00:52]

I'm sorry about being somewhat dogmatic about the style stuff, but I think it is
quite important for our external examples. IF we are not using Google3 style
then please refer me to the style rules you are following.

Biggest feedback item is whether you could simplify the threaded structure of
dlopen.cc. See comment at bottom of dlopen.cc.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/Makefile (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/Makefile:1: # Copyright (c) 2011 The
Native Client Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/dlopen.cc (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:3: // found in the LICENSE file.
Where should a developer expect to see an overview describing this example? I'm
noting there is no README file, no description of this file's contents, no
explanation of what the various files are for. Your thoughts?

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:18: // XHR request to
dynamically load the .so file from the server, and return
XHR needs a definition or cross-reference.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:20: // thread until the .so file
has been cached. (otherwise you will stall the
Not grammatical.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:26: // a global handle to the
CORE  object; this will make it easier to issue
Especially in external examples I think we should observe the Google style
guidelines of making comments proper English grammar with normal
punctuation/capitalization, avoid abbreviations etc. I'd be happy to rewrite the
comments if that would be helpful.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:32: char *(*eightball)(void);
The use of indentation and spaces in the above three lines seems arbitrary and
strange.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:33: // the Instance class.
If this is the first NaCl example a developer looks at this comment will be
completely useless. How about adding a cross-reference or more explanation?

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:34: class dlOpenInstance :
public pp::Instance {
It sure makes me sad we can't do a simple example without all of this C++
complexity.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:37: eightball=(NULL);
" = "

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:49: // .so and it's exposed
function.  Note that this function is called
"its" ; also non-standard capitalization. Maybe we could ask Andy to edit the
comments?

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:53: logmsg("calling dlopen()
from main thread\n");
Why is this not indented? Why the white space before this line?

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:58: if(dlhandle == NULL){
"if (dlhandle == NULL) {"
I really think we need to make this example more like Google3 throughout. Let me
know if you want me to note other problems with spaces and formatting; there are
more that follow.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:114: // A global instance of the
dlOpenInstance object; This will make the callbacks easier
long line, grammar, punctuation, capitalization etc.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:148: 
Please get rid of the extra whitespace.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:157: g_core->CallOnMainThread(0,
cc , 0);
What do you actually need to do on the main thread? Would it be possible to make
this example much simpler by using the main thread only to launch a child, and
then do everything synchronously on the child?

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/dlopen.html (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.html:1: <html>
Please ask an HTML person to review this.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/eightball.cc (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.cc:1: #include <stdlib.h>
Copyright headers on all source files.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.cc:6:
__attribute__((visibility("default")))const char *Magic8Ball() {
If you need to keep the __attribute__ stuff, maybe you can put the "const char
..." on a newline so people can more easily parse the GNU extensions vs. the
standard C.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.cc:21: }
Not worth using the {} for extern "C" if there is only one declaration.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/eightball.h (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.h:6:
__attribute__((visibility("default")))const char* Magic8Ball();
It's kind of nasty seeing this __attribute__ stuff all over the place. Could you
please check with David Sehr or Roland if you haven't already? They would know
if there was something simpler we could do instead.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/hello_world_glibc/hello_world.html (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/hello_world_glibc/hello_world.html:1: <!DOCTYPE
html>
This HTML looks much more readable than the dlopen.html example file.

[noelallen1, 2012-01-31 21:03:20]

The dlOpen example has pretty much been rewritten.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/Makefile (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/Makefile:1: # Copyright (c) 2011 The
Native Client Authors. All rights reserved.
On 2012/01/28 01:00:52, Brad Chen wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/dlopen.cc (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:3: // found in the LICENSE file.
None of the examples have docs locally.  The docs are currently hosted on the
devsite.

On 2012/01/28 01:00:52, Brad Chen wrote:
> Where should a developer expect to see an overview describing this example?
I'm
> noting there is no README file, no description of this file's contents, no
> explanation of what the various files are for. Your thoughts?

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:18: // XHR request to
dynamically load the .so file from the server, and return
On 2012/01/28 01:00:52, Brad Chen wrote:
> XHR needs a definition or cross-reference.

removed

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:20: // thread until the .so file
has been cached. (otherwise you will stall the
On 2012/01/28 01:00:52, Brad Chen wrote:
> Not grammatical.

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:26: // a global handle to the
CORE  object; this will make it easier to issue
Globals removed.
On 2012/01/28 01:00:52, Brad Chen wrote:
> Especially in external examples I think we should observe the Google style
> guidelines of making comments proper English grammar with normal
> punctuation/capitalization, avoid abbreviations etc. I'd be happy to rewrite
the
> comments if that would be helpful.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:32: char *(*eightball)(void);
On 2012/01/28 01:00:52, Brad Chen wrote:
> The use of indentation and spaces in the above three lines seems arbitrary and
> strange.

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:33: // the Instance class.
On 2012/01/28 01:00:52, Brad Chen wrote:
> If this is the first NaCl example a developer looks at this comment will be
> completely useless. How about adding a cross-reference or more explanation?

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:34: class dlOpenInstance :
public pp::Instance {
On 2012/01/28 01:00:52, Brad Chen wrote:
> It sure makes me sad we can't do a simple example without all of this C++
> complexity.

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:37: eightball=(NULL);
On 2012/01/28 01:00:52, Brad Chen wrote:
> " = "

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/dlopen.cc:58: if(dlhandle == NULL){
On 2012/01/28 01:00:52, Brad Chen wrote:
> "if (dlhandle == NULL) {"
> I really think we need to make this example more like Google3 throughout. Let
me
> know if you want me to note other problems with spaces and formatting; there
are
> more that follow.

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/eightball.cc (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.cc:6:
__attribute__((visibility("default")))const char *Magic8Ball() {
On 2012/01/28 01:00:52, Brad Chen wrote:
> If you need to keep the __attribute__ stuff, maybe you can put the "const char
> ..." on a newline so people can more easily parse the GNU extensions vs. the
> standard C.

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.cc:21: }
On 2012/01/28 01:00:52, Brad Chen wrote:
> Not worth using the {} for extern "C" if there is only one declaration.

Done.

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
File native_client_sdk/src/examples/dlopen/eightball.h (right):

https://chromiumcodereview.appspot.com/9234043/diff/11003/native_client_sdk/s...
native_client_sdk/src/examples/dlopen/eightball.h:6:
__attribute__((visibility("default")))const char* Magic8Ball();
On 2012/01/28 01:00:52, Brad Chen wrote:
> It's kind of nasty seeing this __attribute__ stuff all over the place. Could
you
> please check with David Sehr or Roland if you haven't already? They would know
> if there was something simpler we could do instead.

Done.

[Brad Chen, 2012-01-31 22:29:54]

LGTM.
Please deal with the nits below.
This is much better than the last version. Thanks!

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
File native_client_sdk/src/examples/dlopen/dlopen.cc (right):

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
native_client_sdk/src/examples/dlopen/dlopen.cc:80: if(this->_dlhandle == NULL){
Google3 style prescribes a space after "if". Miss this one?

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
native_client_sdk/src/examples/dlopen/dlopen.cc:83: else {
I don't particularly care for this formatting and I don't think it is google3
either. You should use "} else {\n", that is, no newline before the "else".
There are a few more of these below.

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
File native_client_sdk/src/examples/dlopen/eightball.cc (right):

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
native_client_sdk/src/examples/dlopen/eightball.cc:6: const char
*Magic8Ball(void) {
I'm so happy you dropped the __attribute__ stuff. That said, please be sure we
aren't hitting the problem the old code was trying to fix. In particular check
all flavors of Windows.

[noelallen1, 2012-02-01 17:46:10]

Thanks.

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
File native_client_sdk/src/examples/dlopen/dlopen.cc (right):

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
native_client_sdk/src/examples/dlopen/dlopen.cc:80: if(this->_dlhandle == NULL){
On 2012/01/31 22:29:54, Brad Chen wrote:
> Google3 style prescribes a space after "if". Miss this one?

Done.

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
native_client_sdk/src/examples/dlopen/dlopen.cc:83: else {
On 2012/01/31 22:29:54, Brad Chen wrote:
> I don't particularly care for this formatting and I don't think it is google3
> either. You should use "} else {\n", that is, no newline before the "else".
> There are a few more of these below.

Done.

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
File native_client_sdk/src/examples/dlopen/eightball.cc (right):

http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examp...
native_client_sdk/src/examples/dlopen/eightball.cc:6: const char
*Magic8Ball(void) {
Windows bots are passing, but we don't have all OS flavors represented.  I'll
keep an eye out.  However, I don't see how that's possible.  I can understand if
newlib vs glibc fails since they are toolchains, but I would expect our GCC
toolchain and sel_ldr/chrome built for Windows on machine N which is unknown, to
work the same for any Windows platform X that down loads it.  Windows is not
involved in either the link or load process.

On 2012/01/31 22:29:54, Brad Chen wrote:
> I'm so happy you dropped the __attribute__ stuff. That said, please be sure we
> aren't hitting the problem the old code was trying to fix. In particular check
> all flavors of Windows.

[Brad Chen, 2012-02-01 18:04:53]

Maybe ask Colt to check test this on the machines he works with.

Brad

On Wed, Feb 1, 2012 at 9:46 AM, <noelallen@chromium.org> wrote:

> Thanks.
>
>
>
> http://codereview.chromium.**org/9234043/diff/21001/native_**
>
client_sdk/src/examples/**dlopen/dlopen.cc<http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examples/dlopen/dlopen.cc>
> File native_client_sdk/src/**examples/dlopen/dlopen.cc (right):
>
> http://codereview.chromium.**org/9234043/diff/21001/native_**
>
client_sdk/src/examples/**dlopen/dlopen.cc#newcode80<http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examples/dlopen/dlopen.cc#newcode80>
> native_client_sdk/src/**examples/dlopen/dlopen.cc:80: if(this->_dlhandle
> == NULL){
> On 2012/01/31 22:29:54, Brad Chen wrote:
>
>> Google3 style prescribes a space after "if". Miss this one?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/9234043/diff/21001/native_**
>
client_sdk/src/examples/**dlopen/dlopen.cc#newcode83<http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examples/dlopen/dlopen.cc#newcode83>
> native_client_sdk/src/**examples/dlopen/dlopen.cc:83: else {
> On 2012/01/31 22:29:54, Brad Chen wrote:
>
>> I don't particularly care for this formatting and I don't think it is
>>
> google3
>
>> either. You should use "} else {\n", that is, no newline before the
>>
> "else".
>
>> There are a few more of these below.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/9234043/diff/21001/native_**
>
client_sdk/src/examples/**dlopen/eightball.cc<http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examples/dlopen/eightball.cc>
> File native_client_sdk/src/**examples/dlopen/eightball.cc (right):
>
> http://codereview.chromium.**org/9234043/diff/21001/native_**
>
client_sdk/src/examples/**dlopen/eightball.cc#newcode6<http://codereview.chromium.org/9234043/diff/21001/native_client_sdk/src/examples/dlopen/eightball.cc#newcode6>
> native_client_sdk/src/**examples/dlopen/eightball.cc:**6: const char
> *Magic8Ball(void) {
> Windows bots are passing, but we don't have all OS flavors represented.
> I'll keep an eye out.  However, I don't see how that's possible.  I can
> understand if newlib vs glibc fails since they are toolchains, but I
> would expect our GCC toolchain and sel_ldr/chrome built for Windows on
> machine N which is unknown, to work the same for any Windows platform X
> that down loads it.  Windows is not involved in either the link or load
> process.
>
>
> On 2012/01/31 22:29:54, Brad Chen wrote:
>
>> I'm so happy you dropped the __attribute__ stuff. That said, please be
>>
> sure we
>
>> aren't hitting the problem the old code was trying to fix. In
>>
> particular check
>
>> all flavors of Windows.
>>
>
>
http://codereview.chromium.**org/9234043/<http://codereview.chromium.org/9234...
>