Next phase for chrome.networkingPrivate interface.

Next phase for chrome.networkingPrivate interface.

This splits the network changed event into two separate events
and changes the events so that they report only the identifiers for
the networks, and not their properties.

Also fixed a bad bug in startDisconnect (was calling connect instead
of disconnect).

Modified the apitest so that we now run each test as a separate
browser test so that the networking stubs are reset between
tests.

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

[Greg Spencer (Chromium), 2013-02-12 03:37:11]

Ben, since this is almost completely extension code, could you please review
this?

Steven, could you please give me a review for:
chromeos/dbus/shill_service_client.cc
chrome/browser/chromeos/cros/network_library_impl_stub.cc

Thanks!

[not at google - send to devlin, 2013-02-12 04:11:41]

I won't be able to review this until tomorrow but if you want to tbr the
disconnect->connect fix before branch, that's cool.

[stevenjb, 2013-02-12 17:19:16]

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:40: }
I'm not a big fan of macros, and thought the style guide prohibits it? I'd
rather see these expanded out.

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/networking_private_event_router.cc
(right):

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:135: //
this to be the real GUID once we're using the
nit: s/using the/using/

https://chromiumcodereview.appspot.com/12220113/diff/1/chromeos/dbus/shill_se...
File chromeos/dbus/shill_service_client.cc (right):

https://chromiumcodereview.appspot.com/12220113/diff/1/chromeos/dbus/shill_se...
chromeos/dbus/shill_service_client.cc:261: service_path.value(), 0, true);
I'd like to avoid cross dependencies between these client stubs, or at least
talk through how we want these stubs to evolve before adding more complex
behaviors like this.

[Greg Spencer (Chromium), 2013-02-12 17:34:56]

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:40: }
On 2013/02/12 17:19:16, stevenjb (chromium) wrote:
> I'm not a big fan of macros, and thought the style guide prohibits it? I'd
> rather see these expanded out.

It does not prohibit it, it merely warns about the pitfalls, and I'm following
all of the suggested guidelines here. I mainly wanted it for the stringification
of the name so that I can easily build meaningful names for the tests, add new
ones easily and avoid cut-paste errors when adding new ones.

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/networking_private_event_router.cc
(right):

https://chromiumcodereview.appspot.com/12220113/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:135: //
this to be the real GUID once we're using the
On 2013/02/12 17:19:16, stevenjb (chromium) wrote:
> nit: s/using the/using/

Done.

https://chromiumcodereview.appspot.com/12220113/diff/1/chromeos/dbus/shill_se...
File chromeos/dbus/shill_service_client.cc (right):

https://chromiumcodereview.appspot.com/12220113/diff/1/chromeos/dbus/shill_se...
chromeos/dbus/shill_service_client.cc:261: service_path.value(), 0, true);
On 2013/02/12 17:19:16, stevenjb (chromium) wrote:
> I'd like to avoid cross dependencies between these client stubs, or at least
> talk through how we want these stubs to evolve before adding more complex
> behaviors like this.

OK, but the reason for this is that I'm unable to test the network list change
event without connect or disconnect affecting it, since those are the only
methods I have so far to change state (and when we run with Shill, connecting
does change the order).

[not at google - send to devlin, 2013-02-12 22:07:25]

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:49:
DO_NETWORKING_SUBTEST(OnNetworkListChangedEvent);
Another problem with macros is that this makes the tests harder to disable.
Making these DISABLED_StartConnect (for example) would work but that'd mess up
the subtest name thing.

It also makes test names harder to grep for. I think this point is sometimes
undervalued...

It's also just unprecedented from what I've seen.

Being explicit doesn't need to be that more verbose, what's so wrong with

bool RunSubtest(const std::string& subtest) {
  return RunExtensionSubtest(...)
}

IN_PROC_BROWSER_TEST_F(ExtensionNetworkingPrivateApiTest, StartConnect) {
  RunSubtest("startConnect");
}
// copypaste

