PPAPI: Move PPB_Console out of dev.

PPAPI: Move PPB_Console out of dev.

PPB_Console was not added to the SRPC proxy because this proxy is deprecated.

BUG= none


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

[Nick Bray (chromium), 2012-11-27 22:36:14]

1) I have no idea what I am doing, so please review carefully.

2) I still want to fuzz/audit Var to make sure it won't pass junk strings to the
console.

3) LogWithSource seems pretty lame - it just prepends the source to the output. 
I want to dig into why this doesn't work "right".

But none of this is a show stopper for reviewing the CL...

[dmichael (off chromium), 2012-12-04 15:32:33]

lgtm

https://codereview.chromium.org/11416214/diff/1/ppapi/cpp/instance.h
File ppapi/cpp/instance.h (right):

https://codereview.chromium.org/11416214/diff/1/ppapi/cpp/instance.h#newcode485
ppapi/cpp/instance.h:485: const Var& value);
For public APIs, we try to keep the documentation complete and up-to-date in C++
and C, rather than sending developers all over the code to find documentation.
Can you put the C doc here and reword as necessary, instead of refering to
PPB_Console?

https://codereview.chromium.org/11416214/diff/1/ppapi/thunk/interfaces_ppb_pu...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/11416214/diff/1/ppapi/thunk/interfaces_ppb_pu...
ppapi/thunk/interfaces_ppb_public_stable.h:52: PROXIED_IFACE(PPB_Instance,
PPB_CONSOLE_INTERFACE_1_0, PPB_Console_1_0)
These should be sorted first by the API key (the first param to the macro), then
by interface name. (See the other PPB_Instance stuff below). I see we're a
little inconsistent, but I'd rather you put it in one of the 2 PPB_Instance
sections instead of creating a new one :)

[Nick Bray (chromium), 2012-12-04 23:14:56]

+cc brettw: do you have any comments?  I remember you were deferring explaining
something until I had the CL together.

Question: if we (for some unanticipated reason) can't do the IPC switch this
milestone, will the fact I haven't proxied SPRC be a problem?  I don't think so,
but I though I'd check.

https://codereview.chromium.org/11416214/diff/1/ppapi/cpp/instance.h
File ppapi/cpp/instance.h (right):

https://codereview.chromium.org/11416214/diff/1/ppapi/cpp/instance.h#newcode485
ppapi/cpp/instance.h:485: const Var& value);
On 2012/12/04 15:32:33, dmichael wrote:
> For public APIs, we try to keep the documentation complete and up-to-date in
C++
> and C, rather than sending developers all over the code to find documentation.
> Can you put the C doc here and reword as necessary, instead of refering to
> PPB_Console?

Done.

https://codereview.chromium.org/11416214/diff/1/ppapi/thunk/interfaces_ppb_pu...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/11416214/diff/1/ppapi/thunk/interfaces_ppb_pu...
ppapi/thunk/interfaces_ppb_public_stable.h:52: PROXIED_IFACE(PPB_Instance,
PPB_CONSOLE_INTERFACE_1_0, PPB_Console_1_0)
On 2012/12/04 15:32:33, dmichael wrote:
> These should be sorted first by the API key (the first param to the macro),
then
> by interface name. (See the other PPB_Instance stuff below). I see we're a
> little inconsistent, but I'd rather you put it in one of the 2 PPB_Instance
> sections instead of creating a new one :)

Done.

[dmichael (off chromium), 2012-12-04 23:20:01]

On Tue, Dec 4, 2012 at 4:14 PM, <ncbray@chromium.org> wrote:

> +cc brettw: do you have any comments?  I remember you were deferring
> explaining
> something until I had the CL together.
>
> Question: if we (for some unanticipated reason) can't do the IPC switch
> this
> milestone, will the fact I haven't proxied SPRC be a problem?  I don't
> think so,
> but I though I'd check.

Well, the interface wouldn't be found when calling GetInterface in M25,
which would just be inconsistent with the documentation of the API on the
web and shipped in the SDK. I think we could live with that for a release.


