ppapi_simple

Clean up for ppapi main.

Rename ppapi_main to ppapi_simple.
Enable spinning version running on main thread.
Update demos to use ppapi_simple
Remove redundant pappi_main_hello_world and ppapi_main_3d
Simplify Events
Add MT safe pointer Q.

R=nfullagar@chromium.org

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

[noelallen1, 2013-05-22 22:35:45]

ptal

[Sam Clegg, 2013-05-22 22:39:50]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/build_projects.py (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/build_projects.py:144: extra_args =
['CONFIG='+config, 'NACL_SDK_ROOT='+pepperdir]
This script already (via a recent change) unsets
NACL_SDK_ROOT which will cause all the examples to build
with the NACL_SDK_ROOT which contains them.

While this fixes the same issue that the "unset" below does, 
it ends up testing a different path (do the Makefiles work 
with NACL_SDK_ROOT set, vs do they work without it set).

[binji, 2013-05-23 18:06:49]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/sdk_files.list (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_files.list:458:
include/ppapi_simple/ppapi_simple_main.h
2d?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/example.dsc (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/example.dsc:2: 'TOOLS': ['newlib',
'glibc', 'pnacl'],
win, linux?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/example.js (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/example.js:8: function
handleMessage(message_event) {
remove

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/index.html (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/index.html:23: <input type="text"
size="15" id="pi">
remove

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:1: // Copyright 2011 The Native
Client SDK Authors. All rights reserved.
2013

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:31: struct {
why store this is a struct instead of individual globals?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:32: PP_Resource ctx_;
public members for structs don't have trailing _

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:45: // Map of neighboring
colors.
these numbers seem pretty random. Maybe explain a bit more?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:95: static int mouse_down = 0;
unused

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:146: if (x > 0 && x < width && y
> 0 && y < height) {
x < width - 1
y < height - 1

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:157: if
(g_pKeyboardInput->GetKeyCode(event->resource_) == 13) {
enter key

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:158: // PSToggleFullscreen();
does this not work?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:181: g_Context.cell_in_[i] =
rand() & 1;
probably not a big deal, but I always heard that the low bits of rand() were
less random than the high bits...

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:194:
g_pImageData->Create(PSGetInstanceId(), format, psize, PP_FALSE);
seems strange to create a new image every frame. Maybe explain that this is
cached...?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:210: // Do neighbor sumation;
apply rules, output pixel color.
sp: summation

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:211: for (y = 1; y <
(psize->height - 1); ++y) {
why ()?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:217: uint8_t *dst =
(g_Context.cell_out_ + (y) * psize->width) + 1;
why (y)?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:251: int my_main(int argc, const
char *argv[]) {
might want to mention this can be called anything but main.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:265: g_pTouchInput =
unused

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:272: while ((event =
PSAcquireEvent(0)) != NULL) {
what is 0?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:285: PPAPI_SIMPLE_MAIN(my_main);
what is this doing? Maybe a little comment.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:23: * As Pepper
events such as input, or focus and view changes arive on the,
sp: arrive

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:23: * As Pepper
events such as input, or focus and view changes arive on the,
remove , after the

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:29: * are placed on
that particular thread's Q.
s/Q/queue/

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:44: PS_LOST_EVENT =
16,    // When the 3D context is lost, no resource.
PS_LOST_EVENT is a bit generic, maybe PS_3D_LOST_EVENT

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:45: PS_LOCK_EVENT =
32,    // When the mouse lock is lost.
same, maybe PS_MOUSELOCK_LOST_EVENT?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:52: PSEventType
type_;
event type is a bitmask, but are we ensured that type_ only has 1 bit set?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:54: PP_Bool bool_;
which union members are matched to which event types?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:22: *
ppapi_simple_main.h, ppapi_simple_main_2d.h, and ppapi_simple_main_3d.h
3d files aren't in this CL. not uploaded?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:36: * by most of the
Pepper resouce creation routines.
sp: resource

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:54: * the instance
object.  This function is defined by one of the macros bellow,
sp: below

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:106:
ppb_core_ = reinterpret_cast<const PPB_Core*>(
why reinterpret_cast? static_cast should work.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:141: if (0
== strncmp(argn[i], "pm_", 3)) {
for legacy support? or should it be ps_ now

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h:70:
PSEvent* AcquireEvent(bool block);
use an enum for this, it's clearer at the call site

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main.h:19: * main and
view callbacks passed in via the PSSimpleMainInit_t object.
where is PSSimpleMainInit_t?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.cc
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.cc:23:
g_Context.ctx_ = 0;
what is g_Context?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.cc:31: updated
= true;
PP_TRUE

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.cc:34: if
(width != g_Context.size_.width || height != g_Context.size_.height ||
why not set updated in this block? isn't it updated if the context was
previously unbound?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.cc:49:
pInst->EnableRender(enable != 0);
not implemented anywhere

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.h:29: * true
when the context has changed.
probably needs an example of how to use

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.h:54: *
PPAPI_SIMPLE_MAIN
MAIN2D

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.h:56: *
Constructs an instance using PSCreateMain, and configures the 'main' and
this comment is a dupe of the PP_SIMPLE_MAIN, probably should be a bit
different...

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main2d.h:72: static
PSMain2DCfg_t s_cfg = { main, view, rend };      \
PSMain2DCfg_t doesn't seem to be defined

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/utils/pointer_queue.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/utils/pointer_queue.h:22: template<class T>
class PointerQueue {
not crazy about the name PointerQueue. Maybe something to do with
multithreadedness or thread-safety? ThreadSafeQueue?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/utils/pointer_queue.h:34: void Add(T* item) {
should probably use standard queue terminology:
PushBack to match stdc++, or Enqueue to match most queue libraries.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/utils/pointer_queue.h:41: T* Get(bool block) {
same here: PopFront or Dequeue

[nfullagar1, 2013-05-23 21:58:36]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:126: case PS_INPUT_EVENT: {
I'm not super keen on how PS is defining a PS_INPUT_EVENT, but then further
below using PP_INPUTEVENT_TYPE_MOUSEMOVE.  For people new to PPAPI and PS,
there's a fairly subtle line here between the two.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:205:
g_pImageData->Describe(image, &desc);
For consistency, use the desc.size.width & height for the rest of this function,
instead of mixing psize->width, psize->height with desc.stride.  It is a bit
clearer, as the |desc| is from imagedata -- same as |pixels| (whereas psize came
from context.)

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:284: 
How would this sample differ if PPAPI_SIMPLE_MAIN2D was used? (below)

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:16:
int my_main(int argc, const char* argv[]) {
example_main?  main_entry?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:35:
PPAPI_SIMPLE_MAIN(my_main)
What does this do?  What is the failure mode if it is omitted?

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:22: * The PSEvent
system provides a lock-less in-order event processing mechanism.
remove lock-less

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:19: * For C style
development, the PPAPI_SIMPLE_MAIN(XX) macros provide a
maybe PPAPI_SIMPLE_REGISTER_MAIN(XX) is a bit more descriptive (for when it is
used in the demos away from this comment block)

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:64: *
PPAPI_DECLARE_PARAMS
These macros PPAPI_DECLARE_PARAMS, PPAPI_USE_MAIN, PPAPI_USE_CLASS should be
prefixed with PPAPI_SIMPLE_

[noelallen1, 2013-05-23 22:01:24]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/build_projects.py (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/build_projects.py:144: extra_args =
['CONFIG='+config, 'NACL_SDK_ROOT='+pepperdir]
On 2013/05/22 22:39:50, Sam Clegg wrote:
> This script already (via a recent change) unsets
> NACL_SDK_ROOT which will cause all the examples to build
> with the NACL_SDK_ROOT which contains them.
> 
> While this fixes the same issue that the "unset" below does, 
> it ends up testing a different path (do the Makefiles work 
> with NACL_SDK_ROOT set, vs do they work without it set).

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/example.dsc (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/example.dsc:2: 'TOOLS': ['newlib',
'glibc', 'pnacl'],
On 2013/05/23 18:06:49, binji wrote:
> win, linux?

I will enable on windows after testing.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/example.js (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/example.js:8: function
handleMessage(message_event) {
On 2013/05/23 18:06:49, binji wrote:
> remove

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/index.html (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/index.html:23: <input type="text"
size="15" id="pi">
On 2013/05/23 18:06:49, binji wrote:
> remove

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:31: struct {
On 2013/05/23 18:06:49, binji wrote:
> why store this is a struct instead of individual globals?

Just for organization reasons.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:32: PP_Resource ctx_;
On 2013/05/23 18:06:49, binji wrote:
> public members for structs don't have trailing _

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:45: // Map of neighboring
colors.
On 2013/05/23 18:06:49, binji wrote:
> these numbers seem pretty random. Maybe explain a bit more?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:95: static int mouse_down = 0;
On 2013/05/23 18:06:49, binji wrote:
> unused

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:146: if (x > 0 && x < width && y
> 0 && y < height) {
On 2013/05/23 18:06:49, binji wrote:
> x < width - 1
> y < height - 1

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:158: // PSToggleFullscreen();
On 2013/05/23 18:06:49, binji wrote:
> does this not work?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:194:
g_pImageData->Create(PSGetInstanceId(), format, psize, PP_FALSE);
On 2013/05/23 18:06:49, binji wrote:
> seems strange to create a new image every frame. Maybe explain that this is
> cached...?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:210: // Do neighbor sumation;
apply rules, output pixel color.
On 2013/05/23 18:06:49, binji wrote:
> sp: summation

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:211: for (y = 1; y <
(psize->height - 1); ++y) {
On 2013/05/23 18:06:49, binji wrote:
> why ()?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:217: uint8_t *dst =
(g_Context.cell_out_ + (y) * psize->width) + 1;
On 2013/05/23 18:06:49, binji wrote:
> why (y)?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:251: int my_main(int argc, const
char *argv[]) {
On 2013/05/23 18:06:49, binji wrote:
> might want to mention this can be called anything but main.

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:272: while ((event =
PSAcquireEvent(0)) != NULL) {
On 2013/05/23 18:06:49, binji wrote:
> what is 0?

Means don't block, see comment above.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:285: PPAPI_SIMPLE_MAIN(my_main);
On 2013/05/23 18:06:49, binji wrote:
> what is this doing? Maybe a little comment.

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:23: * As Pepper
events such as input, or focus and view changes arive on the,
On 2013/05/23 18:06:49, binji wrote:
> sp: arrive

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:23: * As Pepper
events such as input, or focus and view changes arive on the,
On 2013/05/23 18:06:49, binji wrote:
> remove , after the

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:29: * are placed on
that particular thread's Q.
On 2013/05/23 18:06:49, binji wrote:
> s/Q/queue/

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:44: PS_LOST_EVENT =
16,    // When the 3D context is lost, no resource.
On 2013/05/23 18:06:49, binji wrote:
> PS_LOST_EVENT is a bit generic, maybe PS_3D_LOST_EVENT

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:45: PS_LOCK_EVENT =
32,    // When the mouse lock is lost.
On 2013/05/23 18:06:49, binji wrote:
> same, maybe PS_MOUSELOCK_LOST_EVENT?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:52: PSEventType
type_;
On 2013/05/23 18:06:49, binji wrote:
> event type is a bitmask, but are we ensured that type_ only has 1 bit set?

The PostEvent functions verify that type_ is an expected value for the data
associated with it.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:54: PP_Bool bool_;
On 2013/05/23 18:06:49, binji wrote:
> which union members are matched to which event types?

In the comments above by the event type enum.  Seemed redundant to have it here
as well.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:22: *
ppapi_simple_main.h, ppapi_simple_main_2d.h, and ppapi_simple_main_3d.h
On 2013/05/23 18:06:49, binji wrote:
> 3d files aren't in this CL. not uploaded?

Adding 2D/3D will be a separate CL that will also contain demos using those
functions.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:36: * by most of the
Pepper resouce creation routines.
On 2013/05/23 18:06:49, binji wrote:
> sp: resource

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:54: * the instance
object.  This function is defined by one of the macros bellow,
On 2013/05/23 18:06:49, binji wrote:
> sp: below

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:106:
ppb_core_ = reinterpret_cast<const PPB_Core*>(
On 2013/05/23 18:06:49, binji wrote:
> why reinterpret_cast? static_cast should work.

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:141: if (0
== strncmp(argn[i], "pm_", 3)) {
On 2013/05/23 18:06:49, binji wrote:
> for legacy support? or should it be ps_ now

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/05/23 18:06:49, binji wrote:
> 2013

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h:70:
PSEvent* AcquireEvent(bool block);
On 2013/05/23 18:06:49, binji wrote:
> use an enum for this, it's clearer at the call site

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_main.h:19: * main and
view callbacks passed in via the PSSimpleMainInit_t object.
On 2013/05/23 18:06:49, binji wrote:
> where is PSSimpleMainInit_t?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/utils/pointer_queue.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/utils/pointer_queue.h:22: template<class T>
class PointerQueue {
On 2013/05/23 18:06:49, binji wrote:
> not crazy about the name PointerQueue. Maybe something to do with
> multithreadedness or thread-safety? ThreadSafeQueue?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/utils/pointer_queue.h:34: void Add(T* item) {
On 2013/05/23 18:06:49, binji wrote:
> should probably use standard queue terminology:
> PushBack to match stdc++, or Enqueue to match most queue libraries.

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/utils/pointer_queue.h:41: T* Get(bool block) {
On 2013/05/23 18:06:49, binji wrote:
> same here: PopFront or Dequeue

Done.

[noelallen1, 2013-05-24 18:08:31]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:205:
g_pImageData->Describe(image, &desc);
On 2013/05/23 21:58:36, nfullagar1 wrote:
> For consistency, use the desc.size.width & height for the rest of this
function,
> instead of mixing psize->width, psize->height with desc.stride.  It is a bit
> clearer, as the |desc| is from imagedata -- same as |pixels| (whereas psize
came
> from context.)

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:284: 
On 2013/05/23 21:58:36, nfullagar1 wrote:
> How would this sample differ if PPAPI_SIMPLE_MAIN2D was used? (below)

The idea is that PPAPI_SIMPLE_MAIN2D would automate more of this.  It would
provide helpers for building and maintaining the 2D context, and for allocating
and freeing the image data.  It should be possible to re-write this without
g_pView, g_pImageData, and g_pGraphics2D.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:16:
int my_main(int argc, const char* argv[]) {
On 2013/05/23 21:58:36, nfullagar1 wrote:
> example_main?  main_entry?

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:35:
PPAPI_SIMPLE_MAIN(my_main)
On 2013/05/23 21:58:36, nfullagar1 wrote:
> What does this do?  What is the failure mode if it is omitted?

There are more details in the header, but this should fail to link since
ppapi_simple.cc won't be referenced and then pp::Module will not be defined.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:22: * The PSEvent
system provides a lock-less in-order event processing mechanism.
On 2013/05/23 21:58:36, nfullagar1 wrote:
> remove lock-less

Done.

[nfullagar1, 2013-05-28 18:28:53]

a few more

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:116:
UpdateContext(rect.size.width, rect.size.height);
small nit - UpdateContext() sounds like it should update all aspects of the
context, but it just updates the non-ppapi parts (cell_in, cell_out buffers) 
Looking closer, updating of the ppapi graphics context actually occurs in code
below, g_Context.ctx = ...etc.  Why not have UpdateContext also do this?

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:129: case PS_INPUT_EVENT: {
small nit on PS_INPUT_EVENT: PPAPI doesn't have an _ between INPUT and EVENT on
PP_INPUTEVENT_TYPE_...

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:294: // see:
pappi_simple/ppapi_simple.h
small nit for comment above: ppapi_simple.h has PPAPI_SIMPLE_USE_MAIN but it is
ppapi_simple_main.h that defines PPAPI_SIMPLE_REGISTER_MAIN

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:34:

need comment here to direct reader to appropriate header that describes
PPAPI_SIMPLE_REGISTER_MAIN() in further detail

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:30: * NOTE: The link
will still need to link in C++ since the library is still
...link with library std++

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:20: // simple by
avoiding issues of template traits.
Comment above mentions object pointers, avoiding issues of template traits. 
Code below uses templates.  Suggest re-wording the comment.

[Sam Clegg, 2013-05-29 04:48:25]

Mostly just nits...

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.cc (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.cc:34: PSEvent*
PSWaitForAcquireEvent() {
How about naming these just 
   PSEventPoll()
and
   PSEventWait()

I've always found this naming convention used by SDL to be
very obvious:

http://www.libsdl.org/intro.en/usingevents.html

Also, what do you think about starting all functions in this file with the
common PSEvent prefix (I like C modules where everything starts with a common
prefix, but I'm not sure the
style guide says anything about this).

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:41: PS_VIEW_EVENT   
= 2,   // From DidChangeView, contains a view resource
We don't align the assignment operators normally do we?

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc
(right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:57: int
ret = si->inst_->MainThread(si->argc_, si->argv_);
Should we try to propagate main()'s return value back to the NaCl runtime?

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:140: // If
we start with PM prefix set the instance argument map
PS prefix?

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:234:
vfprintf(stdout, fmt, ap);
Normally all logging goes to stderr, and stdout would be reserved for the actual
output of the program.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:242:
vfprintf(stdout, fmt, ap);
Ditto.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:250:
vfprintf(stderr, fmt, ap);
Maybe factor these 3 logging function into a single function?

I think it would be really useful to have a general logging framework in the
SDK.. maybe this could be it.  Certainly I would find it really useful to enable
logging in nacl_io without recompiling.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h
(right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h:59: void
Log(const char *fmt, ...);
IIRC you can mark these functions with GCC attributes which will allow the
compiler to warn you about bad augments: __attribute__ ((format (printf, 2,
3)));

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:46:
while(list_.empty()) pthread_cond_wait(&cond_, &mutex_);
Space after while

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:49: if (list_.empty())
return NULL;
Perhaps put in an else block?

[noelallen_use_chromium, 2013-05-30 00:25:06]

PTAL

Renamed files to ps_XXX
Renamed functions to PS<FILE><VERB>
Renamed enums to PS<?>_<NAME>
Fixed nits.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:116:
UpdateContext(rect.size.width, rect.size.height);
On 2013/05/28 18:28:53, nfullagar1 wrote:
> small nit - UpdateContext() sounds like it should update all aspects of the
> context, but it just updates the non-ppapi parts (cell_in, cell_out buffers) 
> Looking closer, updating of the ppapi graphics context actually occurs in code
> below, g_Context.ctx = ...etc.  Why not have UpdateContext also do this?

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:129: case PS_INPUT_EVENT: {
On 2013/05/28 18:28:53, nfullagar1 wrote:
> small nit on PS_INPUT_EVENT: PPAPI doesn't have an _ between INPUT and EVENT
on
> PP_INPUTEVENT_TYPE_... 

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:294: // see:
pappi_simple/ppapi_simple.h
On 2013/05/28 18:28:53, nfullagar1 wrote:
> small nit for comment above: ppapi_simple.h has PPAPI_SIMPLE_USE_MAIN but it
is
> ppapi_simple_main.h that defines PPAPI_SIMPLE_REGISTER_MAIN

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:34:

On 2013/05/28 18:28:53, nfullagar1 wrote:
> need comment here to direct reader to appropriate header that describes
> PPAPI_SIMPLE_REGISTER_MAIN() in further detail

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.cc (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.cc:34: PSEvent*
PSWaitForAcquireEvent() {
On 2013/05/29 04:48:25, Sam Clegg wrote:
> How about naming these just 
>    PSEventPoll()
> and
>    PSEventWait()
> 
> I've always found this naming convention used by SDL to be
> very obvious:
> 
> http://www.libsdl.org/intro.en/usingevents.html
> 
> Also, what do you think about starting all functions in this file with the
> common PSEvent prefix (I like C modules where everything starts with a common
> prefix, but I'm not sure the
> style guide says anything about this).

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_event.h:41: PS_VIEW_EVENT   
= 2,   // From DidChangeView, contains a view resource
On 2013/05/29 04:48:25, Sam Clegg wrote:
> We don't align the assignment operators normally do we?

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple.h:30: * NOTE: The link
will still need to link in C++ since the library is still
On 2013/05/28 18:28:53, nfullagar1 wrote:
> ...link with library std++

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc
(right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.cc:57: int
ret = si->inst_->MainThread(si->argc_, si->argv_);
On 2013/05/29 04:48:25, Sam Clegg wrote:
> Should we try to propagate main()'s return value back to the NaCl runtime?

Until it's legal for a NEXE to exit...   The main pepper thread is still running
and will continue to run.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h
(right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ppapi_simple_instance.h:59: void
Log(const char *fmt, ...);
On 2013/05/29 04:48:25, Sam Clegg wrote:
> IIRC you can mark these functions with GCC attributes which will allow the
> compiler to warn you about bad augments: __attribute__ ((format (printf, 2,
> 3)));
> 
> http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

Unfortunately __attribute__ is not available on windows.  So we typically avoid
things that will fail to compile for host.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:20: // simple by
avoiding issues of template traits.
On 2013/05/28 18:28:53, nfullagar1 wrote:
> Comment above mentions object pointers, avoiding issues of template traits. 
> Code below uses templates.  Suggest re-wording the comment.

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:46:
while(list_.empty()) pthread_cond_wait(&cond_, &mutex_);
On 2013/05/29 04:48:25, Sam Clegg wrote:
> Space after while

Done.

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:49: if (list_.empty())
return NULL;
Early out is the convention.

On 2013/05/29 04:48:25, Sam Clegg wrote:
> Perhaps put in an else block?

[nfullagar1, 2013-05-30 00:47:03]

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:157: PSE_
remove 4 lines above?

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:15:
=
there's a stray '=' on line above

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_event.h (right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:45: 
what about focus change PSE_INSTANCE_DIDCHANGEFOCUS ?

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:197: bool
PSInstance::ProcessProperties() {
lots of magic going on in this function, a longer overview comment explaining
why this is needed & how it ties into nacl_io, printf, etc. might be helpful.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:322: case
PS_MESSAGE_EVENT:
PSE_INSTANCE_HANDLEMESSAGE  ?

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:357:
PostEvent(PS_FOCUS_EVENT, focus ? PP_TRUE : PP_FALSE);
PSE_INSTANCE_DIDCHANGEFOCUS ?

[Sam Clegg, 2013-05-30 01:16:13]

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:49: if (list_.empty())
return NULL;
On 2013/05/30 00:25:07, noelallen wrote:
> Early out is the convention.
> 
> On 2013/05/29 04:48:25, Sam Clegg wrote:
> > Perhaps put in an else block?
> 

But isn't this check redundant if you went through the "if" above?  If 'block'
is set to true then the list is guaranteed to be non-empty on this line, right? 
 No need to check again here.

Another way to look at it is that if block is true we should never be able to
return NULL.

[noelallen1, 2013-05-30 03:10:05]

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/28001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:50: 
Since there's no real performance benefit to the else, I thought this was more
readable.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:157: PSE_
On 2013/05/30 00:47:04, nfullagar1 wrote:
> remove 4 lines above?

Done.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:15:
=
On 2013/05/30 00:47:04, nfullagar1 wrote:
> there's a stray '=' on line above

Done.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_event.h (right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:45: 
On 2013/05/30 00:47:04, nfullagar1 wrote:
> what about focus change PSE_INSTANCE_DIDCHANGEFOCUS ?

Done.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:197: bool
PSInstance::ProcessProperties() {
On 2013/05/30 00:47:04, nfullagar1 wrote:
> lots of magic going on in this function, a longer overview comment explaining
> why this is needed & how it ties into nacl_io, printf, etc. might be helpful.

Done.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:322: case
PS_MESSAGE_EVENT:
On 2013/05/30 00:47:04, nfullagar1 wrote:
> PSE_INSTANCE_HANDLEMESSAGE  ?

Done.

https://codereview.chromium.org/15011003/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:357:
PostEvent(PS_FOCUS_EVENT, focus ? PP_TRUE : PP_FALSE);
On 2013/05/30 00:47:04, nfullagar1 wrote:
> PSE_INSTANCE_DIDCHANGEFOCUS ?

Done.

[binji, 2013-05-30 05:07:50]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:146: if (x > 0 && x < width && y
> 0 && y < height) {
On 2013/05/23 22:01:24, noelallen1 wrote:
> On 2013/05/23 18:06:49, binji wrote:
> > x < width - 1
> > y < height - 1
> 
> Done.

Missed this one.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:282: // Regsiter the function to
call once the Instance Object is initialized.
sp: Register

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:283: // see:
pappi_simple/ppapi_simple_main.h
ps_main.h

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:26:
// Regsiter the function to call once the Instance Object is initialized.
sp: Register

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:27:
// see: pappi_simple/ppapi_simple_main.h
ps_main.h

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:19: * In order to provide a
standard blocking environment, the library will hide
nit: indent 1 space

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:20: * the actual "Pepper
Thread" which is the thread that standartd events
sp: standard

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:52: * Return the Pepper
instance refered to by 'name'.  Will return a pointer
sp: referred

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:63: * or by the equivilent
macro in one of the other headers.  For 'C'
sp: equivalent

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:66: * the two macros bellow.
sp: below

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:103: #define
PPAPI_SIMPLE_USE_CLASS(class, ...)                         \
might not want to use a keyword as macro param

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_event.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:79: PSEvent*
PSWaitForAcquireEvent();
remove

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:81: void
PSEventSetFilter(uint32_t);
might be nicer with PSEventTypeMask
and above:
typedef uint32_t PSEventTypeMask;

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:84: * PSPostEvent
PSEventPost
though I prefer omitting this info, it's redundant

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:180:
static_cast<void*>(si));
nit: static_cast shouldn't be necessary.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:286: env->type =
type;
memset here and below for consistency?

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:298:
ppb_core_->AddRefResource(resource);
nit: indent 1 more space

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:347: Log("Got
Message: \n");
print message here? Otherwise remove ": " because it implies that a message
follows.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:55: virtual int
MainThread(int argc, const char* argv[]);
private/protected? Nothing should call this except MainThreadThunk

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:73: //
remove //

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:76: // Callback
functions triggered by Pepepr
sp: Pepper

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:112: uint32_t
verbosity_;
Why not Verbosity verbosity_

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_main.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_main.h:5: #ifndef
PPAPI_SIMPLE_PPAPI_SIMPLE_MAIN_H_
PS_MAIN_H

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_main.h:16: * PSCreateMain
PSMainCreate

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:16: // PointerQueue
ThreadSafeQueue

[nfullagar1, 2013-05-30 18:04:52]

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:73: // values.  The health is
binary: either alive or dead.
There's a mix of // and /* */ style comments in this c file.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:128: {
This code (to scribble dots into the current cell array) used to be in a
separate function - why inline?  There's no comment on what this does.  It might
also need to be triggered on PP_INPUTEVENT_TYPE_MOUSEDOWN - consider the case
where the user clicks on the pointer without moving it, attempting to add a
single point.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:149: case
PP_INPUTEVENT_TYPE_KEYUP: {
Why trigger on keyup?  Wouldn't keydown be more responsive?

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:17:
// Use PostMessage to send "Hello World" to JavaScript.
/* */ style comments for c files?

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps.cc (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.cc:25: /// The Module class. 
The browser calls the CreateInstance() method to create
/// ?

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:50: // main pepper
thread which must never be blocked.
small nit: there is some slight inconsistency in the comments for functions /
classes.

Some (such as here) are:
// comment comment comment
// comment comment...

Others are:
//
// comment comment comment
//

And others are:
// function name
//
// comment comment comment...

And sometimes:
//
// function name
//
// comment comment comment...
//

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:201: //
initialization macro, or via dynamicallys set embed attributes
dynamically

[noelallen1, 2013-05-30 18:07:18]

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:1: // Copyright 2011 The Native
Client SDK Authors. All rights reserved.
On 2013/05/23 18:06:49, binji wrote:
> 2013

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:126: case PS_INPUT_EVENT: {
On 2013/05/23 21:58:36, nfullagar1 wrote:
> I'm not super keen on how PS is defining a PS_INPUT_EVENT, but then further
> below using PP_INPUTEVENT_TYPE_MOUSEMOVE.  For people new to PPAPI and PS,
> there's a fairly subtle line here between the two.

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:146: if (x > 0 && x < width && y
> 0 && y < height) {
On 2013/05/30 05:07:50, binji wrote:
> On 2013/05/23 22:01:24, noelallen1 wrote:
> > On 2013/05/23 18:06:49, binji wrote:
> > > x < width - 1
> > > y < height - 1
> > 
> > Done.
> 
> Missed this one.

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:157: if
(g_pKeyboardInput->GetKeyCode(event->resource_) == 13) {
On 2013/05/23 18:06:49, binji wrote:
> enter key

Done.

https://codereview.chromium.org/15011003/diff/4001/native_client_sdk/src/exam...
native_client_sdk/src/examples/demo/life/life.c:265: g_pTouchInput =
On 2013/05/23 18:06:49, binji wrote:
> unused

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:282: // Regsiter the function to
call once the Instance Object is initialized.
On 2013/05/30 05:07:50, binji wrote:
> sp: Register

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:283: // see:
pappi_simple/ppapi_simple_main.h
On 2013/05/30 05:07:50, binji wrote:
> ps_main.h

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:26:
// Regsiter the function to call once the Instance Object is initialized.
On 2013/05/30 05:07:50, binji wrote:
> sp: Register

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:27:
// see: pappi_simple/ppapi_simple_main.h
On 2013/05/30 05:07:50, binji wrote:
> ps_main.h

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:19: * In order to provide a
standard blocking environment, the library will hide
On 2013/05/30 05:07:50, binji wrote:
> nit: indent 1 space

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:20: * the actual "Pepper
Thread" which is the thread that standartd events
On 2013/05/30 05:07:50, binji wrote:
> sp: standard

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:52: * Return the Pepper
instance refered to by 'name'.  Will return a pointer
On 2013/05/30 05:07:50, binji wrote:
> sp: referred

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:63: * or by the equivilent
macro in one of the other headers.  For 'C'
On 2013/05/30 05:07:50, binji wrote:
> sp: equivalent

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:66: * the two macros bellow.
On 2013/05/30 05:07:50, binji wrote:
> sp: below

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.h:103: #define
PPAPI_SIMPLE_USE_CLASS(class, ...)                         \
On 2013/05/30 05:07:50, binji wrote:
> might not want to use a keyword as macro param

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_event.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:79: PSEvent*
PSWaitForAcquireEvent();
On 2013/05/30 05:07:50, binji wrote:
> remove

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:81: void
PSEventSetFilter(uint32_t);
On 2013/05/30 05:07:50, binji wrote:
> might be nicer with PSEventTypeMask
> and above:
> typedef uint32_t PSEventTypeMask;

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_event.h:84: * PSPostEvent
On 2013/05/30 05:07:50, binji wrote:
> PSEventPost
> though I prefer omitting this info, it's redundant

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:180:
static_cast<void*>(si));
On 2013/05/30 05:07:50, binji wrote:
> nit: static_cast shouldn't be necessary.

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:286: env->type =
type;
On 2013/05/30 05:07:50, binji wrote:
> memset here and below for consistency?

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:298:
ppb_core_->AddRefResource(resource);
On 2013/05/30 05:07:50, binji wrote:
> nit: indent 1 more space

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:347: Log("Got
Message: \n");
On 2013/05/30 05:07:50, binji wrote:
> print message here? Otherwise remove ": " because it implies that a message
> follows.

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:55: virtual int
MainThread(int argc, const char* argv[]);
On 2013/05/30 05:07:50, binji wrote:
> private/protected? Nothing should call this except MainThreadThunk

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:73: //
On 2013/05/30 05:07:50, binji wrote:
> remove //

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:76: // Callback
functions triggered by Pepepr
On 2013/05/30 05:07:50, binji wrote:
> sp: Pepper

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.h:112: uint32_t
verbosity_;
On 2013/05/30 05:07:50, binji wrote:
> Why not Verbosity verbosity_

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_main.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_main.h:5: #ifndef
PPAPI_SIMPLE_PPAPI_SIMPLE_MAIN_H_
On 2013/05/30 05:07:50, binji wrote:
> PS_MAIN_H

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_main.h:16: * PSCreateMain
On 2013/05/30 05:07:50, binji wrote:
> PSMainCreate

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/utils/thread_safe_queue.h (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/utils/thread_safe_queue.h:16: // PointerQueue
On 2013/05/30 05:07:50, binji wrote:
> ThreadSafeQueue

Done.

[noelallen1, 2013-05-30 21:01:43]

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:73: // values.  The health is
binary: either alive or dead.
On 2013/05/30 18:04:52, nfullagar1 wrote:
> There's a mix of // and /* */ style comments in this c file.

Thanks, C style in C files.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:128: {
On 2013/05/30 18:04:52, nfullagar1 wrote:
> This code (to scribble dots into the current cell array) used to be in a
> separate function - why inline?  There's no comment on what this does.  It
might
> also need to be triggered on PP_INPUTEVENT_TYPE_MOUSEDOWN - consider the case
> where the user clicks on the pointer without moving it, attempting to add a
> single point.  

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:149: case
PP_INPUTEVENT_TYPE_KEYUP: {
On 2013/05/30 18:04:52, nfullagar1 wrote:
> Why trigger on keyup?  Wouldn't keydown be more responsive?

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:17:
// Use PostMessage to send "Hello World" to JavaScript.
On 2013/05/30 18:04:52, nfullagar1 wrote:
> /* */ style comments for c files?

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps.cc (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps.cc:25: /// The Module class. 
The browser calls the CreateInstance() method to create
On 2013/05/30 18:04:52, nfullagar1 wrote:
> /// ?

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc (right):

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:50: // main pepper
thread which must never be blocked.
On 2013/05/30 18:04:52, nfullagar1 wrote:
> small nit: there is some slight inconsistency in the comments for functions /
> classes.
> 
> Some (such as here) are:
> // comment comment comment
> // comment comment...
> 
> Others are:
> //
> // comment comment comment
> //
> 
> And others are:
> // function name
> //
> // comment comment comment...
> 
> And sometimes:
> //
> // function name
> //
> // comment comment comment...
> //

Done.

https://codereview.chromium.org/15011003/diff/45002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_simple/ps_instance.cc:201: //
initialization macro, or via dynamicallys set embed attributes
On 2013/05/30 18:04:52, nfullagar1 wrote:
> dynamically

Done.

[nfullagar1, 2013-05-30 21:07:31]

couple more comment style nits otherwise lgtm

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:212: // If we somehow have not
allocated these pointers yet, skip this frame.
/* */ line above

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:20:
// Register the function to call once the Instance Object is initialized.
mix of // & /* */ in this file

[noelallen1, 2013-05-30 21:58:03]

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
File native_client_sdk/src/examples/demo/life/life.c (right):

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
native_client_sdk/src/examples/demo/life/life.c:212: // If we somehow have not
allocated these pointers yet, skip this frame.
On 2013/05/30 21:07:31, nfullagar1 wrote:
> /* */ line above

Done.

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
File
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c
(right):

https://codereview.chromium.org/15011003/diff/63001/native_client_sdk/src/exa...
native_client_sdk/src/examples/getting_started/simple_hello_world/hello_world.c:20:
// Register the function to call once the Instance Object is initialized.
On 2013/05/30 21:07:31, nfullagar1 wrote:
> mix of // & /* */ in this file

Done.

[noelallen1, 2013-05-30 22:47:43]

Committed patchset #13 manually as r203244 (presubmit successful).