??

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:51: #undef
DO_NETWORKING_SUBTEST
kind of unnecessary since the #define is only per-file.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
File chrome/browser/chromeos/extensions/networking_private_event_router.cc
(right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:109:
ExtensionSystem::Get(profile_)->event_router()->
save reference to the event router rather than grabbing it twice?

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:138:
change_str += (*iter)->path() + " ";
change_str doesn't appear to be used?

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:143:
extensions::event_names::kOnNetworkListChanged, args.Pass()));
why does OnNetworkListChanged take OnNetworksChange args and vice versa?
Seems... like it should be the other way around.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:157:
scoped_ptr<base::ListValue> args(api::OnNetworkListChanged::Create(changes));
just

scoped_ptr<base::ListValue> args(
    api::OnNetworkListChanged(std::vector<std::string>(1, network->path()));

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:159:
extensions::event_names::kOnNetworksChanged, args.Pass()));
see above comment

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/common/exten...
File chrome/common/extensions/api/networking_private.json (right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/common/exten...
chrome/common/extensions/api/networking_private.json:107: "description": "Fired
when the properties change on any of the networks.  Sends a list of identifiers
for networks whose properties have changed.",
What is the expected behaviour here? Will the extension just turn around and
request the current properties again? In which case... what you had before seems
better. Or maybe just send down the properties that changed, if that's useful.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:10: function
connectListener_(network, done) {
why the trailing _?

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:13: var finishTest =
function() {
just 2 space indent

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:14:
chrome.networkingPrivate.onNetworksChanged.removeListener(
just 2 space indent

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:17: }
semicolon after all of these

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:29: if (testingConnect)
I don't understand what this testingConnect is supposed to be doing. What does
the false value mean?

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:40: }
semicolon, 2 space indent after all of these

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:53: function
listListener_(network, expected, done) {
ditto all the stuff above.

Actually they apply to the whole file.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:191: var testToRun =
window.location.hash.substring(1);
query string seems more idiomatic but it's also more verbose I suppose. Eh,
whatever.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:192:
chrome.test.runTests([window[testToRun]]);
prefer to put the tests on a separate object rather than window[testToRun].

var tests = {
  getVisibleNetworksWifi: function() {
  },

  ...
};

would be nice.

[Greg Spencer (Chromium), 2013-02-12 23:11:01]

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:49:
DO_NETWORKING_SUBTEST(OnNetworkListChangedEvent);
On 2013/02/12 22:07:25, kalman wrote:
> Another problem with macros is that this makes the tests harder to disable.
> Making these DISABLED_StartConnect (for example) would work but that'd mess up
> the subtest name thing.
> 
> It also makes test names harder to grep for. I think this point is sometimes
> undervalued...
> 
> It's also just unprecedented from what I've seen.
> 
> Being explicit doesn't need to be that more verbose, what's so wrong with
> 
> bool RunSubtest(const std::string& subtest) {
>   return RunExtensionSubtest(...)
> }
> 
> IN_PROC_BROWSER_TEST_F(ExtensionNetworkingPrivateApiTest, StartConnect) {
>   RunSubtest("startConnect");
> }
> // copypaste
> 
> ??

OK, seems consensus is against me here, and the point about disabling the test
is a good one. Expanded these out and removed the macro.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
File chrome/browser/chromeos/extensions/networking_private_event_router.cc
(right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:109:
ExtensionSystem::Get(profile_)->event_router()->
On 2013/02/12 22:07:25, kalman wrote:
> save reference to the event router rather than grabbing it twice?

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:138:
change_str += (*iter)->path() + " ";
On 2013/02/12 22:07:25, kalman wrote:
> change_str doesn't appear to be used?

Removed.  That was for some debugging output that's no longer there.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:143:
extensions::event_names::kOnNetworkListChanged, args.Pass()));
On 2013/02/12 22:07:25, kalman wrote:
> why does OnNetworkListChanged take OnNetworksChange args and vice versa?
> Seems... like it should be the other way around.

Whoa.  I guess this worked because in the end they have the same argument types.
Thanks for catching that.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:157:
scoped_ptr<base::ListValue> args(api::OnNetworkListChanged::Create(changes));
On 2013/02/12 22:07:25, kalman wrote:
> just
> 
> scoped_ptr<base::ListValue> args(
>     api::OnNetworkListChanged(std::vector<std::string>(1, network->path()));

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/browser/chro...
chrome/browser/chromeos/extensions/networking_private_event_router.cc:159:
extensions::event_names::kOnNetworksChanged, args.Pass()));
On 2013/02/12 22:07:25, kalman wrote:
> see above comment

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/common/exten...
File chrome/common/extensions/api/networking_private.json (right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/common/exten...
chrome/common/extensions/api/networking_private.json:107: "description": "Fired
when the properties change on any of the networks.  Sends a list of identifiers
for networks whose properties have changed.",
On 2013/02/12 22:07:25, kalman wrote:
> What is the expected behaviour here? Will the extension just turn around and
> request the current properties again? In which case... what you had before
seems
> better. Or maybe just send down the properties that changed, if that's useful.

Yes, often it will, but it is expected that there will be some consumers who
don't need to know the actual property changes, but rather just that any network
changed, or a particular network changed.  Or they want to know what the
property changes are, but they're only interested in properties for a particular
network or subset of networks.

Since network properties can be pretty large nested objects, it seems better to
only request the property values when they're needed.

Sending deltas would require us to keep old values around on both the C++ and JS
side, and to diff nested dictionaries, and applying deltas on the js side can be
complicated and error prone, so we'd rather avoid that.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:10: function
connectListener_(network, done) {
On 2013/02/12 22:07:25, kalman wrote:
> why the trailing _?

Because I was trying to indicate that these were "private" helpers.  I suppose I
could put them in a namespace instead.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:13: var finishTest =
function() {
On 2013/02/12 22:07:25, kalman wrote:
> just 2 space indent

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:14:
chrome.networkingPrivate.onNetworksChanged.removeListener(
On 2013/02/12 22:07:25, kalman wrote:
> just 2 space indent

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:17: }
On 2013/02/12 22:07:25, kalman wrote:
> semicolon after all of these

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:29: if (testingConnect)
On 2013/02/12 22:07:25, kalman wrote:
> I don't understand what this testingConnect is supposed to be doing. What does
> the false value mean?

I'll see if I can change the name to something more descriptive.  It's meant to
indicate that we're testing the "connect" transition as opposed to the
"disconnect" transition.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:40: }
On 2013/02/12 22:07:25, kalman wrote:
> semicolon, 2 space indent after all of these

Done.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:191: var testToRun =
window.location.hash.substring(1);
On 2013/02/12 22:07:25, kalman wrote:
> query string seems more idiomatic but it's also more verbose I suppose. Eh,
> whatever.

Not really.  I can switch to query string if you want.  Hash just seemed simple.

https://chromiumcodereview.appspot.com/12220113/diff/3003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/networking/test.js:192:
chrome.test.runTests([window[testToRun]]);
On 2013/02/12 22:07:25, kalman wrote:
> prefer to put the tests on a separate object rather than window[testToRun].
> 
> var tests = {
>   getVisibleNetworksWifi: function() {
>   },
> 
>   ...
> };
> 
> would be nice.

Yeah, I'd prefer that too, and started off with that, but the problem is that
then when a test fails, it prints "undefined" as the function name for the
failing test (or even when it succeeds).

I also tried:

var tests = {
  getVisibleNetworksWifi: function getVisibleNetworksWifi(){
  },
  ...
};

And that solved the name problem, but now you have to type it twice on the same
line, which seemed dumb.  Using window[testToRun] seemed the lesser evil.

[not at google - send to devlin, 2013-02-12 23:36:08]

lgtm with some more nits

https://codereview.chromium.org/12220113/diff/3003/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/12220113/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/networking/test.js:191: var testToRun =
window.location.hash.substring(1);
On 2013/02/12 23:11:02, Greg Spencer (Chromium) wrote:
> On 2013/02/12 22:07:25, kalman wrote:
> > query string seems more idiomatic but it's also more verbose I suppose. Eh,
> > whatever.
> 
> Not really.  I can switch to query string if you want.  Hash just seemed
simple.
> 

nah never mind.

https://codereview.chromium.org/12220113/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/networking/test.js:192:
chrome.test.runTests([window[testToRun]]);
On 2013/02/12 23:11:02, Greg Spencer (Chromium) wrote:
> On 2013/02/12 22:07:25, kalman wrote:
> > prefer to put the tests on a separate object rather than window[testToRun].
> > 
> > var tests = {
> >   getVisibleNetworksWifi: function() {
> >   },
> > 
> >   ...
> > };
> > 
> > would be nice.
> 
> Yeah, I'd prefer that too, and started off with that, but the problem is that
> then when a test fails, it prints "undefined" as the function name for the
> failing test (or even when it succeeds).
> 
> I also tried:
> 
> var tests = {
>   getVisibleNetworksWifi: function getVisibleNetworksWifi(){
>   },
>   ...
> };
> 
> And that solved the name problem, but now you have to type it twice on the
same
> line, which seemed dumb.  Using window[testToRun] seemed the lesser evil.

I think that window[testToRun] is the greater evil :)

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:36:
RunNetworkingSubtest("startConnect");
The way you have it failures won't be on the line that they happen, they'll
always be on line 25. I guess here it doesn't matter so much since each test is
only 1 line so you'll be able to tell... but
EXPECT_TRUE(RunNetworkingSubtest(..)) would generally be better (particularly if
any of these tests get more complicated).

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/event_names.cc (right):

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/event_names.cc:101:
"networkingPrivate.onNetworkListChanged";
seems like these could actually fit on 1 line each.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:10: var privateHelpers =
{
I think that if the tests were in a "tests" object then this would be moot.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:13: var
collectProperties = function(testingConnectTransition, properties) {
Ok, I'd make it a string (connect vs disconnect) but whatever.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:41: }
semicolon

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:46: }
semicolon

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:52: }
semicolon

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:61: }
semicolon

[Greg Spencer (Chromium), 2013-02-12 23:57:42]

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:36:
RunNetworkingSubtest("startConnect");
On 2013/02/12 23:36:09, kalman wrote:
> The way you have it failures won't be on the line that they happen, they'll
> always be on line 25. I guess here it doesn't matter so much since each test
is
> only 1 line so you'll be able to tell... but
> EXPECT_TRUE(RunNetworkingSubtest(..)) would generally be better (particularly
if
> any of these tests get more complicated).

Good point.  Fixed.

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/event_names.cc (right):

https://codereview.chromium.org/12220113/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/event_names.cc:101:
"networkingPrivate.onNetworkListChanged";
On 2013/02/12 23:36:09, kalman wrote:
> seems like these could actually fit on 1 line each.

Done.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:13: var
collectProperties = function(testingConnectTransition, properties) {
On 2013/02/12 23:36:09, kalman wrote:
> Ok, I'd make it a string (connect vs disconnect) but whatever.

That's a nice solution.  I'll do that.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:41: }
On 2013/02/12 23:36:09, kalman wrote:
> semicolon

Done.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:46: }
On 2013/02/12 23:36:09, kalman wrote:
> semicolon

