Fix PPB_MouseLock.LockMouse crash and add tests.

Fix PPB_MouseLock.LockMouse crash and add tests.

Because mouse lock tests require focus, the new tests are put in interactive_ui_tests.
In this CL, ppapi_uitest is split into three parts:
1) ppapi_test: contains PPAPITestBase and its subclasses.
2) ppapi_browsertest: used by browser_tests.
3) ppapi_interactive_browsertest: used by interactive_ui_tests.

Besides, they are moved into a new folder, chrome/test/ppapi.
(1) and (2) are less interesting since they are mostly copied from ppapi_uitest.

BUG=135206
TEST=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145708

[yzshen1, 2012-06-30 02:58:17]

Hi, Dave.

Would you please take a look? Thanks!
(And sorry for taking so long to write the tests!)

[scheib, 2012-06-30 03:18:58]

https://chromiumcodereview.appspot.com/10699045/diff/1/ppapi/tests/test_mouse...
File ppapi/tests/test_mouse_lock.cc (right):

https://chromiumcodereview.appspot.com/10699045/diff/1/ppapi/tests/test_mouse...
ppapi/tests/test_mouse_lock.cc:59: if (rv == PP_OK_COMPLETIONPENDING)
How does this test differ from TestSucceedWhenAllowed? What causes it to block
the lock? --- Oh, I see, the content settings are configured per test. Perhaps
mention that here?

[yzshen1, 2012-06-30 21:39:14]

https://chromiumcodereview.appspot.com/10699045/diff/1/ppapi/tests/test_mouse...
File ppapi/tests/test_mouse_lock.cc (right):

https://chromiumcodereview.appspot.com/10699045/diff/1/ppapi/tests/test_mouse...
ppapi/tests/test_mouse_lock.cc:59: if (rv == PP_OK_COMPLETIONPENDING)
On 2012/06/30 03:18:58, scheib wrote:
> How does this test differ from TestSucceedWhenAllowed? What causes it to block
> the lock? --- Oh, I see, the content settings are configured per test. Perhaps
> mention that here?

Done. Thanks!

[dmichael (off chromium), 2012-07-02 17:34:16]

http://codereview.chromium.org/10699045/diff/10001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10699045/diff/10001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:517: 'test/ppapi/ppapi_interactive_browsertest.cc',
Is it appropriate to put this here? We simulate the pepper input events with the
PPB_Testing_Dev API, and I'm not sure what kinds of tests should be
interactive_ui_tests.

(I don't mean to imply that what you've done is wrong; I simply don't know. I
want to make sure you've checked with somebody who does know, like maybe jam. If
we don't need to have the tests separated, I'd rather keep them in one place.)

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:179: TEST_PPAPI_NACL_VIA_HTTP(Graphics2D)
In ppapi_uitest.cc, this was broken out in to multiple tests, since supposedly
some of them didn't work in NaCl. Has that changed?

Also, where's Graphics3D?

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:656:
TEST_PPAPI_OUT_OF_PROCESS(AudioConfig_InvalidConfigs)
Why did you remove the NACL versions?

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:755: #endif // ADDRESS_SANITIZER
I think I'm going to take your word for it on this file...

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_te...
File chrome/test/ppapi/ppapi_test.cc (right):

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_te...
chrome/test/ppapi/ppapi_test.cc:116: // The test sends us the result via a
cookie.
There was a bit about "Do not use mesa if real GPU is required" here. Is it okay
to remove that?

http://codereview.chromium.org/10699045/diff/10001/ppapi/proxy/ppb_instance_p...
File ppapi/proxy/ppb_instance_proxy.cc (left):

http://codereview.chromium.org/10699045/diff/10001/ppapi/proxy/ppb_instance_p...
ppapi/proxy/ppb_instance_proxy.cc:654: enter.callback());
Ugh, sorry for getting this wrong. I must have been hacking through files too
quickly.

http://codereview.chromium.org/10699045/diff/10001/ppapi/tests/test_mouse_loc...
File ppapi/tests/test_mouse_lock.cc (right):

http://codereview.chromium.org/10699045/diff/10001/ppapi/tests/test_mouse_loc...
ppapi/tests/test_mouse_lock.cc:42: TestCompletionCallback
callback(instance_->pp_instance());
Optional suggestion: If you also provide the callback_type() argument, you could
make this work for OPTIONAL, REQUIRED, and BLOCKING callbacks.

