Allows client to access auth token from command line specified file.

Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory.

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines.
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency.

BUG=610398
Committed: https://crrev.com/f16b3ac745768029718fbbc7cd288453677e49e8
Cr-Commit-Position: refs/heads/master@{#393890}

[CJ, 2016-05-10 00:48:09]

lethalantidote@chromium.org changed reviewers:
+ marshallk@chromium.org

[CJ, 2016-05-10 00:48:09]

+marshallk For initial review

[Kevin M, 2016-05-10 00:56:23]

Description was changed from

==========
This change allows for the blimp client to access the client auth token
through command line flags instead of being hardcoded

marshallk

BUG=610398
==========

to

==========
This change allows for the blimp client to access the client auth token
through command line flags instead of being hardcoded

BUG=610398
==========

[Kevin M, 2016-05-10 00:56:23]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org
- marshallk@chromium.org

[Kevin M, 2016-05-10 18:06:35]

A good start so far! LMK if you have questions about any of these feedback
items.

Some Chromium CL etiquette:
* The first line of the CL should be a succinct summary of the change - note
that the original message got truncated @ 72 chars, making the subject line
unusable. The recommended format goes something like this:

<short and sweet summary>

<extended description of solution>
<description of problem being solved>

Other changes:
* Fixed typo in abstract_factory_proxy_singleton_context_getter.h.
* ...

BUG=<bug ID>

These CLs are all open source and public, so it pays to be thorough with
comments and descriptions. We don't want to assume that other readers possess
the same level of context that we do.

* Apply correct capitalization and punctuation in CL descriptions.
* Note all miscellaneous supporting changes that aren't covered by your summary
as bulletpoints.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
File blimp/client/session/assignment_source.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:105: assignment.client_token =
GetClientToken(*cmd_line);
You can just pass the result of ForCurrentProcess() inline vs. getting a
variable, since the reference to "CommandLine" makes it obvious what we're
getting.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:105: assignment.client_token =
GetClientToken(*cmd_line);
Add newline before this line, to visually separate the client token getter from
the variables in the header.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:105: assignment.client_token =
GetClientToken(*cmd_line);
What's the behavior if the token isn't specified?

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:109:
base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
You can replace all calls to base::CommandLine::ForCurrentProcess() with
"cmd_line", since we've already stored the result.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:87:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
This variable doesn't exist?

You can compile the client w/o a device, since this error would be caught by the
compiler.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:203: auto cmd_line =
CreateCommandLine(all_filepath_switches_);
We should be consistent with how we're manipulating commandline switches. Add
another call to AppendSwitchASCII() instead.

(ditto for TestSSLAlternateEndpointSuccess)

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:207: assignment.client_token
= GetClientToken(cmd_line);
Set the expectations directly here; otherwise we conflate the expected value
with the actual value.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
File blimp/common/fake_commandline.h (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
We should use the token testdata file, so this helper library won't be
necessary.

I realize a lot of this is refactored from the engine config test, but I'll go
ahead and comment on issues I've seen in this file for educational purposes. No
need to address actually them if you are rm'ing this file.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:19: void CreateFileForSwitch(const std::string&
filepath_switch,
We don't need to create a tmpfile (or have any supporting functions for it) if
we just use blimp/test/data/test_client_token .

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:26: base::CommandLine CreateCommandLine(
This isn't necessary; look at the existing unit tests to see how they manipulate
the current command line.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:31: const std::vector<std::string>
all_filepath_switches_ =
1. Non-POD (Plain Old Data) constants are disallowed.
2. Please refer to the style guide for constant naming conventions. In this
case, we use kConstant.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:32: {engine::kClientTokenPath};
Brace initialization, while cool, isn't allowed yet. Chromium must be
compileable on a wide range of compilers, and compatibility for this mode of
initialization isn't verified to be at 100% just yet.

See https://chromium-cpp.appspot.com/ , "Uniform Initialization Syntax".

This isn't being caught by the compiler because this target is compiled by Linux
builders only, but if this happened to be built by MSVC, the buildbot would fail
the CL.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
File blimp/common/get_client_token.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
blimp/common/get_client_token.cc:9: #include "blimp/engine/app/switches.h"
Consider promote shared switches to something under "common"; we shouldn't have
cycles in our dependency graph.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
blimp/common/get_client_token.cc:13: std::string GetClientToken(const
base::CommandLine& cmd_line) {
Existing test code already manipulates the current process' command line, so
taking it as a parameter is an unnecessary burden on callers. Recommend making
this function no-arg and having it just get the command line object itself.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
File blimp/common/get_client_token.h (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
blimp/common/get_client_token.h:13: namespace blimp {
newline after open brace

https://codereview.chromium.org/1958033003/diff/1/blimp/engine/app/blimp_engi...
File blimp/engine/app/blimp_engine_config_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_config_unittest.cc:15: #include
"blimp/common/fake_commandline.h"
Not used?

https://codereview.chromium.org/1958033003/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_config_unittest.cc:37: auto cmd_line =
CreateCommandLine(all_filepath_switches_);
As in assignment_source_unittest, reference the test data client token path.

[CJ, 2016-05-10 20:58:07]

#For practice.
Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
File blimp/client/session/assignment_source.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:105: assignment.client_token =
GetClientToken(*cmd_line);
On 2016/05/10 18:06:34, Kevin M wrote:
> Add newline before this line, to visually separate the client token getter
from
> the variables in the header.

Done.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:105: assignment.client_token =
GetClientToken(*cmd_line);
On 2016/05/10 18:06:34, Kevin M wrote:
> What's the behavior if the token isn't specified?

Adding an assert here such that client_token does not equal "". This is similar
to how the server seems to handle this. Please let me know if there is a better
way.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:105: assignment.client_token =
GetClientToken(*cmd_line);
On 2016/05/10 18:06:34, Kevin M wrote:
> You can just pass the result of ForCurrentProcess() inline vs. getting a
> variable, since the reference to "CommandLine" makes it obvious what we're
> getting.

Keeping it, since in a later comment, you suggest reuse.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source.cc:109:
base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
On 2016/05/10 18:06:34, Kevin M wrote:
> You can replace all calls to base::CommandLine::ForCurrentProcess() with
> "cmd_line", since we've already stored the result.

Done.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:87:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
It exists in fake_commandline.h. Too obscured away?
If this is supposed to break, then I'm afraid I'm doing something wrong
compiling.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:203: auto cmd_line =
CreateCommandLine(all_filepath_switches_);
On 2016/05/10 18:06:34, Kevin M wrote:
> We should be consistent with how we're manipulating commandline switches. Add
> another call to AppendSwitchASCII() instead.
> 
> (ditto for TestSSLAlternateEndpointSuccess)

Is this similar to what is done in assignment_source.cc? I was trying to
maintain consistency between this and the server tests.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:207: assignment.client_token
= GetClientToken(cmd_line);
On 2016/05/10 18:06:34, Kevin M wrote:
> Set the expectations directly here; otherwise we conflate the expected value
> with the actual value.

I'm not sure what you mean by this. What do you mean by setting expectations
directly.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
File blimp/common/fake_commandline.h (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/05/10 18:06:35, Kevin M wrote:
> We should use the token testdata file, so this helper library won't be
> necessary.
> 
> I realize a lot of this is refactored from the engine config test, but I'll go
> ahead and comment on issues I've seen in this file for educational purposes.
No
> need to address actually them if you are rm'ing this file.

So, I should make the server test look more like the client tests, and have them
both take in blimp/test/data/test_client_token?

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
File blimp/common/get_client_token.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
blimp/common/get_client_token.cc:9: #include "blimp/engine/app/switches.h"
On 2016/05/10 18:06:35, Kevin M wrote:
> Consider promote shared switches to something under "common"; we shouldn't
have
> cycles in our dependency graph.

Done.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
blimp/common/get_client_token.cc:13: std::string GetClientToken(const
base::CommandLine& cmd_line) {
On 2016/05/10 18:06:35, Kevin M wrote:
> Existing test code already manipulates the current process' command line, so
> taking it as a parameter is an unnecessary burden on callers. Recommend making
> this function no-arg and having it just get the command line object itself.

Although I agree, would this have any impact on the ease of testability? Just
wondering before I go and change it.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
File blimp/common/get_client_token.h (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/get_client_tok...
blimp/common/get_client_token.h:13: namespace blimp {
On 2016/05/10 18:06:35, Kevin M wrote:
> newline after open brace

Done.

https://codereview.chromium.org/1958033003/diff/1/blimp/engine/app/blimp_engi...
File blimp/engine/app/blimp_engine_config_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_config_unittest.cc:15: #include
"blimp/common/fake_commandline.h"
On 2016/05/10 18:06:35, Kevin M wrote:
> Not used?

Was being used for CreateCommandLine, but since we are going a different
direction with this, I guess it will be unused. Just to recap, we are going to
test this in the style similar to that found in the client's tests. The current
implementation was reusing code from the server's tests.

https://codereview.chromium.org/1958033003/diff/1/blimp/engine/app/blimp_engi...
blimp/engine/app/blimp_engine_config_unittest.cc:37: auto cmd_line =
CreateCommandLine(all_filepath_switches_);
On 2016/05/10 18:06:35, Kevin M wrote:
> As in assignment_source_unittest, reference the test data client token path.

Acknowledged.

[Kevin M, 2016-05-11 00:39:57]

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:87:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2016/05/10 20:58:06, lethalantidote wrote:
> It exists in fake_commandline.h. Too obscured away?
> If this is supposed to break, then I'm afraid I'm doing something wrong
> compiling.

Ah, I thought that test_dir_ was a private class field (implied by the trailing
underscore).

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:203: auto cmd_line =
CreateCommandLine(all_filepath_switches_);
On 2016/05/10 20:58:06, lethalantidote wrote:
> On 2016/05/10 18:06:34, Kevin M wrote:
> > We should be consistent with how we're manipulating commandline switches.
Add
> > another call to AppendSwitchASCII() instead.
> > 
> > (ditto for TestSSLAlternateEndpointSuccess)
> 
> Is this similar to what is done in assignment_source.cc? I was trying to
> maintain consistency between this and the server tests. 

Check out line 210 of this patch, which mutates the current process' current
command line w/arguments.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:207: assignment.client_token
= GetClientToken(cmd_line);
On 2016/05/10 20:58:06, lethalantidote wrote:
> On 2016/05/10 18:06:34, Kevin M wrote:
> > Set the expectations directly here; otherwise we conflate the expected value
> > with the actual value.
> 
> I'm not sure what you mean by this. What do you mean by setting expectations
> directly.

In this patch, we're testing GetClientToken() by indirectly comparing the output
of GetClientToken against the output of... itself.

It'd be more effective if we compared it to a known fixed string constant.

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
File blimp/common/fake_commandline.h (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/common/fake_commandli...
blimp/common/fake_commandline.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/05/10 20:58:06, lethalantidote wrote:
> On 2016/05/10 18:06:35, Kevin M wrote:
> > We should use the token testdata file, so this helper library won't be
> > necessary.
> > 
> > I realize a lot of this is refactored from the engine config test, but I'll
go
> > ahead and comment on issues I've seen in this file for educational purposes.
> No
> > need to address actually them if you are rm'ing this file.
> 
> So, I should make the server test look more like the client tests, and have
them
> both take in blimp/test/data/test_client_token?

Yeah.

https://codereview.chromium.org/1958033003/diff/20001/blimp/client/session/as...
File blimp/client/session/assignment_source.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/client/session/as...
blimp/client/session/assignment_source.cc:107: CHECK_NE(assignment.client_token,
"");
suggestion:

CHECK(!client_token.empty()) << "No client token provided.";

https://codereview.chromium.org/1958033003/diff/20001/blimp/client/session/as...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/client/session/as...
blimp/client/session/assignment_source_unittest.cc:87:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
See what we're doing with kCertRelativePath to get an idea of how we can use the
test_client_token file.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/BUILD.gn#n...
blimp/common/BUILD.gn:38: source_set("app_switches") {
"switches"

Does this need to be a standalone target, or can it be rolled in to "common"?

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.cc
File blimp/common/switches.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.c...
blimp/common/switches.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
2016

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h
File blimp/common/switches.h (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h...
blimp/common/switches.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
2016

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h...
blimp/common/switches.h:9: namespace engine {
The "engine" namespace doesn't belong here.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h...
blimp/common/switches.h:14: // The port to listen to for incoming TCP
connections.
A listen port is engine-specific, so it shouldn't be under common/.

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:71: "//blimp/common:app_switches",
Remove "app_switches" and just use "common" top-level instead

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_config.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_config.cc:17: #include "blimp/common/switches.h"
Do we need to include this file, now that GetClientToken() is responsible for
accessing the switch?

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_screen_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_screen_unittest.cc:16: #include
"blimp/common/switches.h"
Not needed; no switches are referenced in this file.

[Kevin M, 2016-05-11 00:41:08]

New CL subject and body text look good to me, go ahead and update the CL fields
with them.

[CJ, 2016-05-11 00:42:32]

Description was changed from

==========
This change allows for the blimp client to access the client auth token
through command line flags instead of being hardcoded

BUG=610398
==========

to

==========
This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

[CJ, 2016-05-11 21:49:30]

Addresses marshallk's comments on assignment_source_unittest.cc.

Feedback on this file may change approach on feedback from other files.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:87:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2016/05/11 00:39:56, Kevin M wrote:
> On 2016/05/10 20:58:06, lethalantidote wrote:
> > It exists in fake_commandline.h. Too obscured away?
> > If this is supposed to break, then I'm afraid I'm doing something wrong
> > compiling.
> 
> Ah, I thought that test_dir_ was a private class field (implied by the
trailing
> underscore).

It got removed because it is unneeded in the preferred way to manipulate the
commandline.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:203: auto cmd_line =
CreateCommandLine(all_filepath_switches_);
On 2016/05/11 00:39:56, Kevin M wrote:
> On 2016/05/10 20:58:06, lethalantidote wrote:
> > On 2016/05/10 18:06:34, Kevin M wrote:
> > > We should be consistent with how we're manipulating commandline switches.
> Add
> > > another call to AppendSwitchASCII() instead.
> > > 
> > > (ditto for TestSSLAlternateEndpointSuccess)
> > 
> > Is this similar to what is done in assignment_source.cc? I was trying to
> > maintain consistency between this and the server tests. 
> 
> Check out line 210 of this patch, which mutates the current process' current
> command line w/arguments.

Done.

https://codereview.chromium.org/1958033003/diff/1/blimp/client/session/assign...
blimp/client/session/assignment_source_unittest.cc:207: assignment.client_token
= GetClientToken(cmd_line);
On 2016/05/11 00:39:56, Kevin M wrote:
> On 2016/05/10 20:58:06, lethalantidote wrote:
> > On 2016/05/10 18:06:34, Kevin M wrote:
> > > Set the expectations directly here; otherwise we conflate the expected
value
> > > with the actual value.
> > 
> > I'm not sure what you mean by this. What do you mean by setting expectations
> > directly.
> 
> In this patch, we're testing GetClientToken() by indirectly comparing the
output
> of GetClientToken against the output of... itself.
> 
> It'd be more effective if we compared it to a known fixed string constant.

So, would adding an assert like so work? Or are we looking for something a
little deeper than that?

[CJ, 2016-05-11 23:48:50]

Addresses the rest of kmarshalls #7 comments

https://codereview.chromium.org/1958033003/diff/20001/blimp/client/session/as...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/client/session/as...
blimp/client/session/assignment_source_unittest.cc:87:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2016/05/11 00:39:57, Kevin M wrote:
> See what we're doing with kCertRelativePath to get an idea of how we can use
the
> test_client_token file.

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/BUILD.gn#n...
blimp/common/BUILD.gn:38: source_set("app_switches") {
On 2016/05/11 00:39:57, Kevin M wrote:
> "switches"
> 
> Does this need to be a standalone target, or can it be rolled in to "common"?

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.cc
File blimp/common/switches.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.c...
blimp/common/switches.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2016/05/11 00:39:57, Kevin M wrote:
> 2016

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h
File blimp/common/switches.h (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h...
blimp/common/switches.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2016/05/11 00:39:57, Kevin M wrote:
> 2016

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h...
blimp/common/switches.h:9: namespace engine {
On 2016/05/11 00:39:57, Kevin M wrote:
> The "engine" namespace doesn't belong here.

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/common/switches.h...
blimp/common/switches.h:14: // The port to listen to for incoming TCP
connections.
On 2016/05/11 00:39:57, Kevin M wrote:
> A listen port is engine-specific, so it shouldn't be under common/.

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:71: "//blimp/common:app_switches",
On 2016/05/11 00:39:57, Kevin M wrote:
> Remove "app_switches" and just use "common" top-level instead

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_config.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_config.cc:17: #include "blimp/common/switches.h"
On 2016/05/11 00:39:57, Kevin M wrote:
> Do we need to include this file, now that GetClientToken() is responsible for
> accessing the switch?

Done.

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_screen_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_screen_unittest.cc:16: #include
"blimp/common/switches.h"
On 2016/05/11 00:39:57, Kevin M wrote:
> Not needed; no switches are referenced in this file.

Done.

[Kevin M, 2016-05-12 17:44:44]

lgtm

Awesome! Click the "Commit" checkbox on the CL once you've addressed the final
changes and want to submit.

https://codereview.chromium.org/1958033003/diff/100001/blimp/client/session/a...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/100001/blimp/client/session/a...
blimp/client/session/assignment_source_unittest.cc:212: 
Nit: remove this newline, so that all command-line handling stuffs is in the
same chunk (same below).

https://codereview.chromium.org/1958033003/diff/100001/blimp/common/switches.h
File blimp/common/switches.h (right):

https://codereview.chromium.org/1958033003/diff/100001/blimp/common/switches....
blimp/common/switches.h:12: // Path to the client token/shared secret between
the engine and the client.
nit: capitalized Engine and Client.

[CJ, 2016-05-12 18:09:30]

Issues addressed. Will commit after running tests on device.

https://codereview.chromium.org/1958033003/diff/100001/blimp/client/session/a...
File blimp/client/session/assignment_source_unittest.cc (right):

https://codereview.chromium.org/1958033003/diff/100001/blimp/client/session/a...
blimp/client/session/assignment_source_unittest.cc:212: 
On 2016/05/12 17:44:44, Kevin M wrote:
> Nit: remove this newline, so that all command-line handling stuffs is in the
> same chunk (same below).

Done.

https://codereview.chromium.org/1958033003/diff/100001/blimp/common/switches.h
File blimp/common/switches.h (right):

https://codereview.chromium.org/1958033003/diff/100001/blimp/common/switches....
blimp/common/switches.h:12: // Path to the client token/shared secret between
the engine and the client.
On 2016/05/12 17:44:44, Kevin M wrote:
> nit: capitalized Engine and Client.

Done.

[CJ, 2016-05-12 18:37:55]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-05-12 18:37:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1958033003/#ps120001
(title: "Addresses kmarshall's #12 comments")

[CJ, 2016-05-12 18:38:05]

The CQ bit was unchecked by lethalantidote@chromium.org

[commit-bot: I haz the power, 2016-05-12 18:38:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958033003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958033003/120001

[CJ, 2016-05-12 18:38:24]

The CQ bit was checked by lethalantidote@chromium.org

[commit-bot: I haz the power, 2016-05-12 18:39:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958033003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958033003/120001

[CJ, 2016-05-12 18:45:32]

The CQ bit was unchecked by lethalantidote@chromium.org

[CJ, 2016-05-12 23:36:23]

The CQ bit was checked by lethalantidote@chromium.org

[commit-bot: I haz the power, 2016-05-12 23:36:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958033003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958033003/120001

[commit-bot: I haz the power, 2016-05-12 23:52:29]

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

[commit-bot: I haz the power, 2016-05-12 23:52:30]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[CJ, 2016-05-13 00:15:59]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-05-13 00:16:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1958033003/#ps140001
(title: "Fixes errors")

[commit-bot: I haz the power, 2016-05-13 00:16:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958033003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958033003/140001

[commit-bot: I haz the power, 2016-05-13 00:21:06]

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

[commit-bot: I haz the power, 2016-05-13 00:21:07]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[CJ, 2016-05-13 00:45:09]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-05-13 00:45:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1958033003/#ps160001
(title: "Fixes analyze")

[commit-bot: I haz the power, 2016-05-13 00:45:26]

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

[commit-bot: I haz the power, 2016-05-13 01:57:54]

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

[commit-bot: I haz the power, 2016-05-13 01:57:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nyquist, 2016-05-13 17:14:49]

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

[nyquist, 2016-05-13 17:14:50]

Please remember to put the subject from the code review as the first line in the
description, and then an empty line.

Our tools are a little bit odd in that we have to duplicate the first line in
both the Subject and as the first line in the Description field.

Please do this before you try to commit again. You can do this by clicking 'Edit
Issue' in the top left, and then clicking 'Update Issue' when you're done.

[CJ, 2016-05-13 20:33:47]

Description was changed from

==========
This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

to

==========
Issue 1958033003: Allows client to access auth token from command line specified
file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

[CJ, 2016-05-13 20:34:43]

On 2016/05/13 17:14:50, nyquist wrote:
> Please remember to put the subject from the code review as the first line in
the
> description, and then an empty line.
> 
> Our tools are a little bit odd in that we have to duplicate the first line in
> both the Subject and as the first line in the Description field.
> 
> Please do this before you try to commit again. You can do this by clicking
'Edit
> Issue' in the top left, and then clicking 'Update Issue' when you're done.

Like so? Wasn't sure if the issue number should be included.

[nyquist, 2016-05-13 20:35:39]

Almost, just remove the issue-number. Just start with "Allows..."

[CJ, 2016-05-13 20:36:34]

Description was changed from

==========
Issue 1958033003: Allows client to access auth token from command line specified
file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

to

==========
Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

[CJ, 2016-05-13 20:37:07]

On 2016/05/13 20:35:39, nyquist wrote:
> Almost, just remove the issue-number. Just start with "Allows..."

Cool. Thank you!

[CJ, 2016-05-13 22:50:16]

Hello,

I have updated running.md. Please let me know if there should be any changes.

Thank you,
CJ

[chromium-reviews, 2016-05-13 22:52:13]

Not sure why or how running.md got auto-linked to something random. Please
ignore.

On Fri, May 13, 2016 at 3:50 PM, <lethalantidote@chromium.org> wrote:

> Hello,
>
> I have updated running.md. Please let me know if there should be any
> changes.
>
> Thank you,
> CJ
>
> https://codereview.chromium.org/1958033003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kevin M, 2016-05-16 17:58:18]

still lgtm !

[CJ, 2016-05-16 18:01:24]

The CQ bit was checked by lethalantidote@chromium.org

[commit-bot: I haz the power, 2016-05-16 18:02:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1958033003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1958033003/200001

[commit-bot: I haz the power, 2016-05-16 19:07:02]

Description was changed from

==========
Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

to

==========
Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

[commit-bot: I haz the power, 2016-05-16 19:07:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-16 19:08:11]

Description was changed from

==========
Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory. 

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines. 
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency. 

BUG=610398
==========

to

==========
Allows client to access auth token from command line specified file.

This change allows the client obtain the auth token from the file specified on
the command line just as the server does. Because the functionality would be
very similar, this was pulled out and placed in the common directory.

This allows the code to move away from a hard-coded auth token, and gives the
developer the freedom to specify the auth token, as one has the power to do now
on the server.

Other changes:
* Moved code from blimp_engine_config_unittest.cc into fake_commandline.cc and
fake_commandline.h to make fake command line functionality accessible to both
client and server code.
* Updated blimp_engine_config_unittest.cc and assignment_source_unittest.cc to
use fake_commandline.cc functionality to create fake command lines.
* Updated blimp_browser_test.cc to use get_client_token:GetClientToken() to
maintain consistency.

BUG=610398

Committed: https://crrev.com/f16b3ac745768029718fbbc7cd288453677e49e8
Cr-Commit-Position: refs/heads/master@{#393890}
==========

[commit-bot: I haz the power, 2016-05-16 19:08:13]

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