Done.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:52: }
On 2013/02/12 23:36:09, kalman wrote:
> semicolon

Done.

https://codereview.chromium.org/12220113/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/networking/test.js:61: }
On 2013/02/12 23:36:09, kalman wrote:
> semicolon

Done.

[stevenjb, 2013-02-13 00:39:13]

OK, I thought about this some, and I can't think of a better way to do this.

However, I would like to put the stubs into their own files. We dont have to do
this here, but would you mind filing an issue for that? Thanks.

lgtm

[Greg Spencer (Chromium), 2013-02-13 00:51:16]

On 2013/02/13 00:39:13, stevenjb (chromium) wrote:
> OK, I thought about this some, and I can't think of a better way to do this.
> 
> However, I would like to put the stubs into their own files. We dont have to
do
> this here, but would you mind filing an issue for that? Thanks.
> 
> lgtm

Agreed.  Bug filed: http://crbug.com/175891

[commit-bot: I haz the power, 2013-02-13 00:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/12220113/5004

[commit-bot: I haz the power, 2013-02-13 00:53:04]

Presubmit check for 12220113-5004 failed and returned exit status 1.

INFO:root:Found 12 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/cros/network_library_impl_stub.cc:465
    if (network->is_behind_portal_for_testing())

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/test/data/extensions/api_test/networking/test.js

Presubmit checks took 3.4s to calculate.

[commit-bot: I haz the power, 2013-02-13 00:59:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/12220113/9003

[commit-bot: I haz the power, 2013-02-13 06:28:23]

Change committed as 182143