Adding the first set of remote test cases and associated framework.

Adding the first set of remote test cases and associated framework.

BUG=443003
Committed: https://crrev.com/1dc627dbbc80589e34284698062108a93a84ced1
Cr-Commit-Position: refs/heads/master@{#311115}

[Mike Meade, 2014-12-17 00:33:35]

mmeade@chromium.org changed reviewers:
+ anandc@chromium.org, chaitali@chromium.org, jamiewalch@chromium.org

[Jamie, 2014-12-17 03:15:41]

I haven't reviewed the Python code; I think others are more qualified than me to
do that. Did you write all the Python code? If not, then it belongs under
third_party/

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/key_cod...
File chrome/test/remoting/key_code_test_map.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/key_cod...
chrome/test/remoting/key_code_test_map.h:26: // Code  KeyboardCode
Indentation is wrong (though you could just remove this comment, since it's not
present in any of the other maps, and it's right next to the KeyCodeTestMap
definition).

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:103:
SimulateKeyPressWithCode(ui::VKEY_SPACE, "Space", false, false, true, false);
None of the other tests do this. Why is it needed here?

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:123: // javascript on the page. This
ensures that the next run will be clean.
Is this needed? Cleanup uninstalls the app, which I assume closes the tab and
the associated context menu.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:154: // Wake up the machine if its
sleeping
nit: s/its/it's/

I have concerns about this step. If the machine is asleep then you need some
sort of wait, since it will take some take to awaken. If it's not asleep, then
this is an additional keyboard event that tests may need to account for (though,
if you're only reporting the most recent, it should be fine).

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:162: LOG(INFO) << "Pressing " << code;
Given that we test all keys, this log will be very spammy. I suggest VLOG
instead.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:174: // right click;
I don't understand why the context menu affects where the click should occur.

Also, comments don't need to end in a semi-colon :)

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:178: LOG(INFO) << "Clicking left mouse
button";
This should probably also be a VLOG, although it will not be as spammy.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:194: ASSERT_EQ(button, event.value);
Can we also verify the location somehow? If that makes the test substantially
more complex, consider moving the mouse test into a separate CL.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:32: :
remote_test_helper_(NULL), extension_(NULL) {
Use nullptr in new code (might as well update |extension_| too, for
consistency).

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:85: remote_test_helper_ =
RemoteTestHelper(client_web_content_);
I don't know what the style guide has to say about this, but I don't recall
seeing this assign-to-object pattern elsewhere in Chromium code. It would be
more usual for the member to be a scoped_ptr, and to use reset() here.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.h:219: }
This returns an implicit copy of the helper, which is probably not what you want
from a maintainability standpoint. It would be better to return a pointer, I
think.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_test_helper.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.cc:39: content::WebContents*
web_content,
Nit: Line-wrap for these parameters looks odd. I think we tend to have the first
parameter on the same line as the method name and wrap other parameters under it
if they don't all fit on one line. This style is only needed if any of the
parameters don't fit with that scheme.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:24: Action action = Action::NONE;
I don't think this form of member initialization is permitted yet. See
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0
for details.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:32: // ctor.
You don't need this comment.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:34: :web_content_(web_content) {}
Since you already have an .cc file for this class, I don't think you gain
anything by inlining this ctor.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:37: void ExecuteRpc(const std::string&
method, int timeout_secs = 10) {
I don't think we're allowed to use default parameters. If we are, then it seems
odd to have a different default for timeout_secs for these two methods.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:49: static void
ClearLastEvent(content::WebContents*);
Do you need any of the methods that take an explicit WebContents? It seems like
we'd always want to use the implicit versions, and it's a shame to pollute the
interface with unnecessary additional methods.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:52: void GetLastEvent(Event& event) {
Please double-check, but I don't think the style guide allows non-const
references to be used as OUT parameters. Either make this function return an
Event (or a scoped_ptr to one) or pass the event by pointer.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:54: }
Nit: indentation.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:57: protected:
It's not clear that this class is intended to be inherited from (no virtual
dtor, for example), so I'm not sure why you've separated out the protected vs.
private members.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:59: static int
ExecuteScriptAndExtractInt(content::WebContents*,
We already have definitions for these in RemoteDesktopBrowserTest. If we need
these versions as well, consider moving them out of the interface, since they're
not public.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:63: const std::string&);
Nit: Indentation.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:65: private:
You would typically have DISALLOW_COPY_AND_ASSIGN here (but you'll need to fix
up the places you're copying and assigning first).

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/SimpleJSONRPCServer.py (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/SimpleJSONRPCServer.py:1: # Copyright (c) 2014
The Chromium Authors. All rights reserved.
Nit: No need for (c), here and below.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/client.html (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/client.html:10: <script
src="/jsonrpc.js"></script>
Is the leading slash needed in this URL?

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/host.js (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/host.js:6: var ACTIONS = {
Call this Action for consistency with the C++ code.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/host.js:7: Error: -1,
Nit: Indentation. Also, the values are not consistent wrt case; for other enums,
we've tended to use UPPER_CASE.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/jsonrpc.js (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/jsonrpc.js:51: //      console.log('Response
text: ' + xhr.responseText);
Remove, or reinstate.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/jsonrpc.js:56: console.log('Could not parse
server response.');
console.error?

[Mike Meade, 2014-12-18 18:54:23]

All of the python code is derived from standard python libraries. No third party
code was used or is needed to run this.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/key_cod...
File chrome/test/remoting/key_code_test_map.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/key_cod...
chrome/test/remoting/key_code_test_map.h:26: // Code  KeyboardCode
On 2014/12/17 03:15:40, Jamie wrote:
> Indentation is wrong (though you could just remove this comment, since it's
not
> present in any of the other maps, and it's right next to the KeyCodeTestMap
> definition).

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:103:
SimulateKeyPressWithCode(ui::VKEY_SPACE, "Space", false, false, true, false);
On 2014/12/17 03:15:40, Jamie wrote:
> None of the other tests do this. Why is it needed here?

Its left over from when I had to wake the machine up during testing. I actually
moved a wake up keystroke to SetupForRemoteHostTest() and forgot to remove it
here.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:123: // javascript on the page. This
ensures that the next run will be clean.
On 2014/12/17 03:15:40, Jamie wrote:
> Is this needed? Cleanup uninstalls the app, which I assume closes the tab and
> the associated context menu.

This is here for when the tests are run manually, which I understand will be the
first way to validate functionality while working on multi-machine
configurations. This way anyone running the test manually won't have the context
menu up for the next run.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:154: // Wake up the machine if its
sleeping
On 2014/12/17 03:15:40, Jamie wrote:
> nit: s/its/it's/
> 
> I have concerns about this step. If the machine is asleep then you need some
> sort of wait, since it will take some take to awaken. If it's not asleep, then
> this is an additional keyboard event that tests may need to account for
(though,
> if you're only reporting the most recent, it should be fine).

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:162: LOG(INFO) << "Pressing " << code;
On 2014/12/17 03:15:40, Jamie wrote:
> Given that we test all keys, this log will be very spammy. I suggest VLOG
> instead.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:178: LOG(INFO) << "Clicking left mouse
button";
On 2014/12/17 03:15:40, Jamie wrote:
> This should probably also be a VLOG, although it will not be as spammy.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:32: :
remote_test_helper_(NULL), extension_(NULL) {
On 2014/12/17 03:15:40, Jamie wrote:
> Use nullptr in new code (might as well update |extension_| too, for
> consistency).

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:85: remote_test_helper_ =
RemoteTestHelper(client_web_content_);
On 2014/12/17 03:15:40, Jamie wrote:
> I don't know what the style guide has to say about this, but I don't recall
> seeing this assign-to-object pattern elsewhere in Chromium code. It would be
> more usual for the member to be a scoped_ptr, and to use reset() here.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.h:219: }
On 2014/12/17 03:15:40, Jamie wrote:
> This returns an implicit copy of the helper, which is probably not what you
want
> from a maintainability standpoint. It would be better to return a pointer, I
> think.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_test_helper.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.cc:39: content::WebContents*
web_content,
On 2014/12/17 03:15:40, Jamie wrote:
> Nit: Line-wrap for these parameters looks odd. I think we tend to have the
first
> parameter on the same line as the method name and wrap other parameters under
it
> if they don't all fit on one line. This style is only needed if any of the
> parameters don't fit with that scheme.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:32: // ctor.
On 2014/12/17 03:15:41, Jamie wrote:
> You don't need this comment.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:34: :web_content_(web_content) {}
On 2014/12/17 03:15:40, Jamie wrote:
> Since you already have an .cc file for this class, I don't think you gain
> anything by inlining this ctor.

I got a lint warning indicating that since the only declared constructor takes a
parameter it needs to be declared explicit.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:37: void ExecuteRpc(const std::string&
method, int timeout_secs = 10) {
On 2014/12/17 03:15:40, Jamie wrote:
> I don't think we're allowed to use default parameters. If we are, then it
seems
> odd to have a different default for timeout_secs for these two methods.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:49: static void
ClearLastEvent(content::WebContents*);
On 2014/12/17 03:15:40, Jamie wrote:
> Do you need any of the methods that take an explicit WebContents? It seems
like
> we'd always want to use the implicit versions, and it's a shame to pollute the
> interface with unnecessary additional methods.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:52: void GetLastEvent(Event& event) {
On 2014/12/17 03:15:40, Jamie wrote:
> Please double-check, but I don't think the style guide allows non-const
> references to be used as OUT parameters. Either make this function return an
> Event (or a scoped_ptr to one) or pass the event by pointer.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:57: protected:
On 2014/12/17 03:15:40, Jamie wrote:
> It's not clear that this class is intended to be inherited from (no virtual
> dtor, for example), so I'm not sure why you've separated out the protected vs.
> private members.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:59: static int
ExecuteScriptAndExtractInt(content::WebContents*,
On 2014/12/17 03:15:40, Jamie wrote:
> We already have definitions for these in RemoteDesktopBrowserTest. If we need
> these versions as well, consider moving them out of the interface, since
they're
> not public.

I moved the static ones from RemoteDesktopBrowserTest to here and changed the
references. The non-static versions are still in RemoteDesktopBrowserTest, they
just reference these methods now.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:63: const std::string&);
On 2014/12/17 03:15:41, Jamie wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:65: private:
On 2014/12/17 03:15:41, Jamie wrote:
> You would typically have DISALLOW_COPY_AND_ASSIGN here (but you'll need to fix
> up the places you're copying and assigning first).

Done.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/SimpleJSONRPCServer.py (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/SimpleJSONRPCServer.py:1: # Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/12/17 03:15:41, Jamie wrote:
> Nit: No need for (c), here and below.

Done.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/client.html (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/client.html:10: <script
src="/jsonrpc.js"></script>
On 2014/12/17 03:15:41, Jamie wrote:
> Is the leading slash needed in this URL?

Done.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/host.js (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/host.js:6: var ACTIONS = {
On 2014/12/17 03:15:41, Jamie wrote:
> Call this Action for consistency with the C++ code.

Done.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/host.js:7: Error: -1,
On 2014/12/17 03:15:41, Jamie wrote:
> Nit: Indentation. Also, the values are not consistent wrt case; for other
enums,
> we've tended to use UPPER_CASE.

Done.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
File remoting/tools/remote_test_helper/jsonrpc.js (right):

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/jsonrpc.js:51: //      console.log('Response
text: ' + xhr.responseText);
On 2014/12/17 03:15:41, Jamie wrote:
> Remove, or reinstate.

Done.

https://codereview.chromium.org/807343002/diff/1/remoting/tools/remote_test_h...
remoting/tools/remote_test_helper/jsonrpc.js:56: console.log('Could not parse
server response.');
On 2014/12/17 03:15:41, Jamie wrote:
> console.error?

Done.

[andrewrs, 2014-12-18 19:10:31]

andrewrs@chromium.org changed reviewers:
+ andrewrs@chromium.org

[andrewrs, 2014-12-18 19:10:32]

lgtm

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/jsonrpclib.py (right):

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpclib.py:120: }
This trailing brace doesn't look quite aligned in the review tool.

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpclib.py:259: 
What do you think about setting Transport.user_agent for these classes?

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpclib.py:270: self.send_content(h,
request_body)
Looking at the default implementation of xmlrpclib, it appears that send_content
is setting a Content-Type: text/xml header. Would you want to override that
behavior to set Content-Type: application/json?

[Jamie, 2014-12-18 21:31:44]

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:123: // javascript on the page. This
ensures that the next run will be clean.
On 2014/12/18 18:54:24, Mike Meade wrote:
> On 2014/12/17 03:15:40, Jamie wrote:
> > Is this needed? Cleanup uninstalls the app, which I assume closes the tab
and
> > the associated context menu.
> 
> This is here for when the tests are run manually, which I understand will be
the
> first way to validate functionality while working on multi-machine
> configurations. This way anyone running the test manually won't have the
context
> menu up for the next run.

I still don't understand; whether it's run manually or automatically, the last
thing this test does is uninstall the app, so it seems redundant to close the
context menu explicitly. What happens if you remove this?

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:154: // Wake up the machine if its
sleeping
On 2014/12/18 18:54:24, Mike Meade wrote:
> On 2014/12/17 03:15:40, Jamie wrote:
> > nit: s/its/it's/
> > 
> > I have concerns about this step. If the machine is asleep then you need some
> > sort of wait, since it will take some take to awaken. If it's not asleep,
then
> > this is an additional keyboard event that tests may need to account for
> (though,
> > if you're only reporting the most recent, it should be fine).
> 
> Done.

Can you address my concerns about what this means in terms of timing and the
additional keypress?

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:174: // right click;
On 2014/12/17 03:15:40, Jamie wrote:
> I don't understand why the context menu affects where the click should occur.
> 
> Also, comments don't need to end in a semi-colon :)

You haven't addressed this comment.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:194: ASSERT_EQ(button, event.value);
On 2014/12/17 03:15:40, Jamie wrote:
> Can we also verify the location somehow? If that makes the test substantially
> more complex, consider moving the mouse test into a separate CL.

You haven't addressed this comment.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:24: Action action = Action::NONE;
On 2014/12/17 03:15:41, Jamie wrote:
> I don't think this form of member initialization is permitted yet. See
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0
> for details.

You haven't addressed this comment.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:34: :web_content_(web_content) {}
On 2014/12/18 18:54:24, Mike Meade wrote:
> On 2014/12/17 03:15:40, Jamie wrote:
> > Since you already have an .cc file for this class, I don't think you gain
> > anything by inlining this ctor.
> 
> I got a lint warning indicating that since the only declared constructor takes
a
> parameter it needs to be declared explicit.

The use of explicit is fine; it's the inlining of the ctor body that I don't
think is good style. In fact, if you had any non-trivial member variables, it
would be disallowed.

https://codereview.chromium.org/807343002/diff/20001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/20001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.h:57: // We do this so we can be used in
a Callback.
If you're sure that the reference to this object will not outlive the object
itself, you can use base::Unretained (which is essentially an explicit,
self-documenting and standard way of achieving the same as you've done here). If
not, you should use a WeakPtr or have this class inherit from RefCounted.

[Mike Meade, 2015-01-05 23:18:58]

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:123: // javascript on the page. This
ensures that the next run will be clean.
On 2014/12/18 21:31:44, Jamie wrote:
> On 2014/12/18 18:54:24, Mike Meade wrote:
> > On 2014/12/17 03:15:40, Jamie wrote:
> > > Is this needed? Cleanup uninstalls the app, which I assume closes the tab
> and
> > > the associated context menu.
> > 
> > This is here for when the tests are run manually, which I understand will be
> the
> > first way to validate functionality while working on multi-machine
> > configurations. This way anyone running the test manually won't have the
> context
> > menu up for the next run.
> 
> I still don't understand; whether it's run manually or automatically, the last
> thing this test does is uninstall the app, so it seems redundant to close the
> context menu explicitly. What happens if you remove this?

The context menu is opened in Chrome on a remote host machine and the test
cannot currently uninstall the app between runs. Once multi-machine support is
added to Omnibot this won't be a problem, but until then we will have a
persistent state between runs to worry about.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:154: // Wake up the machine if its
sleeping
On 2014/12/18 21:31:44, Jamie wrote:
> On 2014/12/18 18:54:24, Mike Meade wrote:
> > On 2014/12/17 03:15:40, Jamie wrote:
> > > nit: s/its/it's/
> > > 
> > > I have concerns about this step. If the machine is asleep then you need
some
> > > sort of wait, since it will take some take to awaken. If it's not asleep,
> then
> > > this is an additional keyboard event that tests may need to account for
> > (though,
> > > if you're only reporting the most recent, it should be fine).
> > 
> > Done.
> 
> Can you address my concerns about what this means in terms of timing and the
> additional keypress?

There are 3 things here
1) The server only tracks the last event that was received
2) The first thing each test does is a reset on the server to remove any old
event. This way we can detect when an event wasn't received, even after
verifying multiple events were received.
3) This was only added because I found when running the tests manually that the
Chrome page may not get the first event if I hadn't run a test for a while. It
seemed to me to need a "wake up" event after a while.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:174: // right click;
On 2014/12/18 21:31:44, Jamie wrote:
> On 2014/12/17 03:15:40, Jamie wrote:
> > I don't understand why the context menu affects where the click should
occur.
> > 
> > Also, comments don't need to end in a semi-colon :)
> 
> You haven't addressed this comment.

I removed this code until we could make it a bit more robust with more
information on the events raised on the remote machine. However, to address your
question, when run manually these tests are run on a persistent, remote machine.
When the context menu is raised its possible for any following events to be
raised in the context menu frame as opposed to the web frame. I offset this key
so that a left or middle click in the standard position would dismiss the
context menu instead of being captured by it.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:194: ASSERT_EQ(button, event.value);
On 2014/12/18 21:31:44, Jamie wrote:
> On 2014/12/17 03:15:40, Jamie wrote:
> > Can we also verify the location somehow? If that makes the test
substantially
> > more complex, consider moving the mouse test into a separate CL.
> 
> You haven't addressed this comment.

I removed this test for now. It seems to deserve its own cl.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:24: Action action = Action::NONE;
On 2014/12/18 21:31:44, Jamie wrote:
> On 2014/12/17 03:15:41, Jamie wrote:
> > I don't think this form of member initialization is permitted yet. See
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0
> > for details.
> 
> You haven't addressed this comment.

It looks like adding an inline constructor is what is being recommended now. I
have changed it to have a simple default constructor.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:34: :web_content_(web_content) {}
On 2014/12/18 21:31:44, Jamie wrote:
> On 2014/12/18 18:54:24, Mike Meade wrote:
> > On 2014/12/17 03:15:40, Jamie wrote:
> > > Since you already have an .cc file for this class, I don't think you gain
> > > anything by inlining this ctor.
> > 
> > I got a lint warning indicating that since the only declared constructor
takes
> a
> > parameter it needs to be declared explicit.
> 
> The use of explicit is fine; it's the inlining of the ctor body that I don't
> think is good style. In fact, if you had any non-trivial member variables, it
> would be disallowed.

Done.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_test_helper.h:54: }
On 2014/12/17 03:15:41, Jamie wrote:
> Nit: indentation.

Done.

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/jsonrpclib.py (right):

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpclib.py:120: }
On 2014/12/18 19:10:32, andrewrs wrote:
> This trailing brace doesn't look quite aligned in the review tool.
Actually this whole block was badly formatted. Thats what I get for letting
Eclipse decide what my indents should be.

Done.

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpclib.py:259: 
On 2014/12/18 19:10:32, andrewrs wrote:
> What do you think about setting Transport.user_agent for these classes?

Done.

https://codereview.chromium.org/807343002/diff/20001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpclib.py:270: self.send_content(h,
request_body)
On 2014/12/18 19:10:32, andrewrs wrote:
> Looking at the default implementation of xmlrpclib, it appears that
send_content
> is setting a Content-Type: text/xml header. Would you want to override that
> behavior to set Content-Type: application/json?

Good catch on that one. Done.

[Jamie, 2015-01-06 19:31:39]

As before, I haven't review the Python code.

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/1/chrome/test/remoting/me2me_b...
chrome/test/remoting/me2me_browsertest.cc:154: // Wake up the machine if its
sleeping
On 2015/01/05 23:18:58, Mike Meade wrote:
> On 2014/12/18 21:31:44, Jamie wrote:
> > On 2014/12/18 18:54:24, Mike Meade wrote:
> > > On 2014/12/17 03:15:40, Jamie wrote:
> > > > nit: s/its/it's/
> > > > 
> > > > I have concerns about this step. If the machine is asleep then you need
> some
> > > > sort of wait, since it will take some take to awaken. If it's not
asleep,
> > then
> > > > this is an additional keyboard event that tests may need to account for
> > > (though,
> > > > if you're only reporting the most recent, it should be fine).
> > > 
> > > Done.
> > 
> > Can you address my concerns about what this means in terms of timing and the
> > additional keypress?
> 
> There are 3 things here
> 1) The server only tracks the last event that was received
> 2) The first thing each test does is a reset on the server to remove any old
> event. This way we can detect when an event wasn't received, even after
> verifying multiple events were received.
> 3) This was only added because I found when running the tests manually that
the
> Chrome page may not get the first event if I hadn't run a test for a while. It
> seemed to me to need a "wake up" event after a while.

Okay, in that case, please remove the reference to "sleeping" in the comment, as
I think it's misleading (I thought you were referring to the OS-level sleep
functionality). Ideally, we should find out what's going on here, otherwise we
could end up with a bug whereby the first event of every session is lost, and
we'd never know. This is better than nothing, but please add a TODO to
investigate the underlying issue.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/key...
File chrome/test/remoting/key_code_test_map.h (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/key...
chrome/test/remoting/key_code_test_map.h:25: const KeyCodeTestMap test_key_map[]
= {
I suggest you call this |test_alpha_map|, since the other maps also contain
"keys".

That said, I'm not sure there's much value in having these in separate arrays.
Why not just one array, with one corresponding test for all keys?

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/me2...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/me2...
chrome/test/remoting/me2me_browsertest.cc:118:
IN_PROC_BROWSER_TEST_F(Me2MeBrowserTest,
Is it possible to add a test for modifier keys as well? Based on past bug
reports, those are the most likely to fail. It can be a follow-up CL.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_test_helper.cc (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:15: :web_content_(web_content) {}
Nit: indentation.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:57: content::ExecuteScript(
This indentation is inconsistent with the _ASSERT_TRUE above and the EXPECT_TRUE
in IsValidEvent, neither of which start a new line after the open-paren (which
is more consistent with the rest of our code, I think).

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:62: // Wait until the client tab sets
the right variables
This comment is a bit vague. If I'm reading the code correctly, I think you're
waiting for |method| to take effect, which you detect via the responseObject
becoming non-null, correct? Something more explicit to that effect would be
useful.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:64: timeout,
Nit: Indentation.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:80: web_content_,
"jsonRpc.responseObject = null"));
ExecuteRpc already does this, so you don't need this line.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:92: base::TimeDelta::FromSeconds(2),
Indentation.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.h:47: int modifiers = 0;
These initializers are no longer needed. Also, the ctor should be the first
struct member, with a blank line separating it from the data members.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.h:54: 
No need for this blank line.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/host.js (right):

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/host.js:7: ERROR: -1,
Indentation.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/jsonrpc.js (right):

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:32: ident = ident == undefined ?
'...' : ident;
Why is this needed? Under what circumstances will a zero ident be provided?

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:46: jsonRpc.sendRpcRequest =
function(json_request) {
I think it would be better to clear the responseObject here, rather than doing
it from C++.

[Mike Meade, 2015-01-08 18:58:43]

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/key...
File chrome/test/remoting/key_code_test_map.h (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/key...
chrome/test/remoting/key_code_test_map.h:25: const KeyCodeTestMap test_key_map[]
= {
On 2015/01/06 19:31:38, Jamie wrote:
> I suggest you call this |test_alpha_map|, since the other maps also contain
> "keys".
> 
> That said, I'm not sure there's much value in having these in separate arrays.
> Why not just one array, with one corresponding test for all keys?

Changed.

The reason for splitting these out is the length of time the tests take. With
all of the characters in a single test the test would get killed before
validating all of the characters. By splitting them out we reduce this time per
test and (hopefully) make a failure a little clearer before starting to dig in
(seeing the digit test fail for example)

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/me2...
File chrome/test/remoting/me2me_browsertest.cc (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/me2...
chrome/test/remoting/me2me_browsertest.cc:118:
IN_PROC_BROWSER_TEST_F(Me2MeBrowserTest,
On 2015/01/06 19:31:38, Jamie wrote:
> Is it possible to add a test for modifier keys as well? Based on past bug
> reports, those are the most likely to fail. It can be a follow-up CL.

I have a few special keys in there now. We can detect the keypress since I used
the onkeydown function, but many of them have side effects in the browser (like
moving focus) that I haven't figured out a good way to get around. I can go back
and see what others can be added and what the roadblocks are for the ones that
can't be easily added.

I have a feeling we would need a system level event listener to get these
without making a test hacky by trying to guess how to get focus back to the
page. The good thing is that it would be very easy to reuse the remote test
server code with something like this, so that the whole thing doesn't need to be
re-engineered.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_test_helper.cc (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:15: :web_content_(web_content) {}
On 2015/01/06 19:31:38, Jamie wrote:
> Nit: indentation.

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:57: content::ExecuteScript(
On 2015/01/06 19:31:39, Jamie wrote:
> This indentation is inconsistent with the _ASSERT_TRUE above and the
EXPECT_TRUE
> in IsValidEvent, neither of which start a new line after the open-paren (which
> is more consistent with the rest of our code, I think).

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:62: // Wait until the client tab sets
the right variables
On 2015/01/06 19:31:38, Jamie wrote:
> This comment is a bit vague. If I'm reading the code correctly, I think you're
> waiting for |method| to take effect, which you detect via the responseObject
> becoming non-null, correct? Something more explicit to that effect would be
> useful.

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:64: timeout,
On 2015/01/06 19:31:38, Jamie wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:80: web_content_,
"jsonRpc.responseObject = null"));
On 2015/01/06 19:31:39, Jamie wrote:
> ExecuteRpc already does this, so you don't need this line.

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.cc:92: base::TimeDelta::FromSeconds(2),
On 2015/01/06 19:31:39, Jamie wrote:
> Indentation.

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_test_helper.h (right):

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.h:47: int modifiers = 0;
On 2015/01/06 19:31:39, Jamie wrote:
> These initializers are no longer needed. Also, the ctor should be the first
> struct member, with a blank line separating it from the data members.

Done.

https://codereview.chromium.org/807343002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_test_helper.h:54: 
On 2015/01/06 19:31:39, Jamie wrote:
> No need for this blank line.

Done.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/host.js (right):

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/host.js:7: ERROR: -1,
On 2015/01/06 19:31:39, Jamie wrote:
> Indentation.

Done.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/jsonrpc.js (right):

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:32: ident = ident == undefined ?
'...' : ident;
On 2015/01/06 19:31:39, Jamie wrote:
> Why is this needed? Under what circumstances will a zero ident be provided?

I wrote this to be a general library module for any test/code to use. I used
'...' as the default parameter for ident, but that shouldn't force other code to
use this as well. Instead of needing to modify this method in later code I just
added the default argument pattern here for the identifier arg.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:46: jsonRpc.sendRpcRequest =
function(json_request) {
On 2015/01/06 19:31:39, Jamie wrote:
> I think it would be better to clear the responseObject here, rather than doing
> it from C++.

Done.

[Jamie, 2015-01-08 20:34:37]

LGTM with nits. If you decide to run jscompile over it as part of this CL, then
I'll take another look.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/jsonrpc.js (right):

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:28: * @param {integer} ident The
request id.
{integer} is not a valid type annotation. I think you mean {number}.

I don't know if there's any value in running jscompile over this, but now that
jscompile is integrated into the Chromium build process, it shouldn't be too
difficult. Since this is library code (and therefore not expected to be
exercised in full by any given test), I think it would be worthwhile. It can be
a follow-up CL though.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:32: ident = ident == undefined ?
'...' : ident;
On 2015/01/08 18:58:42, Mike Meade wrote:
> On 2015/01/06 19:31:39, Jamie wrote:
> > Why is this needed? Under what circumstances will a zero ident be provided?
> 
> I wrote this to be a general library module for any test/code to use. I used
> '...' as the default parameter for ident, but that shouldn't force other code
to
> use this as well. Instead of needing to modify this method in later code I
just
> added the default argument pattern here for the identifier arg.

I think the problem I have with it is that you've declared |ident| as being a
number, but are assigning a string default value to it. That's not prohibited by
JS, but I think it leads to confusing code. I don't see any problem in requiring
an id to be supplied, even for generic library code, but if you do want to keep
it optional, you should do the following:

* Make it explicit by calling it opt_ident and making its type {number=}
* Use === instead of == for comparing it to undefined (== does type conversion
so that, for example "null==undefined" will evaluate to true).
* Consider using 0 (or -1) as a default, instead of '...'

[Mike Meade, 2015-01-12 18:03:53]

The CQ bit was checked by mmeade@chromium.org

[Mike Meade, 2015-01-12 18:04:26]

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
File remoting/tools/remote_test_helper/jsonrpc.js (right):

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:28: * @param {integer} ident The
request id.
On 2015/01/08 20:34:37, Jamie wrote:
> {integer} is not a valid type annotation. I think you mean {number}.
> 
> I don't know if there's any value in running jscompile over this, but now that
> jscompile is integrated into the Chromium build process, it shouldn't be too
> difficult. Since this is library code (and therefore not expected to be
> exercised in full by any given test), I think it would be worthwhile. It can
be
> a follow-up CL though.

I fixed this to be {number=} and assigned a default value of 0. I also changed
the name to opt_ident.

https://codereview.chromium.org/807343002/diff/60001/remoting/tools/remote_te...
remoting/tools/remote_test_helper/jsonrpc.js:32: ident = ident == undefined ?
'...' : ident;
On 2015/01/08 20:34:37, Jamie wrote:
> On 2015/01/08 18:58:42, Mike Meade wrote:
> > On 2015/01/06 19:31:39, Jamie wrote:
> > > Why is this needed? Under what circumstances will a zero ident be
provided?
> > 
> > I wrote this to be a general library module for any test/code to use. I used
> > '...' as the default parameter for ident, but that shouldn't force other
code
> to
> > use this as well. Instead of needing to modify this method in later code I
> just
> > added the default argument pattern here for the identifier arg.
> 
> I think the problem I have with it is that you've declared |ident| as being a
> number, but are assigning a string default value to it. That's not prohibited
by
> JS, but I think it leads to confusing code. I don't see any problem in
requiring
> an id to be supplied, even for generic library code, but if you do want to
keep
> it optional, you should do the following:
> 
> * Make it explicit by calling it opt_ident and making its type {number=}
> * Use === instead of == for comparing it to undefined (== does type conversion
> so that, for example "null==undefined" will evaluate to true).
> * Consider using 0 (or -1) as a default, instead of '...'

Done.

[commit-bot: I haz the power, 2015-01-12 18:05:13]

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

[commit-bot: I haz the power, 2015-01-12 18:42:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-12 18:42:15]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10...)

[Mike Meade, 2015-01-12 21:03:23]

The CQ bit was checked by mmeade@chromium.org

[commit-bot: I haz the power, 2015-01-12 21:04:00]

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

[commit-bot: I haz the power, 2015-01-12 21:55:23]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-01-12 21:56:16]

Patchset 13 (id:??) landed as
https://crrev.com/1dc627dbbc80589e34284698062108a93a84ced1
Cr-Commit-Position: refs/heads/master@{#311115}