Add an extensions keep-alive client.

Add an extensions keep-alive client.

This adds the renderer side of adding keep-alives while API calls
implemented on mojo services are in progress.

BUG=389016
Committed: https://crrev.com/d9fddbc5700ebdea8d4643b4d2c2f6f76abd4b30
Cr-Commit-Position: refs/heads/master@{#301528}

[Sam McNally, 2014-10-24 04:13:32]

Patchset #3 (id:40001) has been deleted

[Sam McNally, 2014-10-24 04:13:40]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2014-10-24 04:13:48]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2014-10-24 04:16:40]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2014-10-24 04:18:05]

sammc@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-10-24 18:29:55]

[This comment should really be read chronologically after the inline comments:)]

Keep-alive client in general LG(TM), but I feel like there's a huge disconnect
between the tests that are being run and the keep-alive count expectations.

RUN(some_serial_test_js);
EXPECT(5 keep-alives);

Really only seems to test a specific (and as I see it, potentially fragile)
implementation detail of the serial API and/or extension-in-test.

Do we really need a keep-alive counter at all? It would be nice if we could
instead introduce a separate set of keep-alive unit tests that make explicit
renderer-JS-side calls to acquire and close keepalives, while we validating
expected service state along the way. WDYT?

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api/...
File extensions/renderer/api/serial/serial_api_unittest.cc (right):

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api/...
extensions/renderer/api/serial/serial_api_unittest.cc:453: EXPECT_EQ(1,
keep_alive_counter().keep_alives());
It is not immediately clear to me when looking at these tests why you would want
to make assertions about keepalive counts *everywhere*. Is this a pattern we
expect everyone to follow when writing API tests? (I think not)

Is this just temporary because serial is the only available keep-alive client
for now?

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api_...
File extensions/renderer/api_test_base.h (right):

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api_...
extensions/renderer/api_test_base.h:66: class KeepAliveCounter {
nit: What do you think about moving KeepAliveCounter and maybe
TestServiceProvider into their own source files? Doing so might also warrant
introducing a new //extensions/renderer/test/ home to keep them separated from
everything else. 

[OR we could invert this and like //content have an //extensions/test/ (and
maybe even subpaths for {browser, common, renderer}), but I don't want to fret
too much over this detail for your CL]

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/reso...
File extensions/renderer/resources/keep_alive.js (right):

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/reso...
extensions/renderer/resources/keep_alive.js:11: function KeepAlive() {
Documentation? It would be nice (IMO) to have our JS code littered with
appropriate type annotations, but I actually don't know how to name types in
this require-js universe.

What is the qualified name of the mojo Router type? Is it
mojo.public.js.bindings.router.Router? Should it be assigned via annotation in
router.js?

Anyway not looking to answer these questions here, but we should be thinking
about it. Compileable JS throughout Chrome is a very worthy goal to keep in
mind.

[Sam McNally, 2014-10-27 04:58:36]

Patchset #2 (id:90001) has been deleted

[Sam McNally, 2014-10-27 05:07:25]

On 2014/10/24 18:29:55, Ken Rockot wrote:
> [This comment should really be read chronologically after the inline
comments:)]
> 
> Keep-alive client in general LG(TM), but I feel like there's a huge disconnect
> between the tests that are being run and the keep-alive count expectations.
> 
> RUN(some_serial_test_js);
> EXPECT(5 keep-alives);
> 
> Really only seems to test a specific (and as I see it, potentially fragile)
> implementation detail of the serial API and/or extension-in-test.
> 
> Do we really need a keep-alive counter at all? It would be nice if we could
> instead introduce a separate set of keep-alive unit tests that make explicit
> renderer-JS-side calls to acquire and close keepalives, while we validating
> expected service state along the way. WDYT?

I wasn't particularly happy with the way those tests ended up, but I wanted the
tests to cover that making API calls triggered keep-alives. I've switched to
some keep-alive specific tests.

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api/...
File extensions/renderer/api/serial/serial_api_unittest.cc (right):

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api/...
extensions/renderer/api/serial/serial_api_unittest.cc:453: EXPECT_EQ(1,
keep_alive_counter().keep_alives());
On 2014/10/24 18:29:54, Ken Rockot wrote:
> It is not immediately clear to me when looking at these tests why you would
want
> to make assertions about keepalive counts *everywhere*. Is this a pattern we
> expect everyone to follow when writing API tests? (I think not)
> 
> Is this just temporary because serial is the only available keep-alive client
> for now?

It was like this because serial is the only client, but I've realized that I can
use special custom bindings in the tests to avoid needing to bolt keep-alive
client testing onto serial like this.

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api_...
File extensions/renderer/api_test_base.h (right):

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/api_...
extensions/renderer/api_test_base.h:66: class KeepAliveCounter {
On 2014/10/24 18:29:54, Ken Rockot wrote:
> nit: What do you think about moving KeepAliveCounter and maybe
> TestServiceProvider into their own source files? Doing so might also warrant
> introducing a new //extensions/renderer/test/ home to keep them separated from
> everything else. 
> 
> [OR we could invert this and like //content have an //extensions/test/ (and
> maybe even subpaths for {browser, common, renderer}), but I don't want to fret
> too much over this detail for your CL]

TestServiceProvider is fairly tightly coupled to ApiTestBase so I think it's
reasonable for it to live here. Removed KeepAliveCounter.

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/reso...
File extensions/renderer/resources/keep_alive.js (right):

https://codereview.chromium.org/669303002/diff/50008/extensions/renderer/reso...
extensions/renderer/resources/keep_alive.js:11: function KeepAlive() {
On 2014/10/24 18:29:55, Ken Rockot wrote:
> Documentation? It would be nice (IMO) to have our JS code littered with
> appropriate type annotations, but I actually don't know how to name types in
> this require-js universe.
> 
> What is the qualified name of the mojo Router type? Is it
> mojo.public.js.bindings.router.Router? Should it be assigned via annotation in
> router.js?
> 
> Anyway not looking to answer these questions here, but we should be thinking
> about it. Compileable JS throughout Chrome is a very worthy goal to keep in
> mind.

Done. (Switched to holding the handle directly since we never send or receive
any messages with it).

I'm not sure how naming works, but I would guess
module:mojo/public/js/bindings/router~Router. I expect router.js would need to
be annotated for it to be useful.

[Ken Rockot(use gerrit already), 2014-10-27 15:27:31]

On 2014/10/27 05:07:25, Sam McNally wrote:
> I'm not sure how naming works, but I would guess
> module:mojo/public/js/bindings/router~Router. I expect router.js would need to
> be annotated for it to be useful.

Yeah, it would need to be. I was more just wondering out loud because I think it
would be nice to have everything (including mojo JS) annotated. :D

LGTM

[Sam McNally, 2014-10-27 23:33:17]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-27 23:38:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/669303002/110001

[commit-bot: I haz the power, 2014-10-28 01:07:10]

Committed patchset #2 (id:110001)

[commit-bot: I haz the power, 2014-10-28 01:07:55]

Patchset 2 (id:??) landed as
https://crrev.com/d9fddbc5700ebdea8d4643b4d2c2f6f76abd4b30
Cr-Commit-Position: refs/heads/master@{#301528}