Add a mouselock example.

Add a mouselock example.

BUG=none
TEST=run_installer_test
Committed: http://code.google.com/p/nativeclient-sdk/source/detail?r=1359

[der Springer, 2011-11-10 02:24:14]

@jond: Please review the language in index_staging.html.
@cstefansen: Please glance over this to make sure I didn't fling out huge
important pieces of your original code.
others: review as usual.

[gwink1, 2011-11-10 14:54:33]

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock.cc
File examples/mouselock/mouselock.cc (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:11: typedef enum {
It would be nice to know what these are used for.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:107: if (fullscreen_pending_ &&
!saw_first_didchangeview_) {
One comment I got from Polina, on the fullscreen sample, is that she keeps track
of the FS state in class Fullscreen. Therefore it isn't necessary to do so here.
If you look at DidChangeView in FS Tumbler, you'll see that it doesn't have any
of this. (This stuff is required when using the C interface, to avoid too many
trips across RPC.)

[Matt Ball, 2011-11-10 15:03:10]

LGTM

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/build.scons
File examples/mouselock/build.scons (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/build.sco...
examples/mouselock/build.scons:1: #! -*- python -*-
I'm guessing that this was copied from one of the other examples.  You may
consider marking this as a copy using either the 'svn copy' or git. 
Unfortunately, git doesn't have a good way to detect copies, except by automatic
detection.  There's the '--find-copies-harder' option, but that may be too much
work...

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock.cc
File examples/mouselock/mouselock.cc (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:100: break;
If you're mixing some cases using 'break' and others using 'return', you may
consider making them all return something and then remove the 'return false' at
the end of the function.  That way, the compiler will check whether all the
cases are handled.

[jond, 2011-11-10 17:34:00]

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html
File examples/index_staging.html (right):

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html#...
examples/index_staging.html:102: <p>Teaching focus: switching to/from
full-screen display</p>
The teaching focus area has generally listed APIs. So, I'd suggest changing this
to "Full-screen"

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html#...
examples/index_staging.html:104: <dt><a
href="mouselock/mouselock.html">Full-screen and Mouse-lock</a></dt>
Every other example has a more generic and shorter application name such as
"Hello World," that doesn't describe the functionality. Maybe we need to rename
this to something like "Mouse locker."

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html#...
examples/index_staging.html:109: <p>Teaching focus: Locking and unlocking the
mouse cursor</p>
Does the example show how to go into full-screen? If so, we might want to list
it as something taught here. Mouse lock, Full-screen

[jond, 2011-11-10 17:34:14]

Done.

On Wed, Nov 9, 2011 at 7:24 PM, <dspringer@chromium.org> wrote:

> Reviewers: gwink1, mball, jond, cstefansen,
>
> Message:
> @jond: Please review the language in index_staging.html.
> @cstefansen: Please glance over this to make sure I didn't fling out huge
> important pieces of your original code.
> others: review as usual.
>
> Description:
> Add a mouselock example.
>
> BUG=none
> TEST=run_installer_test
>
> Please review this at
http://codereview.chromium.**org/8510041/<http://codereview.chromium.org/8510...
>
> SVN Base:
http://nativeclient-sdk.**googlecode.com/svn/trunk/src/<http://nativeclient-s...
>
> Affected files:
>  M     build_tools/installer_**contents.py
>  M     examples/build.scons
>  M     examples/index_staging.html
>  A     examples/mouselock/mouselock.**cc
>  A     examples/mouselock/mouselock.**html
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[cstefansen, 2011-11-10 17:53:03]

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock.cc
File examples/mouselock/mouselock.cc (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:9: #include "examples/mouselock/mouselock.h"
Nit: Google style suggests putting this include first
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...)

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:38: case PP_INPUTEVENT_TYPE_UNDEFINED:
Why not move this one down to the list of all the other event types that you
don't care about? (If there is a good reason, make a comment about it.)

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:126: if (!BindGraphics(device_context_)) {
Maybe issue a log message here?

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:158: if (image.is_null()) {
Log message? (This seems serious.)

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:203: for (int y = 0; y < image.size().height();
++y) {
Nit: it may be faster to init the entire canvas to kBackgroundColor, and then
only iterate over a bounding box defined by center_y +/- mouse_movement_.y() and
similarly for x. (You can probably make the bounding box tighter.) This would
shave off a number of GetDistance calculations.

[cstefansen, 2011-11-10 17:58:00]

One more nit.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
File examples/mouselock/mouselock.html (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.html:63: <li>Unlock mouse involuntarily (i.e.
Chrome unlock mouse):
Chrome unlock -> Chrome unlocks

[der Springer, 2011-11-11 19:23:01]

Issues addressed.  PTAL.  Thanks!

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html
File examples/index_staging.html (right):

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html#...
examples/index_staging.html:102: <p>Teaching focus: switching to/from
full-screen display</p>
On 2011/11/10 17:34:00, jond wrote:
> The teaching focus area has generally listed APIs. So, I'd suggest changing
this
> to "Full-screen"

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html#...
examples/index_staging.html:104: <dt><a
href="mouselock/mouselock.html">Full-screen and Mouse-lock</a></dt>
On 2011/11/10 17:34:00, jond wrote:
> Every other example has a more generic and shorter application name such as
> "Hello World," that doesn't describe the functionality. Maybe we need to
rename
> this to something like "Mouse locker."

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/index_staging.html#...
examples/index_staging.html:109: <p>Teaching focus: Locking and unlocking the
mouse cursor</p>
On 2011/11/10 17:34:00, jond wrote:
> Does the example show how to go into full-screen? If so, we might want to list
> it as something taught here. Mouse lock, Full-screen

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/build.scons
File examples/mouselock/build.scons (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/build.sco...
examples/mouselock/build.scons:1: #! -*- python -*-
On 2011/11/10 15:03:11, Matt Ball wrote:
> I'm guessing that this was copied from one of the other examples.  You may
> consider marking this as a copy using either the 'svn copy' or git. 
> Unfortunately, git doesn't have a good way to detect copies, except by
automatic
> detection.  There's the '--find-copies-harder' option, but that may be too
much
> work...

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock.cc
File examples/mouselock/mouselock.cc (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:9: #include "examples/mouselock/mouselock.h"
On 2011/11/10 17:53:03, cstefansen wrote:
> Nit: Google style suggests putting this include first
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...)

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:11: typedef enum {
On 2011/11/10 14:54:33, gwink1 wrote:
> It would be nice to know what these are used for.

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:38: case PP_INPUTEVENT_TYPE_UNDEFINED:
On 2011/11/10 17:53:03, cstefansen wrote:
> Why not move this one down to the list of all the other event types that you
> don't care about? (If there is a good reason, make a comment about it.)

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:100: break;
On 2011/11/10 15:03:11, Matt Ball wrote:
> If you're mixing some cases using 'break' and others using 'return', you may
> consider making them all return something and then remove the 'return false'
at
> the end of the function.  That way, the compiler will check whether all the
> cases are handled.

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:107: if (fullscreen_pending_ &&
!saw_first_didchangeview_) {
On 2011/11/10 14:54:33, gwink1 wrote:
> One comment I got from Polina, on the fullscreen sample, is that she keeps
track
> of the FS state in class Fullscreen. Therefore it isn't necessary to do so
here.
> If you look at DidChangeView in FS Tumbler, you'll see that it doesn't have
any
> of this. (This stuff is required when using the C interface, to avoid too many
> trips across RPC.)

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:126: if (!BindGraphics(device_context_)) {
On 2011/11/10 17:53:03, cstefansen wrote:
> Maybe issue a log message here?

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:158: if (image.is_null()) {
On 2011/11/10 17:53:03, cstefansen wrote:
> Log message? (This seems serious.)

Done.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.cc:203: for (int y = 0; y < image.size().height();
++y) {
On 2011/11/10 17:53:03, cstefansen wrote:
> Nit: it may be faster to init the entire canvas to kBackgroundColor, and then
> only iterate over a bounding box defined by center_y +/- mouse_movement_.y()
and
> similarly for x. (You can probably make the bounding box tighter.) This would
> shave off a number of GetDistance calculations.

Factored this routine into three parts and made it all go a little faster.

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
File examples/mouselock/mouselock.html (right):

http://codereview.chromium.org/8510041/diff/5007/examples/mouselock/mouselock...
examples/mouselock/mouselock.html:63: <li>Unlock mouse involuntarily (i.e.
Chrome unlock mouse):
On 2011/11/10 17:58:00, cstefansen wrote:
> Chrome unlock -> Chrome unlocks

Done.

[cstefansen, 2011-11-11 19:44:42]

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
File examples/mouselock/mouselock.cc (right):

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
examples/mouselock/mouselock.cc:193: assert(image);
Any reason not to use the 'Log("..."); return;' patterns you've used elsewhere?
(I'm all in favor of asserts normally, but in NaCl/Chrome if you run this on
Windows, it'll be hard to actually set it up so you can see the error because
the console tends to go to NUL:.)

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
examples/mouselock/mouselock.cc:208: assert(image);
Ditto.

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
examples/mouselock/mouselock.cc:229: uint32_t needle_color) {
Nit: you *could* check that image is not null like you did in the other
functions. (Or you could remove some of the checks; your call.)

[jond, 2011-11-11 20:07:18]

lgtm

[der Springer, 2011-11-11 21:21:37]

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
File examples/mouselock/mouselock.cc (right):

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
examples/mouselock/mouselock.cc:193: assert(image);
On 2011/11/11 19:44:43, cstefansen wrote:
> Any reason not to use the 'Log("..."); return;' patterns you've used
elsewhere?
> (I'm all in favor of asserts normally, but in NaCl/Chrome if you run this on
> Windows, it'll be hard to actually set it up so you can see the error because
> the console tends to go to NUL:.)

Done.

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
examples/mouselock/mouselock.cc:208: assert(image);
On 2011/11/11 19:44:43, cstefansen wrote:
> Ditto.

Done.

http://codereview.chromium.org/8510041/diff/15001/examples/mouselock/mouseloc...
examples/mouselock/mouselock.cc:229: uint32_t needle_color) {
On 2011/11/11 19:44:43, cstefansen wrote:
> Nit: you *could* check that image is not null like you did in the other
> functions. (Or you could remove some of the checks; your call.)

Done.

[cstefansen, 2011-11-11 22:25:43]

lgtm

[Matt Ball, 2011-11-11 22:42:39]

I'll take Christian's approval

On Fri, Nov 11, 2011 at 3:25 PM, <cstefansen@google.com> wrote:

> lgtm
>
>
>
>
http://codereview.chromium.**org/8510041/<http://codereview.chromium.org/8510...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.