Make GlobalDescriptors::MaybeGet return -1 when the key is not found.

Make GlobalDescriptors::MaybeGet return -1 when the key is not found.

BUG=None
TEST=Unit tests should still pass.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144350

[agl, 2012-05-21 18:18:34]

LGTM if the tests pass.

[Jay Civelli, 2012-05-24 05:15:40]

Sorry I had not realized it would be the ipc_tests that would fail, not the
unit_tests.
They now pass with these new changes, not entirely sure if that the right thing
to do though.

PTAL, thanks!

Jay

[agl, 2012-05-24 16:30:31]

Ok, I was surprised that the tests just passed :)

This updated patch LGTM.

[jeremy, 2012-06-13 19:25:55]

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_channel_posix_unit...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_channel_posix_unit...
ipc/ipc_channel_posix_unittest.cc:409:
MULTIPROCESS_TEST_MAIN(IPCChannelPosixFailConnectionProc,
Can you define a version of this Macro that defines the second parameter so
MULTIPROCESS_IPC_TEST_MAIN or somesuch ?

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_tests.h
File ipc/ipc_tests.h (right):

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_tests.h#newcode50
ipc/ipc_tests.h:50: // MULTIPROCESS_TEST_MAIN macro.
Need a more descriptive comment here e.g. what does this function do...

http://codereview.chromium.org/10387218/diff/19001/testing/multiprocess_func_...
File testing/multiprocess_func_list.h (right):

http://codereview.chromium.org/10387218/diff/19001/testing/multiprocess_func_...
testing/multiprocess_func_list.h:21: //  MULTIPROCESS_TEST_MAIN(a_test_func,
a_setup_fun) {
Need to document what setup_func does...

http://codereview.chromium.org/10387218/diff/19001/testing/multiprocess_func_...
testing/multiprocess_func_list.h:52: #define MULTIPROCESS_TEST_MAIN(test_main,
test_setup) \
I think you want to create 2 versions of this macro, one with a setup function
and one without, so users who don't need the setup function can just ignore it -
that will also make this CL smaller :)

[Jay Civelli, 2012-06-13 20:40:49]

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_channel_posix_unit...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_channel_posix_unit...
ipc/ipc_channel_posix_unittest.cc:409:
MULTIPROCESS_TEST_MAIN(IPCChannelPosixFailConnectionProc,
On 2012/06/13 19:25:55, jeremy wrote:
> Can you define a version of this Macro that defines the second parameter so
> MULTIPROCESS_IPC_TEST_MAIN or somesuch ?

Good idea, done.

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_tests.h
File ipc/ipc_tests.h (right):

http://codereview.chromium.org/10387218/diff/19001/ipc/ipc_tests.h#newcode50
ipc/ipc_tests.h:50: // MULTIPROCESS_TEST_MAIN macro.
On 2012/06/13 19:25:55, jeremy wrote:
> Need a more descriptive comment here e.g. what does this function do...

Done.

http://codereview.chromium.org/10387218/diff/19001/testing/multiprocess_func_...
File testing/multiprocess_func_list.h (right):

http://codereview.chromium.org/10387218/diff/19001/testing/multiprocess_func_...
testing/multiprocess_func_list.h:21: //  MULTIPROCESS_TEST_MAIN(a_test_func,
a_setup_fun) {
On 2012/06/13 19:25:55, jeremy wrote:
> Need to document what setup_func does...

I documented it in the comment for the new macro
MULTIPROCESS_TEST_MAIN_WITH_SETUP

http://codereview.chromium.org/10387218/diff/19001/testing/multiprocess_func_...
testing/multiprocess_func_list.h:52: #define MULTIPROCESS_TEST_MAIN(test_main,
test_setup) \
On 2012/06/13 19:25:55, jeremy wrote:
> I think you want to create 2 versions of this macro, one with a setup function
> and one without, so users who don't need the setup function can just ignore it
-
> that will also make this CL smaller :)

Done.

[jeremy, 2012-06-13 20:46:28]

http://codereview.chromium.org/10387218/diff/20019/ipc/ipc_tests.h
File ipc/ipc_tests.h (right):

http://codereview.chromium.org/10387218/diff/20019/ipc/ipc_tests.h#newcode12
ipc/ipc_tests.h:12: #define MULTIPROCESS_IPC_TEST_MAIN(test_main) \
Can you add a comment that explains when a test needs to use this.

http://codereview.chromium.org/10387218/diff/20019/ipc/ipc_tests.h#newcode53
ipc/ipc_tests.h:53: // Registers the IPC channel as a global descriptor on
Posix.
Can you expand this comment, esp. it's not clear why this is needed.

http://codereview.chromium.org/10387218/diff/20019/testing/multiprocess_func_...
File testing/multiprocess_func_list.h (right):