http://codereview.chromium.org/10699045/diff/10001/ppapi/tests/test_mouse_loc...
ppapi/tests/test_mouse_lock.cc:44: int32_t rv = LockMouse(callback);
nit: You could do:
callback.WaitForResult(LockMouse(callback));
ASSERT_EQ(PP_OK, callback.result());
(... If you do the above suggestion, you'll have to do this so it will work for
any of those 3 callback types.)

http://codereview.chromium.org/10699045/diff/10001/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (right):

http://codereview.chromium.org/10699045/diff/10001/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:246: //                 MouseLock (maybe others?).
nit: This should probably be indented at the same level as EnterBase.
(Oops, that was my fault.)

[jam, 2012-07-02 17:58:02]

which parts do you need me to review?

[yzshen1, 2012-07-02 18:00:16]

On 2012/07/02 17:58:02, John Abd-El-Malek wrote:
> which parts do you need me to review?

Hi, John. Would you please take a look at 
chrome/test/ppapi/ppapi_interactive_browsertest.cc?
And please let us know if it is appropriate to put it in the
interactive_ui_tests.

I put it in the interactive_ui_tests because it requires focus to work properly.
Other mouse lock tests are in this group, too.

Thanks!

[yzshen1, 2012-07-02 19:20:20]

http://codereview.chromium.org/10699045/diff/10001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10699045/diff/10001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:517: 'test/ppapi/ppapi_interactive_browsertest.cc',
On 2012/07/02 17:34:16, dmichael wrote:
> Is it appropriate to put this here? We simulate the pepper input events with
the
> PPB_Testing_Dev API, and I'm not sure what kinds of tests should be
> interactive_ui_tests.
> 
> (I don't mean to imply that what you've done is wrong; I simply don't know. I
> want to make sure you've checked with somebody who does know, like maybe jam.
If
> we don't need to have the tests separated, I'd rather keep them in one place.)

It requires focus to work properly, which means we cannot run it in parallel
with others. browser_tests does sharding while interactive_ui_tests doesn't.
Other mouse lock tests are put in this group, too.
(fullscreen_controller_interactive_browsertest)

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:179: TEST_PPAPI_NACL_VIA_HTTP(Graphics2D)
On 2012/07/02 17:34:16, dmichael wrote:
> In ppapi_uitest.cc, this was broken out in to multiple tests, since supposedly
> some of them didn't work in NaCl. Has that changed?
> 
> Also, where's Graphics3D?
It is because my client is a few days old. So it doesn't reflect these recent
changes.

I will make sure it is up-to-date.

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:656:
TEST_PPAPI_OUT_OF_PROCESS(AudioConfig_InvalidConfigs)
On 2012/07/02 17:34:16, dmichael wrote:
> Why did you remove the NACL versions?
Please see my comment above.

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:755: #endif // ADDRESS_SANITIZER
:)

On 2012/07/02 17:34:16, dmichael wrote:
> I think I'm going to take your word for it on this file...

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_te...
File chrome/test/ppapi/ppapi_test.cc (right):

http://codereview.chromium.org/10699045/diff/10001/chrome/test/ppapi/ppapi_te...
chrome/test/ppapi/ppapi_test.cc:116: // The test sends us the result via a
cookie.
On 2012/07/02 17:34:16, dmichael wrote:
> There was a bit about "Do not use mesa if real GPU is required" here. Is it
okay
> to remove that?

No. I just used a slightly old client. I will sync and make sure it reflects the
latest change.
I didn't intent to make any changes to existing tests.

http://codereview.chromium.org/10699045/diff/10001/ppapi/tests/test_mouse_loc...
File ppapi/tests/test_mouse_lock.cc (right):

http://codereview.chromium.org/10699045/diff/10001/ppapi/tests/test_mouse_loc...
ppapi/tests/test_mouse_lock.cc:42: TestCompletionCallback
callback(instance_->pp_instance());
On 2012/07/02 17:34:16, dmichael wrote:
> Optional suggestion: If you also provide the callback_type() argument, you
could
> make this work for OPTIONAL, REQUIRED, and BLOCKING callbacks.

Done.

http://codereview.chromium.org/10699045/diff/10001/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (right):