>
>
>
>
https://codereview.chromium.**org/11416214/diff/1/ppapi/cpp/**instance.h<http...
> File ppapi/cpp/instance.h (right):
>
> https://codereview.chromium.**org/11416214/diff/1/ppapi/cpp/**
>
instance.h#newcode485<https://codereview.chromium.org/11416214/diff/1/ppapi/cpp/instance.h#newcode485>
> ppapi/cpp/instance.h:485: const Var& value);
> On 2012/12/04 15:32:33, dmichael wrote:
>
>> For public APIs, we try to keep the documentation complete and
>>
> up-to-date in C++
>
>> and C, rather than sending developers all over the code to find
>>
> documentation.
>
>> Can you put the C doc here and reword as necessary, instead of
>>
> refering to
>
>> PPB_Console?
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11416214/diff/1/ppapi/**
>
thunk/interfaces_ppb_public_**stable.h<https://codereview.chromium.org/11416214/diff/1/ppapi/thunk/interfaces_ppb_public_stable.h>
> File ppapi/thunk/interfaces_ppb_**public_stable.h (right):
>
> https://codereview.chromium.**org/11416214/diff/1/ppapi/**
>
thunk/interfaces_ppb_public_**stable.h#newcode52<https://codereview.chromium.org/11416214/diff/1/ppapi/thunk/interfaces_ppb_public_stable.h#newcode52>
> ppapi/thunk/interfaces_ppb_**public_stable.h:52:
> PROXIED_IFACE(PPB_Instance, PPB_CONSOLE_INTERFACE_1_0, PPB_Console_1_0)
> On 2012/12/04 15:32:33, dmichael wrote:
>
>> These should be sorted first by the API key (the first param to the
>>
> macro), then
>
>> by interface name. (See the other PPB_Instance stuff below). I see
>>
> we're a
>
>> little inconsistent, but I'd rather you put it in one of the 2
>>
> PPB_Instance
>
>> sections instead of creating a new one :)
>>
>
> Done.
>
>
https://codereview.chromium.**org/11416214/<https://codereview.chromium.org/1...
>

[brettw, 2012-12-05 22:43:15]

https://codereview.chromium.org/11416214/diff/6001/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/11416214/diff/6001/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_public_stable.h:57: PROXIED_IFACE(PPB_Instance,
PPB_CONSOLE_INTERFACE_1_0, PPB_Console_1_0)
Can you add a second line here for backwards compat?

PROXIED_IFACE(PPB_Instance, "PPB_Console(Dev);0.1", PPB_Console_1_0)

Since the dev and non-dev vtable are exactly the same, you can just map the dev
string to the non-dev inteface table. This will keep e.g. Flash which uses the
dev interface able to log.

[Nick Bray (chromium), 2012-12-12 00:00:28]

PTAL, added a smoke test in the last patch.

The deck is clear, I will commit after approval.

https://codereview.chromium.org/11416214/diff/6001/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/11416214/diff/6001/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_public_stable.h:57: PROXIED_IFACE(PPB_Instance,
PPB_CONSOLE_INTERFACE_1_0, PPB_Console_1_0)
On 2012/12/05 22:43:15, brettw wrote:
> Can you add a second line here for backwards compat?
> 
> PROXIED_IFACE(PPB_Instance, "PPB_Console(Dev);0.1", PPB_Console_1_0)
> 
> Since the dev and non-dev vtable are exactly the same, you can just map the
dev
> string to the non-dev inteface table. This will keep e.g. Flash which uses the
> dev interface able to log.

Done.

[brettw, 2012-12-12 00:09:16]

lgtm

https://codereview.chromium.org/11416214/diff/19001/ppapi/thunk/interfaces_pp...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/11416214/diff/19001/ppapi/thunk/interfaces_pp...
ppapi/thunk/interfaces_ppb_public_stable.h:58: PROXIED_IFACE(PPB_Instance,
"PPB_Console(Dev);0.1", PPB_Console_1_0)
Sorry, I should have said to put this in the interfaces_ppb_public_dev.h file.
Can you also add a comment like:
// Map the old dev console interface to the stable one (which is the same) to
keep Flash, etc. working.

[dmichael (off chromium), 2012-12-12 19:09:07]

still lgtm

https://codereview.chromium.org/11416214/diff/29004/ppapi/tests/test_console.cc
File ppapi/tests/test_console.cc (right):

https://codereview.chromium.org/11416214/diff/29004/ppapi/tests/test_console....
ppapi/tests/test_console.cc:47: var_interface_->Release(source);
It might be cleaner to just use pp::Var; string conversion is easier, and it
manages the ref count for you.

https://codereview.chromium.org/11416214/diff/29004/ppapi/tests/test_console.h
File ppapi/tests/test_console.h (right):

https://codereview.chromium.org/11416214/diff/29004/ppapi/tests/test_console....
ppapi/tests/test_console.h:11: #include "ppapi/c/ppb_var.h"
total nit: These could both be forward references

https://codereview.chromium.org/11416214/diff/29004/ppapi/tests/test_console....
ppapi/tests/test_console.h:16: TestConsole(TestingInstance* instance);
nit: explicit

[commit-bot: I haz the power, 2012-12-12 19:45:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/11416214/16005

[commit-bot: I haz the power, 2012-12-12 19:45:49]

Presubmit check for 11416214-16005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
TODOs found in stable public PPAPI files:

***************
ppapi/cpp/instance.cc
***************

Presubmit checks took 4.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[dmichael (off chromium), 2012-12-12 20:40:03]

Either git cl dcommit, or...  you can change that TODO(brettw) to
FIXME(dmichael)...

or, if you're particularly ambitious, make that TODO not apply to .cc files
(ppapi/PRESUBMIT.py, line 69)

The check was put in so we wouldn't get TODOs showing up in public headers
and Doxygen-generated content.

On Wed, Dec 12, 2012 at 12:45 PM, <commit-bot@chromium.org> wrote:

> Presubmit check for 11416214-16005 failed and returned exit status 1.
>
>
> Running presubmit commit checks ...
>
> ** Presubmit ERRORS **
> TODOs found in stable public PPAPI files:
>
> ***************
> ppapi/cpp/instance.cc
> ***************
>
> Presubmit checks took 4.5s to calculate.
>
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!
>
>
>
https://chromiumcodereview.**appspot.com/11416214/<https://chromiumcodereview...
>

[Nick Bray (chromium), 2012-12-12 20:48:44]

Will manually check in after try job.  Will fix presubmit in separate CL.

On 2012/12/12 20:40:03, dmichael wrote:
> Either git cl dcommit, or...  you can change that TODO(brettw) to
> FIXME(dmichael)...
> 
> or, if you're particularly ambitious, make that TODO not apply to .cc files
> (ppapi/PRESUBMIT.py, line 69)
> 
> The check was put in so we wouldn't get TODOs showing up in public headers
> and Doxygen-generated content.
> 
> On Wed, Dec 12, 2012 at 12:45 PM, <mailto:commit-bot@chromium.org> wrote:
> 
> > Presubmit check for 11416214-16005 failed and returned exit status 1.
> >
> >
> > Running presubmit commit checks ...
> >
> > ** Presubmit ERRORS **
> > TODOs found in stable public PPAPI files:
> >
> > ***************
> > ppapi/cpp/instance.cc
> > ***************
> >
> > Presubmit checks took 4.5s to calculate.
> >
> > Was the presubmit check useful? Please send feedback & hate mail to
> > maruel@chromium.org!
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/11416214/%3Chttps://chromiumcoderevi...>
> >

[commit-bot: I haz the power, 2012-12-12 22:37:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/11416214/16005

[commit-bot: I haz the power, 2012-12-13 01:34:34]

Change committed as 172762