http://codereview.chromium.org/10387218/diff/20019/testing/multiprocess_func_...
testing/multiprocess_func_list.h:31: typedef int (*ChildFunctionPtr)();
I'd rename this to TestMainFunctionPtr or somesuch and add another typdef that
returns a void of type SetupFunctionPtr.

http://codereview.chromium.org/10387218/diff/20019/testing/multiprocess_func_...
testing/multiprocess_func_list.h:37: AppendMultiProcessTest(std::string
test_name,
Need a short comment explaining the arguments.

http://codereview.chromium.org/10387218/diff/20019/testing/multiprocess_func_...
testing/multiprocess_func_list.h:51: // Same as above but lets callers specify a
setup method that should be run when
nit: should be -> is

http://codereview.chromium.org/10387218/diff/20019/testing/multiprocess_func_...
testing/multiprocess_func_list.h:53: // value is ignored.
per comment above - should return void, and then you can remove the end of this
comment.

[Jay Civelli, 2012-06-22 23:28:29]

Jeremy,
Sorry for the delay, I got sidetracked last week.
I had to factor out the MULTIPROCESS_IPC_TEST_MAIN macro and its associated
method in a separate file and include it with the test_support_ipc so other
tests exes could depend on it.
I also moved the execution of the setup method from the AppendMultiProcessTest
constructor to InvokeChildProcessTest (where main is invoked), as otherwise
defining only 1 MULTIPROCESS_IPC_TEST_MAIN would run cause the setup to be run
for any subprocess, even the ones not using that multiprocess main.

https://chromiumcodereview.appspot.com/10387218/diff/20019/ipc/ipc_tests.h
File ipc/ipc_tests.h (right):

https://chromiumcodereview.appspot.com/10387218/diff/20019/ipc/ipc_tests.h#ne...
ipc/ipc_tests.h:12: #define MULTIPROCESS_IPC_TEST_MAIN(test_main) \
On 2012/06/13 20:46:28, jeremy wrote:
> Can you add a comment that explains when a test needs to use this.

Done.

https://chromiumcodereview.appspot.com/10387218/diff/20019/ipc/ipc_tests.h#ne...
ipc/ipc_tests.h:53: // Registers the IPC channel as a global descriptor on
Posix.
On 2012/06/13 20:46:28, jeremy wrote:
> Can you expand this comment, esp. it's not clear why this is needed.

Done.

https://chromiumcodereview.appspot.com/10387218/diff/20019/testing/multiproce...
File testing/multiprocess_func_list.h (right):

https://chromiumcodereview.appspot.com/10387218/diff/20019/testing/multiproce...
testing/multiprocess_func_list.h:31: typedef int (*ChildFunctionPtr)();
On 2012/06/13 20:46:28, jeremy wrote:
> I'd rename this to TestMainFunctionPtr or somesuch and add another typdef that
> returns a void of type SetupFunctionPtr.

Done.

https://chromiumcodereview.appspot.com/10387218/diff/20019/testing/multiproce...
testing/multiprocess_func_list.h:37: AppendMultiProcessTest(std::string
test_name,
On 2012/06/13 20:46:28, jeremy wrote:
> Need a short comment explaining the arguments.

Done.

https://chromiumcodereview.appspot.com/10387218/diff/20019/testing/multiproce...
testing/multiprocess_func_list.h:51: // Same as above but lets callers specify a
setup method that should be run when
On 2012/06/13 20:46:28, jeremy wrote:
> nit: should be -> is

Done.

https://chromiumcodereview.appspot.com/10387218/diff/20019/testing/multiproce...
testing/multiprocess_func_list.h:53: // value is ignored.
On 2012/06/13 20:46:28, jeremy wrote:
> per comment above - should return void, and then you can remove the end of
this
> comment.

Done.

[jeremy, 2012-06-24 13:49:37]

LGTM with nits.

https://chromiumcodereview.appspot.com/10387218/diff/30013/ipc/ipc_multiproce...
File ipc/ipc_multiprocess_test.h (right):

https://chromiumcodereview.appspot.com/10387218/diff/30013/ipc/ipc_multiproce...
ipc/ipc_multiprocess_test.h:19: // is needed on Posix as the IPCChannel when
created looks for a specific global
nit: capitalize POSIX

https://chromiumcodereview.appspot.com/10387218/diff/30013/testing/multiproce...
File testing/multiprocess_func_list.h (right):

https://chromiumcodereview.appspot.com/10387218/diff/30013/testing/multiproce...
testing/multiprocess_func_list.h:58: // global mapping is added. |test_setup| is
a SetupFunctionPtr.
* when the global mapping is added -> in the child process, just before the main
function is added.  This facilitates adding a generic one-time setup function
for multiple tests.

[Jay Civelli, 2012-06-25 17:49:55]

Hi Miranda,
Could you take a look at:
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
I need an owner's approval.

Thanks!

Jay

http://codereview.chromium.org/10387218/diff/30013/ipc/ipc_multiprocess_test.h
File ipc/ipc_multiprocess_test.h (right):

http://codereview.chromium.org/10387218/diff/30013/ipc/ipc_multiprocess_test....
ipc/ipc_multiprocess_test.h:19: // is needed on Posix as the IPCChannel when
created looks for a specific global
On 2012/06/24 13:49:37, jeremy wrote:
> nit: capitalize POSIX

Done.

http://codereview.chromium.org/10387218/diff/30013/testing/multiprocess_func_...
File testing/multiprocess_func_list.h (right):

http://codereview.chromium.org/10387218/diff/30013/testing/multiprocess_func_...
testing/multiprocess_func_list.h:58: // global mapping is added. |test_setup| is
a SetupFunctionPtr.
On 2012/06/24 13:49:37, jeremy wrote:
> * when the global mapping is added -> in the child process, just before the
main
> function is added.  This facilitates adding a generic one-time setup function
> for multiple tests.

Done.

[Miranda Callahan, 2012-06-25 18:57:08]

Hi Jay,

I hope you mean "importer/*", because I haven't a clue about cloud_print. :-)
LGTM for importer/* stuff.

On 2012/06/25 17:49:55, Jay Civelli wrote:
> Hi Miranda,
> Could you take a look at:
>
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
> I need an owner's approval.
> 
> Thanks!
> 
> Jay
> 
> http://codereview.chromium.org/10387218/diff/30013/ipc/ipc_multiprocess_test.h
> File ipc/ipc_multiprocess_test.h (right):
> 
>
http://codereview.chromium.org/10387218/diff/30013/ipc/ipc_multiprocess_test....
> ipc/ipc_multiprocess_test.h:19: // is needed on Posix as the IPCChannel when
> created looks for a specific global
> On 2012/06/24 13:49:37, jeremy wrote:
> > nit: capitalize POSIX
> 
> Done.
> 
>
http://codereview.chromium.org/10387218/diff/30013/testing/multiprocess_func_...
> File testing/multiprocess_func_list.h (right):
> 
>
http://codereview.chromium.org/10387218/diff/30013/testing/multiprocess_func_...
> testing/multiprocess_func_list.h:58: // global mapping is added. |test_setup|
is
> a SetupFunctionPtr.
> On 2012/06/24 13:49:37, jeremy wrote:
> > * when the global mapping is added -> in the child process, just before the
> main
> > function is added.  This facilitates adding a generic one-time setup
function
> > for multiple tests.
> 
> Done.

[Jay Civelli, 2012-06-26 22:33:55]

Brad,
Could you look at the chrome/nacl/nacl_helper_linux.cc change?

Thanks.

Jay

[Brad Chen, 2012-06-26 22:45:13]

LGTM for nacl_helper_linux.cc

I'm not confident I know the descriptor handling code well enough to read this
with perfect clarity. The trybot says NaCl worked fine on 64-bit Linux, and as
this is portable code it's unlikely 32-bit would behave differently. It would be
nice to try this on 32-bit too; sadly I don't think there are 32-bit Linux
trybots.

On 2012/06/26 22:33:55, Jay Civelli wrote:
> Brad,
> Could you look at the chrome/nacl/nacl_helper_linux.cc change?
> 
> Thanks.
> 
> Jay

[commit-bot: I haz the power, 2012-06-26 22:48:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/10387218/40001

[commit-bot: I haz the power, 2012-06-26 22:48:18]

Presubmit check for 10387218-40001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base
    chrome/browser/printing/cloud_print

Presubmit checks took 1.4s to calculate.

[Jay Civelli, 2012-06-26 22:53:11]

Brett,
I need an owner approval for base/. Could you take a look? (I already have an
LGTM from agl for that file)

Scott,
Could you look at the small cloud print change, I need approval from an OWNER?

Thanks.

[Scott Byer, 2012-06-26 22:56:08]

CloudPrint LGTM.

On 2012/06/26 22:53:11, Jay Civelli wrote:
> Brett,
> I need an owner approval for base/. Could you take a look? (I already have an
> LGTM from agl for that file)
> 
> Scott,
> Could you look at the small cloud print change, I need approval from an OWNER?
> 
> Thanks.

[brettw, 2012-06-26 23:47:51]

base lgtm rubberstamp

[commit-bot: I haz the power, 2012-06-26 23:48:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/10387218/40001

[commit-bot: I haz the power, 2012-06-27 01:12:26]

Change committed as 144350