http://codereview.chromium.org/10699045/diff/10001/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:246: //                 MouseLock (maybe others?).
On 2012/07/02 17:34:16, dmichael wrote:
> nit: This should probably be indented at the same level as EnterBase.
> (Oops, that was my fault.)

Done.

[dmichael (off chromium), 2012-07-02 19:46:32]

One more thing...  we probably should add a new OWNERS file in chrome/test/ppapi
so we can stop bugging sky et al (assuming one of those owners of chrome/test is
okay with it). That could be a separate CL if you think it makes more sense.

W.r.t. the changes from ppapi_uitest.cc: since git didn't detect the copies, I
downloaded the patch and manually diffed rather than look at all of it. If
you're sure you got everything (i.e., all the things I pointed out were just
because our clients were out of sync) then lgtm.

[jam, 2012-07-03 00:56:12]

it's very unfortunate to lose svn history for this file.

Can you do this without losing history? i.e. commit through svn, or figure out
how to tell git that this file was moved?

http://codereview.chromium.org/10699045/diff/5003/ppapi/tests/test_mouse_lock.h
File ppapi/tests/test_mouse_lock.h (right):

http://codereview.chromium.org/10699045/diff/5003/ppapi/tests/test_mouse_lock...
ppapi/tests/test_mouse_lock.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
nit: do you really need a header for this test file? i.e. it seems you can put
this all in the cc file

[yzshen1, 2012-07-03 21:32:10]

Fix mouse lock.

[yzshen1, 2012-07-03 21:39:16]

Hi, Dave and John.

Thanks!

I have switched to svn to preserve the history. It looks much nicer now. Thanks!

And I have added an OWNERS file, which includes all owners from ppapi/ except
brettw, because he is already an owner of a parent directory. Please let me know
whether it looks okay to you.

http://codereview.chromium.org/10699045/diff/5003/ppapi/tests/test_mouse_lock.h
File ppapi/tests/test_mouse_lock.h (right):

http://codereview.chromium.org/10699045/diff/5003/ppapi/tests/test_mouse_lock...
ppapi/tests/test_mouse_lock.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
All other test_<ppapi_interface> have corresponding .h files. Some of them are
also pretty short.
I think it is good to be consistent.

On 2012/07/03 00:56:12, John Abd-El-Malek wrote:
> nit: do you really need a header for this test file? i.e. it seems you can put
> this all in the cc file

[jam, 2012-07-04 04:43:00]

lgtm, thanks for preserving the history

http://codereview.chromium.org/10699045/diff/5003/ppapi/tests/test_mouse_lock.h
File ppapi/tests/test_mouse_lock.h (right):

http://codereview.chromium.org/10699045/diff/5003/ppapi/tests/test_mouse_lock...
ppapi/tests/test_mouse_lock.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2012/07/03 21:39:17, yzshen1 wrote:
> All other test_<ppapi_interface> have corresponding .h files. Some of them are
> also pretty short.
> I think it is good to be consistent.
> 
> On 2012/07/03 00:56:12, John Abd-El-Malek wrote:
> > nit: do you really need a header for this test file? i.e. it seems you can
put
> > this all in the cc file
> 

ah, I wasn't familiar with that convention.

[commit-bot: I haz the power, 2012-07-05 02:53:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10699045/5006

[commit-bot: I haz the power, 2012-07-05 02:53:06]

Failed to apply patch for chrome/test/ppapi/ppapi_browsertest.cc:
While running patch -p0 --forward --force;
patching file chrome/test/ppapi/ppapi_browsertest.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/ppapi/ppapi_browsertest.cc.rej

[commit-bot: I haz the power, 2012-07-09 17:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10699045/16001

[commit-bot: I haz the power, 2012-07-09 17:10:21]

Failed to apply patch for chrome/test/ppapi/ppapi_browsertest.cc:
While running patch -p0 --forward --force;
patching file chrome/test/ppapi/ppapi_browsertest.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/ppapi/ppapi_browsertest.cc.rej

[commit-bot: I haz the power, 2012-07-09 17:11:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10699045/16001

[commit-bot: I haz the power, 2012-07-09 17:11:23]

Failed to apply patch for chrome/test/ppapi/ppapi_browsertest.cc:
While running patch -p0 --forward --force;
patching file chrome/test/ppapi/ppapi_browsertest.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/ppapi/ppapi_browsertest.cc.rej