Chromad: added AD Join ui, authpolicy_client

Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f2e2c4230755808178a0b50ffe482a06586f9c63
Cr-Commit-Position: refs/heads/master@{#429846}

[Roman Sorokin (ftl), 2016-10-21 13:32:01]

Description was changed from

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
==========

to

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Roman Sorokin (ftl), 2016-10-21 13:32:48]

The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 13:33:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-10-21 13:33:29]

rsorokin@chromium.org changed reviewers:
+ alemate@chromium.org, tnagel@chromium.org

[Roman Sorokin (ftl), 2016-10-21 13:33:29]

Thiemo, Alex, please take a look

[commit-bot: I haz the power, 2016-10-21 14:03:52]

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

[commit-bot: I haz the power, 2016-10-21 14:03:53]

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

[hashimoto, 2016-10-24 03:01:17]

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

[hashimoto, 2016-10-24 03:01:18]

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
File chromeos/dbus/authpolicy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:6: #include <map>
Please remove unneeded include.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:28: // TODO(rsorokin): switch to service
constants when it's landed.
URL to the bug?

Please follow the naming rule
https://google.github.io/styleguide/cppguide.html#Constant_Names

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:29: const char* objPath =
"/org/chromium/authpolicy/Domain";
This should be const char[].

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:32: const char* joinMethod = "Join";
Add a blank line.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:37: AuthpolicyClientImpl() : bus_(NULL),
weak_ptr_factory_(this) {}
nullptr instead of NULL.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:47: if (pipe(fd) < 0) {
Please move this pipe creation/preparation logic out of this class.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:57: if (close(fd[1])) {
Use ScopedFD.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:84: callback.Run(1);
What does this "1" mean?

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:89: dbus::ObjectPath
connection_settings_path;
These variables are unused.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
File chromeos/dbus/authpolicy_client.h (right):

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:14: #include
"chromeos/dbus/dbus_method_call_status.h"
Remove unused include.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:17: class DictionaryValue;
Remove unused forward declarations.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:25: 
Add a comment to describe what this class is.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:26: class CHROMEOS_EXPORT AuthpolicyClient :
public DBusClient {
Is "authpolicy" a word? Isn't AuthPolicy better?

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:28: typedef base::Callback<void(int)>
JoinCallback;
Please give the int parameter a name.
Like:
 base::Callback<void(int foo)>

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:36: virtual void Join(const std::string&
machine_name,
Please add a comment about this method.

[Roman Sorokin (ftl), 2016-10-24 16:50:34]

hashimoto@, thanks for the review! Sorry for the mess. PTAL

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
File chromeos/dbus/authpolicy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:6: #include <map>
On 2016/10/24 03:01:18, hashimoto wrote:
> Please remove unneeded include.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:28: // TODO(rsorokin): switch to service
constants when it's landed.
On 2016/10/24 03:01:18, hashimoto wrote:
> URL to the bug?
> 
> Please follow the naming rule
> https://google.github.io/styleguide/cppguide.html#Constant_Names

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:29: const char* objPath =
"/org/chromium/authpolicy/Domain";
On 2016/10/24 03:01:18, hashimoto wrote:
> This should be const char[].

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:32: const char* joinMethod = "Join";
On 2016/10/24 03:01:18, hashimoto wrote:
> Add a blank line.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:37: AuthpolicyClientImpl() : bus_(NULL),
weak_ptr_factory_(this) {}
On 2016/10/24 03:01:18, hashimoto wrote:
> nullptr instead of NULL.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:47: if (pipe(fd) < 0) {
On 2016/10/24 03:01:18, hashimoto wrote:
> Please move this pipe creation/preparation logic out of this class.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:57: if (close(fd[1])) {
On 2016/10/24 03:01:18, hashimoto wrote:
> Use ScopedFD.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:84: callback.Run(1);
On 2016/10/24 03:01:18, hashimoto wrote:
> What does this "1" mean?

Have not defined error codes yet. For now it's just |not success|
Added TODO

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.cc:89: dbus::ObjectPath
connection_settings_path;
On 2016/10/24 03:01:18, hashimoto wrote:
> These variables are unused.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
File chromeos/dbus/authpolicy_client.h (right):

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:14: #include
"chromeos/dbus/dbus_method_call_status.h"
On 2016/10/24 03:01:18, hashimoto wrote:
> Remove unused include.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:17: class DictionaryValue;
On 2016/10/24 03:01:18, hashimoto wrote:
> Remove unused forward declarations.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:25: 
On 2016/10/24 03:01:18, hashimoto wrote:
> Add a comment to describe what this class is.

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:26: class CHROMEOS_EXPORT AuthpolicyClient :
public DBusClient {
On 2016/10/24 03:01:18, hashimoto wrote:
> Is "authpolicy" a word? Isn't AuthPolicy better?

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:28: typedef base::Callback<void(int)>
JoinCallback;
On 2016/10/24 03:01:18, hashimoto wrote:
> Please give the int parameter a name.
> Like:
>  base::Callback<void(int foo)>

Done.

https://codereview.chromium.org/2433363004/diff/1/chromeos/dbus/authpolicy_cl...
chromeos/dbus/authpolicy_client.h:36: virtual void Join(const std::string&
machine_name,
On 2016/10/24 03:01:18, hashimoto wrote:
> Please add a comment about this method.

Done.

[hashimoto, 2016-10-26 06:11:12]

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
File chromeos/dbus/authpolicy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:18: // TODO(rsorokin): switch to service
constants when it's landed (crbug/658655/)
nit: crbug.com is the publicly accessible URL of our bug tracker.

Can't you land the service constants change first?
Are you working on a project which requires internal code access?

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:36: base::WorkerPool::GetTaskRunner(true /*
tasks_are_slow */);
Generally speaking, you should avoid using WorkerPool.
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:37: pipe_string_writer_.reset(new
PipeStringWriter(
Sorry for not being clear, please put the pipe related logic to somewhere close
to the user code (i.e. enrollment_screen_handler.cc), and make this client just
receive its FD.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:63: // TODO(rsorokin): make proper call,
after defining possible errors codes.
Before this change, can't you land error code constants together with the
service constants above?

Even when not, please make the "1" a constant and put it next to the service
constants above.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:69: int res;
Please don't leave a variable uninitialized.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
File chromeos/dbus/authpolicy_client.h (right):

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.h:5: #ifndef CHROMEOS_DBUS_AUTHPOLICY_CLIENT_H_
The file name should be auth_policy_client.h
The same goes for other files and variable names.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.h:29: // Calls JoinADDomain. Uses |machine_name|
to join it to the domain. |user|,
Could you add some words to make it clear what "joining AD domain" means?
Especially, I don't think most Chromium developers can understand the
abbreviation "AD".

[Thiemo Nagel, 2016-10-26 09:12:14]

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
File chromeos/dbus/authpolicy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:18: // TODO(rsorokin): switch to service
constants when it's landed (crbug/658655/)
On 2016/10/26 06:11:12, hashimoto wrote:
> nit: http://crbug.com is the publicly accessible URL of our bug tracker.

I think a nice way to write this would be:

TODO(rsorokin, 658655): Switch to service constants when they have landed.

[Roman Sorokin (ftl), 2016-10-26 18:45:34]

rsorokin@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Roman Sorokin (ftl), 2016-10-26 18:45:36]

hashimoto@, PTAL on auth_policy_client

xiyuan@, PTAL at c/b

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
File chromeos/dbus/authpolicy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:18: // TODO(rsorokin): switch to service
constants when it's landed (crbug/658655/)
On 2016/10/26 06:11:12, hashimoto wrote:
> nit: http://crbug.com is the publicly accessible URL of our bug tracker.
> 
> Can't you land the service constants change first?
> Are you working on a project which requires internal code access?

Done.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:18: // TODO(rsorokin): switch to service
constants when it's landed (crbug/658655/)
On 2016/10/26 09:12:13, Thiemo Nagel (slow) wrote:
> On 2016/10/26 06:11:12, hashimoto wrote:
> > nit: http://crbug.com is the publicly accessible URL of our bug tracker.
> 
> I think a nice way to write this would be:
> 
> TODO(rsorokin, 658655): Switch to service constants when they have landed.

Done.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:36: base::WorkerPool::GetTaskRunner(true /*
tasks_are_slow */);
On 2016/10/26 06:11:11, hashimoto wrote:
> Generally speaking, you should avoid using WorkerPool.
>
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

Done.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:37: pipe_string_writer_.reset(new
PipeStringWriter(
On 2016/10/26 06:11:12, hashimoto wrote:
> Sorry for not being clear, please put the pipe related logic to somewhere
close
> to the user code (i.e. enrollment_screen_handler.cc), and make this client
just
> receive its FD.

Done.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:63: // TODO(rsorokin): make proper call,
after defining possible errors codes.
On 2016/10/26 06:11:11, hashimoto wrote:
> Before this change, can't you land error code constants together with the
> service constants above?
> 
> Even when not, please make the "1" a constant and put it next to the service
> constants above.

Constants cl is in the pipeline, left TODO

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.cc:69: int res;
On 2016/10/26 06:11:12, hashimoto wrote:
> Please don't leave a variable uninitialized.

Done.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
File chromeos/dbus/authpolicy_client.h (right):

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.h:5: #ifndef CHROMEOS_DBUS_AUTHPOLICY_CLIENT_H_
On 2016/10/26 06:11:12, hashimoto wrote:
> The file name should be auth_policy_client.h
> The same goes for other files and variable names.

Done.

https://codereview.chromium.org/2433363004/diff/20001/chromeos/dbus/authpolic...
chromeos/dbus/authpolicy_client.h:29: // Calls JoinADDomain. Uses |machine_name|
to join it to the domain. |user|,
On 2016/10/26 06:11:12, hashimoto wrote:
> Could you add some words to make it clear what "joining AD domain" means?
> Especially, I don't think most Chromium developers can understand the
> abbreviation "AD".

Can't do that for now. Added TODO to change it in the future.

[xiyuan, 2016-10-26 22:03:17]

https://codereview.chromium.org/2433363004/diff/60001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:6176: <message
name="IDS_AD_MACHINE_NAME_INPUT_LABEL">
Add a desc to let translators have more context of where the text is used, here
and other new strings.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_config.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_config.h:104: // The realm the device
joined to (if managed by AD)
nit: Add a "is" and end with "."
i.e.
The realm the device joined to (if managed by AD) ->
The realm the device [is] joined to (if managed by AD)[.] ?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/offline_ad_login.html (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:15:
'showMachineInput' - If the input of the machine name should be shown.
nit: If -> Whether

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:16: Used for
domain joining.
nit: Indent 4 more spaces or align with the beginning of "If the input..."

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:18: 'userRealm' -
autocomplete realm for the user input.
nit: autocomplete -> Autocomplete

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:21:
'authCompleted' - fired when user enters login and password. Fires with an
nit: fired -> Fired, or keep the lower case and fix other places where we start
with uppercase. We should be consistent.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:33: <gaia-card
id="gaiaCard" class="fit">
nit: wrong indent

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:42: <gaia-input
id="machineNameInput" required hidden="[[!showMachineInput]]"
What does it mean when the field is required and also hidden?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:42: <gaia-input
id="machineNameInput" required hidden="[[!showMachineInput]]"
nit: wrap to fit in 80 chars per line

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:29:
this.$.machineNameInput.focus();
What if machineNameInput is hidden ?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:39:
loadTimeData.getStringF('ADAuthWelcomeMessage', this.realm);
nit: wrong indent, add 2 more spaces

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:42: setUser:
function(user, machineName) {
Document the function and mention |user| and |machienName| are optinal.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:45:
this.$.passwordInput.isInvalid = true;
Why setting/resetting the userInput changes passwordInput.isInvalid flag?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:80:
this.$.gaiaCard.classList.remove('disabled');
nit: seems the "if" could be replace with:
  this.$.gaiaCard.classList.toggle('disabled', newValue);

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:64:
offline_ad_ui_: undefined,
offline_ad_ui_ -> offlineAdUi_ to fit JS var name convention

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:127: 
nit: nuke one empty line ?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:185: }
What would "back" do for STEP_AD_JOIN ?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:12: #include
"base/command_line.h"
nit: not used?

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:35: #include
"content/public/browser/browser_thread.h"
nit: remove if we don't use.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:406:
builder->Add("ADLoginUser", IDS_AD_LOGIN_USER);
nit: ADLoginUser -> adLoginUser, similarly for ADLoginPassword

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:546: if
(!base::CreateLocalNonBlockingPipe(pipe_fds)) {
Are we allowed to do this (and the write on Line 553) on UI thread? If we have
to do it on UI thread, let's add a base::ThreadRestrictions::ScopedAllowIO with
a comment.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:576:
CallJS("invalidateAD", machine_name, user);
Does this mean the join fails? Do we need to show an error or something to the
user?

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:20: AuthPolicyClientImpl() : bus_(nullptr),
weak_ptr_factory_(this) {}
nit: move |bus_| initializer to line 73 where it is declared.
i.e.
  dbus::Bus* bus_ = nullptr;

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:24: void JoinADDomain(const std::string&
machine_name,
nit: // AuthPolicyClient:

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:67: void OnPipeReadComplete(int error_code)
{
Is this used anywhere?

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.h:30: typedef base::Callback<void(int
error_code)> JoinCallback;
nit: using JoinCallback = base::Callback<void(int error_code)>;

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:108: AuthPolicyClient*
DBusThreadManager::GetAuthPolicyClient() {
nit: move this after GetArcObbMounterClient to follow the declaration order in
header.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
File chromeos/dbus/fake_auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:19: void Init(dbus::Bus* bus) override;
nit: // AuthPolicyClient:

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:20: 
nit: nuke the empty line?

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:26: protected:
private: ?

[hashimoto, 2016-10-27 02:26:07]

Thanks, code under chromeos/dbus almost looks good to me.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:8: #include "base/threading/worker_pool.h"
nit: Please remove unused include.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_clients_browser.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_browser.cc:57:
auth_policy_client_.reset(AuthPolicyClient::Create());
nit: Sort in the alphabetical order.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_clients_browser.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_browser.h:26: class AuthPolicyClient;
nit: Sort in the alphabetical order.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_browser.h:49: std::unique_ptr<AuthPolicyClient>
auth_policy_client_;
ditto.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
File chromeos/dbus/fake_auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:19: void Init(dbus::Bus* bus) override;
On 2016/10/26 22:03:17, xiyuan wrote:
> nit: // AuthPolicyClient:

As xiyuan said, please add comments to make it clear which class you are
overriding.

Actually, Init() is overriding DBusClient so it should say DBusClient while
JoinADDomain is overriding AuthPolicyClient.

[Roman Sorokin (ftl), 2016-10-27 13:10:46]

Thanks for the quick reviews! PTAL.

https://codereview.chromium.org/2433363004/diff/60001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:6176: <message
name="IDS_AD_MACHINE_NAME_INPUT_LABEL">
On 2016/10/26 22:03:16, xiyuan wrote:
> Add a desc to let translators have more context of where the text is used,
here
> and other new strings.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_config.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_config.h:104: // The realm the device
joined to (if managed by AD)
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: Add a "is" and end with "."
> i.e.
> The realm the device joined to (if managed by AD) ->
> The realm the device [is] joined to (if managed by AD)[.] ?

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/offline_ad_login.html (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:15:
'showMachineInput' - If the input of the machine name should be shown.
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: If -> Whether

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:16: Used for
domain joining.
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: Indent 4 more spaces or align with the beginning of "If the input..."

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:18: 'userRealm' -
autocomplete realm for the user input.
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: autocomplete -> Autocomplete

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:21:
'authCompleted' - fired when user enters login and password. Fires with an
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: fired -> Fired, or keep the lower case and fix other places where we
start
> with uppercase. We should be consistent.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:33: <gaia-card
id="gaiaCard" class="fit">
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:42: <gaia-input
id="machineNameInput" required hidden="[[!showMachineInput]]"
On 2016/10/26 22:03:16, xiyuan wrote:
> What does it mean when the field is required and also hidden?

Right! It's required if it's shown. Added check in .js

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.html:42: <gaia-input
id="machineNameInput" required hidden="[[!showMachineInput]]"
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: wrap to fit in 80 chars per line

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:29:
this.$.machineNameInput.focus();
On 2016/10/26 22:03:16, xiyuan wrote:
> What if machineNameInput is hidden ?

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:39:
loadTimeData.getStringF('ADAuthWelcomeMessage', this.realm);
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: wrong indent, add 2 more spaces

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:42: setUser:
function(user, machineName) {
On 2016/10/26 22:03:16, xiyuan wrote:
> Document the function and mention |user| and |machienName| are optinal.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:45:
this.$.passwordInput.isInvalid = true;
On 2016/10/26 22:03:16, xiyuan wrote:
> Why setting/resetting the userInput changes passwordInput.isInvalid flag?

We used that way in offline_gaia to load ui with second time after unsuccessful
login (when we already know username) and invalidate password input.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/offline_ad_login.js:80:
this.$.gaiaCard.classList.remove('disabled');
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: seems the "if" could be replace with:
>   this.$.gaiaCard.classList.toggle('disabled', newValue);

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:64:
offline_ad_ui_: undefined,
On 2016/10/26 22:03:16, xiyuan wrote:
> offline_ad_ui_ -> offlineAdUi_ to fit JS var name convention

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:127: 
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: nuke one empty line ?

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:185: }
On 2016/10/26 22:03:16, xiyuan wrote:
> What would "back" do for STEP_AD_JOIN ?

We don't show the button on domain join screen yet. We show |close| button
though. (But UX is no finalised)

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:12: #include
"base/command_line.h"
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: not used?

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:35: #include
"content/public/browser/browser_thread.h"
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: remove if we don't use.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:406:
builder->Add("ADLoginUser", IDS_AD_LOGIN_USER);
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: ADLoginUser -> adLoginUser, similarly for ADLoginPassword

Done.

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:546: if
(!base::CreateLocalNonBlockingPipe(pipe_fds)) {
On 2016/10/26 22:03:16, xiyuan wrote:
> Are we allowed to do this (and the write on Line 553) on UI thread? If we have
> to do it on UI thread, let's add a base::ThreadRestrictions::ScopedAllowIO
with
> a comment.

Moved onto BlockingPool

https://codereview.chromium.org/2433363004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:576:
CallJS("invalidateAD", machine_name, user);
On 2016/10/26 22:03:16, xiyuan wrote:
> Does this mean the join fails? Do we need to show an error or something to the
> user?

Yes! But we don't have list of errors just yet. Added TODO and BUG to fix that.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:8: #include "base/threading/worker_pool.h"
On 2016/10/27 02:26:07, hashimoto wrote:
> nit: Please remove unused include.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:20: AuthPolicyClientImpl() : bus_(nullptr),
weak_ptr_factory_(this) {}
On 2016/10/26 22:03:17, xiyuan wrote:
> nit: move |bus_| initializer to line 73 where it is declared.
> i.e.
>   dbus::Bus* bus_ = nullptr;

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:24: void JoinADDomain(const std::string&
machine_name,
On 2016/10/26 22:03:16, xiyuan wrote:
> nit: // AuthPolicyClient:

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:67: void OnPipeReadComplete(int error_code)
{
On 2016/10/26 22:03:17, xiyuan wrote:
> Is this used anywhere?

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.h:30: typedef base::Callback<void(int
error_code)> JoinCallback;
On 2016/10/26 22:03:17, xiyuan wrote:
> nit: using JoinCallback = base::Callback<void(int error_code)>;

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_clients_browser.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_browser.cc:57:
auth_policy_client_.reset(AuthPolicyClient::Create());
On 2016/10/27 02:26:07, hashimoto wrote:
> nit: Sort in the alphabetical order.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_clients_browser.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_browser.h:26: class AuthPolicyClient;
On 2016/10/27 02:26:07, hashimoto wrote:
> nit: Sort in the alphabetical order.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_browser.h:49: std::unique_ptr<AuthPolicyClient>
auth_policy_client_;
On 2016/10/27 02:26:07, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:108: AuthPolicyClient*
DBusThreadManager::GetAuthPolicyClient() {
On 2016/10/26 22:03:17, xiyuan wrote:
> nit: move this after GetArcObbMounterClient to follow the declaration order in
> header.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
File chromeos/dbus/fake_auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:19: void Init(dbus::Bus* bus) override;
On 2016/10/26 22:03:17, xiyuan wrote:
> nit: // AuthPolicyClient:

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:19: void Init(dbus::Bus* bus) override;
On 2016/10/27 02:26:07, hashimoto wrote:
> On 2016/10/26 22:03:17, xiyuan wrote:
> > nit: // AuthPolicyClient:
> 
> As xiyuan said, please add comments to make it clear which class you are
> overriding.
> 
> Actually, Init() is overriding DBusClient so it should say DBusClient while
> JoinADDomain is overriding AuthPolicyClient.

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:20: 
On 2016/10/26 22:03:17, xiyuan wrote:
> nit: nuke the empty line?

Done.

https://codereview.chromium.org/2433363004/diff/60001/chromeos/dbus/fake_auth...
chromeos/dbus/fake_auth_policy_client.h:26: protected:
On 2016/10/26 22:03:17, xiyuan wrote:
> private: ?

Done.

[xiyuan, 2016-10-27 16:58:17]

lgtm

https://codereview.chromium.org/2433363004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:567: 
nit: nuke the two empty lines

[hashimoto, 2016-10-28 09:00:36]

chromeos/dbus lgtm

[Roman Sorokin (ftl), 2016-10-28 09:31:19]

https://codereview.chromium.org/2433363004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:567: 
On 2016/10/27 16:58:17, xiyuan wrote:
> nit: nuke the two empty lines

Done.

[Roman Sorokin (ftl), 2016-10-28 09:31:48]

The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 09:32:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 09:58:01]

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

[commit-bot: I haz the power, 2016-10-28 09:58:02]

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

[Alexander Alekseev, 2016-10-28 10:49:10]

alemate@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Alexander Alekseev, 2016-10-28 10:49:11]

+michaelpg

Michael, could you, please, take a look at *.html and *.js files?

https://codereview.chromium.org/2433363004/diff/100001/chrome/app/chromeos_st...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/app/chromeos_st...
chrome/app/chromeos_strings.grdp:6176: <message
name="IDS_AD_MACHINE_NAME_INPUT_LABEL" desc="Label for machine name input. User
should tell us the name of his machine" >
The description one item above suggests that we should describe not only the
meaning of the phrase itself, but also a context that it is used in.
Please include short screen description and a hint that this is a label for an
input field.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:58: virtual
void ShowADJoin() = 0;
Document this, please.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:329: //
TODO(rsorokin): remove that when lock the device with both realm and
"that" -> "device_mode_ check"?
"When device is locked with both realm and domain set" ?

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:4: 
Document this file, please.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:111: //
Returns reading end of pipe with the password to read.
nit: Returns file descriptor of a pipe, open for reading. Pipe keeps user
password, which can be read from the returned descriptor.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:112:
base::ScopedFD GetPasswordReadPipe(const std::string& password) {
Add DCHECK that this function is rinnung on a BlockingPool.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:115:
LOG(ERROR) << "Failed to create pipes";
nit: pipe

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:571:
base::ScopedFD password_fd) {
Add DCHECK for UI thread.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:593:
controller_->OnADJoined(gaia::ExtractDomainName(user));
return here?

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:595: //
TODO(rsorokin,659984): Add passing/displaying error codes.
Could you, please, add issues as full URL?
Like crbug.com/659984 . This makes it simpler to open.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:77: void
OnPasswordPipeReady(const std::string& machine_name,
This is not WebUI handler, is it?
Could you move it below this group?

[Roman Sorokin (ftl), 2016-10-28 12:35:06]

The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 12:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-10-28 12:35:29]

https://codereview.chromium.org/2433363004/diff/100001/chrome/app/chromeos_st...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/app/chromeos_st...
chrome/app/chromeos_strings.grdp:6176: <message
name="IDS_AD_MACHINE_NAME_INPUT_LABEL" desc="Label for machine name input. User
should tell us the name of his machine" >
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> The description one item above suggests that we should describe not only the
> meaning of the phrase itself, but also a context that it is used in.
> Please include short screen description and a hint that this is a label for an
> input field.

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:58: virtual
void ShowADJoin() = 0;
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> Document this, please.

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:329: //
TODO(rsorokin): remove that when lock the device with both realm and
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> "that" -> "device_mode_ check"?
> "When device is locked with both realm and domain set" ?

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:4: 
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> Document this file, please.

Hmm, our polymer elements have documentation in the html files. All the js files
don't.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:111: //
Returns reading end of pipe with the password to read.
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> nit: Returns file descriptor of a pipe, open for reading. Pipe keeps user
> password, which can be read from the returned descriptor.

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:112:
base::ScopedFD GetPasswordReadPipe(const std::string& password) {
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> Add DCHECK that this function is rinnung on a BlockingPool.

Hm, How to check that? DCHECK_CURRENTLY_ON accepts only names threads, which
blockingpool is not.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:115:
LOG(ERROR) << "Failed to create pipes";
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> nit: pipe

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:571:
base::ScopedFD password_fd) {
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> Add DCHECK for UI thread.

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:593:
controller_->OnADJoined(gaia::ExtractDomainName(user));
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> return here?

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:595: //
TODO(rsorokin,659984): Add passing/displaying error codes.
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> Could you, please, add issues as full URL?
> Like crbug.com/659984 . This makes it simpler to open.

Done.

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:77: void
OnPasswordPipeReady(const std::string& machine_name,
On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> This is not WebUI handler, is it?
> Could you move it below this group?

Done.

[Alexander Alekseev, 2016-10-28 13:00:59]

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:4: 
On 2016/10/28 12:35:28, Roman Sorokin wrote:
> On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> > Document this file, please.
> 
> Hmm, our polymer elements have documentation in the html files. All the js
files
> don't.

Chromium coding style
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md
) requires use of Web Development Style Guide
(https://www.chromium.org/developers/web-development-style-guide) which requires
conformance to Google JavaScript Style Guide
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md)
with a few exceptions.

"All files, classes, methods and properties should be documented with JSDoc
comments with the appropriate tags and types. Textual descriptions for
properties, methods, method parameters and method return values should be
included unless obvious from the property, method, or parameter name."

+ don't forget about closure compiler.

You can take a look at other (recent) polymer elements in
chrome/browser/resources/chromeos/login/* or in settings/* (which is probably
better source).

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:112:
base::ScopedFD GetPasswordReadPipe(const std::string& password) {
On 2016/10/28 12:35:29, Roman Sorokin wrote:
> On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> > Add DCHECK that this function is rinnung on a BlockingPool.
> 
> Hm, How to check that? DCHECK_CURRENTLY_ON accepts only names threads, which
> blockingpool is not.

DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[commit-bot: I haz the power, 2016-10-28 13:34:12]

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

[commit-bot: I haz the power, 2016-10-28 13:34:14]

Dry run: This issue passed the CQ dry run.

[Roman Sorokin (ftl), 2016-10-28 14:07:14]

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:112:
base::ScopedFD GetPasswordReadPipe(const std::string& password) {
On 2016/10/28 13:00:59, Alexander Alekseev wrote:
> On 2016/10/28 12:35:29, Roman Sorokin wrote:
> > On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> > > Add DCHECK that this function is rinnung on a BlockingPool.
> > 
> > Hm, How to check that? DCHECK_CURRENTLY_ON accepts only names threads, which
> > blockingpool is not.
> 
> DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

Done.

[Roman Sorokin (ftl), 2016-10-28 14:09:03]

rsorokin@chromium.org changed reviewers:
+ dbeam@chromium.org

[Roman Sorokin (ftl), 2016-10-28 14:09:04]

dbeam@chromium.org: Please review changes in 

third_party/closure_compiler/compiled_resources2.gyp

[michaelpg, 2016-10-28 19:04:42]

On 2016/10/28 10:49:11, Alexander Alekseev wrote:
> +michaelpg
> 
> Michael, could you, please, take a look at *.html and *.js files?

Can you provide some context in the bug (which is literally empty)? What is
Chromad? What is AD? What do we need authpolicyd for?

I don't need a full technical specification but I have no idea what this CL is
for, so a little info would go a long way. Thanks!

> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/app/chromeos_st...
> File chrome/app/chromeos_strings.grdp (right):
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/app/chromeos_st...
> chrome/app/chromeos_strings.grdp:6176: <message
> name="IDS_AD_MACHINE_NAME_INPUT_LABEL" desc="Label for machine name input.
User
> should tell us the name of his machine" >
> The description one item above suggests that we should describe not only the
> meaning of the phrase itself, but also a context that it is used in.
> Please include short screen description and a hint that this is a label for an
> input field.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h
(right):
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:58: virtual
> void ShowADJoin() = 0;
> Document this, please.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:329: //
> TODO(rsorokin): remove that when lock the device with both realm and
> "that" -> "device_mode_ check"?
> "When device is locked with both realm and domain set" ?
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
> File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/login/offline_ad_login.js:4: 
> Document this file, please.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:111: //
> Returns reading end of pipe with the password to read.
> nit: Returns file descriptor of a pipe, open for reading. Pipe keeps user
> password, which can be read from the returned descriptor.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:112:
> base::ScopedFD GetPasswordReadPipe(const std::string& password) {
> Add DCHECK that this function is rinnung on a BlockingPool.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:115:
> LOG(ERROR) << "Failed to create pipes";
> nit: pipe
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:571:
> base::ScopedFD password_fd) {
> Add DCHECK for UI thread.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:593:
> controller_->OnADJoined(gaia::ExtractDomainName(user));
> return here?
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:595: //
> TODO(rsorokin,659984): Add passing/displaying error codes.
> Could you, please, add issues as full URL?
> Like crbug.com/659984 . This makes it simpler to open.
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h
(right):
> 
>
https://codereview.chromium.org/2433363004/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:77: void
> OnPasswordPipeReady(const std::string& machine_name,
> This is not WebUI handler, is it?
> Could you move it below this group?

[Dan Beam, 2016-10-28 21:07:45]

lgtm w/nits

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:54: * */
nit: /** @private */

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:66: }
nit: no curlies

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:80: }
nit:

this.$.userInput.value = user || '';
this.$.passwordInput.isInvalid = !!user;

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:96: }
nit: this seems equivalent and shorter

if (!user.includes('@') && this.userRealm)
  user += this.userRealm;

(assuming it actually compiles)

[Dan Beam, 2016-10-28 21:13:04]

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:20:
"ad-welcome-message:oauthEnrollADDomainJoinWelcomeMessage">
4\s indent for continuations in HTML

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:63: 
can you document and/or type this?  and maybe add @private?

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:125:
}).bind(this));
these outer () aren't necessary, i.e. you can just do function(e) { ...
}.bind(this)

also, this file is odd, but

this.offlineAdUi_.addEventListener('authCompleted', function(e) {
  ...
});

adds less indent

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:182: if
(this.currentStep_ == STEP_SIGNIN) {
nit: no curlies

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:126:
LOG(ERROR) << "Failed to write to pipe";
do you want this in release builds?  this bloats the production binary, if that
matters

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:76: 
doc comments

https://codereview.chromium.org/2433363004/diff/160001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2433363004/diff/160001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:215: // Enables AD functionality
nit: . at end

can we call this active directory (or whatever it means) anywhere?

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/auth_pol...
File chromeos/dbus/auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/auth_pol...
chromeos/dbus/auth_policy_client.h:45: virtual void JoinADDomain(const
std::string& machine_name,
JoinAdDomain

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/fake_aut...
File chromeos/dbus/fake_auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/fake_aut...
chromeos/dbus/fake_auth_policy_client.h:9: 
#include "base/macros.h"

[Roman Sorokin (ftl), 2016-10-31 11:37:11]

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:54: * */
On 2016/10/28 21:07:45, Dan Beam wrote:
> nit: /** @private */

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:66: }
On 2016/10/28 21:07:45, Dan Beam wrote:
> nit: no curlies

Seems like I need it after all

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:80: }
On 2016/10/28 21:07:45, Dan Beam wrote:
> nit:
> 
> this.$.userInput.value = user || '';
> this.$.passwordInput.isInvalid = !!user;

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:96: }
On 2016/10/28 21:07:45, Dan Beam wrote:
> nit: this seems equivalent and shorter
> 
> if (!user.includes('@') && this.userRealm)
>   user += this.userRealm;
> 
> (assuming it actually compiles)

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:20:
"ad-welcome-message:oauthEnrollADDomainJoinWelcomeMessage">
On 2016/10/28 21:13:03, Dan Beam wrote:
> 4\s indent for continuations in HTML

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:63: 
On 2016/10/28 21:13:03, Dan Beam wrote:
> can you document and/or type this?  and maybe add @private?

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:125:
}).bind(this));
On 2016/10/28 21:13:03, Dan Beam wrote:
> these outer () aren't necessary, i.e. you can just do function(e) { ...
> }.bind(this)
> 
> also, this file is odd, but
> 
> this.offlineAdUi_.addEventListener('authCompleted', function(e) {
>   ...
> });
> 
> adds less indent

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:182: if
(this.currentStep_ == STEP_SIGNIN) {
On 2016/10/28 21:13:03, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:126:
LOG(ERROR) << "Failed to write to pipe";
On 2016/10/28 21:13:03, Dan Beam wrote:
> do you want this in release builds?  this bloats the production binary, if
that
> matters

I see there calls in the files. Would that matter if there is on more?

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:76: 
On 2016/10/28 21:13:04, Dan Beam wrote:
> doc comments

Done.

https://codereview.chromium.org/2433363004/diff/160001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2433363004/diff/160001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:215: // Enables AD functionality
On 2016/10/28 21:13:04, Dan Beam wrote:
> nit: . at end
> 
> can we call this active directory (or whatever it means) anywhere?

Done.

We're still trying to fly under the radar.

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/auth_pol...
File chromeos/dbus/auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/auth_pol...
chromeos/dbus/auth_policy_client.h:45: virtual void JoinADDomain(const
std::string& machine_name,
On 2016/10/28 21:13:04, Dan Beam wrote:
> JoinAdDomain

Done.

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/fake_aut...
File chromeos/dbus/fake_auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/160001/chromeos/dbus/fake_aut...
chromeos/dbus/fake_auth_policy_client.h:9: 
On 2016/10/28 21:13:04, Dan Beam wrote:
> #include "base/macros.h"

Done.

[Dan Beam, 2016-10-31 23:52:26]

still lgtm, but if there's any way you can rewrite the stuff of the form

/** @type {...} */(thing.unknownProperty) = blah;

to like:

var thing = /** @type {Thing} */(maybeThing);
thing.value = blah;

that'd be saner, IMO

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:126:
LOG(ERROR) << "Failed to write to pipe";
On 2016/10/31 11:37:11, Roman Sorokin wrote:
> On 2016/10/28 21:13:03, Dan Beam wrote:
> > do you want this in release builds?  this bloats the production binary, if
> that
> > matters
> 
> I see there calls in the files. Would that matter if there is on more?

yes, more calls = more bloat ;)

https://codereview.chromium.org/2433363004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:73: /** @type
{string} */ (this.$.userInput.value) = user || '';
were these needed for closure?  assigning to an expression seems ... dubious

[Alexander Alekseev, 2016-11-01 06:00:02]

Description was changed from

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Alexander Alekseev, 2016-11-01 06:18:32]

lgtm with nit

https://codereview.chromium.org/2433363004/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/2433363004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:120: const
std::string& user,
Could you name "user" better?
We are trying to either move away from std::string as user identifier.
It looks like "user_name" is more specific in this case.

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2433363004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:126:
LOG(ERROR) << "Failed to write to pipe";
On 2016/10/31 23:52:25, Dan Beam wrote:
> On 2016/10/31 11:37:11, Roman Sorokin wrote:
> > On 2016/10/28 21:13:03, Dan Beam wrote:
> > > do you want this in release builds?  this bloats the production binary, if
> > that
> > > matters
> > 
> > I see there calls in the files. Would that matter if there is on more?
> 
> yes, more calls = more bloat ;)

I'd vote for this error to stay. This may fail if user supplies too long
password (maybe we should add password length here too).

[Roman Sorokin (ftl), 2016-11-02 11:20:19]

https://codereview.chromium.org/2433363004/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/2433363004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:120: const
std::string& user,
On 2016/11/01 06:18:32, Alexander Alekseev wrote:
> Could you name "user" better?
> We are trying to either move away from std::string as user identifier.
> It looks like "user_name" is more specific in this case.

Done.

https://codereview.chromium.org/2433363004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:73: /** @type
{string} */ (this.$.userInput.value) = user || '';
On 2016/10/31 23:52:25, Dan Beam wrote:
> were these needed for closure?  assigning to an expression seems ... dubious

Yep, removed that. Can't user GaiaInputElement type since it not refactored into
closure resources.

[michaelpg, 2016-11-02 23:08:54]

a few nits for c/b/r/chromeos and what looks like an unused property

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.html (right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.html:18: 'userRealm' -
Autocomplete realm for the user input.
should we list disabled and adWelcomeMessage?

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.html:22: argument
|credentials| which contains.
nit: period => colon

"which contains:"

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.html:37: <h1
id="welcomeMsg" class="welcome-message">[[adWelcomeMessage]]
Looks like adWelcomeMessage isn't actually used, and is replaced manually in
realmChanged_. Either remove the property, or set the property instead of
this.$.welcomeMsg.textContent.

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:43:
adWelcomeMessage: String
unused? (see above)

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:69: * @param
{!string|undefined} user
nit: ! not required for fundamental types (non-objects/arrays)

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:349:
this.offlineAdUi_.disabled = false;
nit (throughout): can you decide whether you want to use "fooAD" or "fooAd"
style? or is there a good reason to be inconsistent (eg different styles in C++
and JS)?

[Roman Sorokin (ftl), 2016-11-03 13:59:53]

Patchset #12 (id:220001) has been deleted

[Roman Sorokin (ftl), 2016-11-03 14:03:46]

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.html (right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.html:18: 'userRealm' -
Autocomplete realm for the user input.
On 2016/11/02 23:08:53, michaelpg wrote:
> should we list disabled and adWelcomeMessage?

Done.

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.html:22: argument
|credentials| which contains.
On 2016/11/02 23:08:53, michaelpg wrote:
> nit: period => colon
> 
> "which contains:"

Done.

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.html:37: <h1
id="welcomeMsg" class="welcome-message">[[adWelcomeMessage]]
On 2016/11/02 23:08:53, michaelpg wrote:
> Looks like adWelcomeMessage isn't actually used, and is replaced manually in
> realmChanged_. Either remove the property, or set the property instead of
> this.$.welcomeMsg.textContent.

Done.

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/offline_ad_login.js (right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:43:
adWelcomeMessage: String
On 2016/11/02 23:08:53, michaelpg wrote:
> unused? (see above)

Changed it. Will be used more in the next CL (Login flow)

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/offline_ad_login.js:69: * @param
{!string|undefined} user
On 2016/11/02 23:08:53, michaelpg wrote:
> nit: ! not required for fundamental types (non-objects/arrays)

Done.

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:349:
this.offlineAdUi_.disabled = false;
On 2016/11/02 23:08:53, michaelpg wrote:
> nit (throughout): can you decide whether you want to use "fooAD" or "fooAd"
> style? or is there a good reason to be inconsistent (eg different styles in
C++
> and JS)?

Done.

[michaelpg, 2016-11-04 00:34:21]

lgtm

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/2433363004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:349:
this.offlineAdUi_.disabled = false;
On 2016/11/03 14:03:46, Roman Sorokin wrote:
> On 2016/11/02 23:08:53, michaelpg wrote:
> > nit (throughout): can you decide whether you want to use "fooAD" or "fooAd"
> > style? or is there a good reason to be inconsistent (eg different styles in
> C++
> > and JS)?
> 
> Done.

thanks. Makes the code easier to modify when you don't need to guess which
capitalization to use.

https://codereview.chromium.org/2433363004/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/2433363004/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:58: // Shows
the Ad domain joining screen.
nit: keep "AD" for comments :-)

[Roman Sorokin (ftl), 2016-11-04 09:48:21]

https://codereview.chromium.org/2433363004/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/2433363004/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:58: // Shows
the Ad domain joining screen.
On 2016/11/04 00:34:21, michaelpg wrote:
> nit: keep "AD" for comments :-)

Done.

[Roman Sorokin (ftl), 2016-11-04 09:49:17]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2016-11-04 09:49:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
hashimoto@chromium.org, xiyuan@chromium.org, alemate@chromium.org,
dbeam@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2433363004/#ps260001
(title: "nit")

[commit-bot: I haz the power, 2016-11-04 09:49:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 10:39:40]

Description was changed from

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-04 10:39:42]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-11-04 10:41:44]

Description was changed from

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Chromad: added AD Join ui, authpolicy_client

* Added flag --enable-ad (enabling it would add AD join step into
* enrollment flow)
* Added new polymer element with Join/Auth UI.
* Added authpolicy_client

BUG=658228
TEST=compiles
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-11-04 10:41:49]

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

[hashimoto, 2016-11-07 02:34:18]

https://codereview.chromium.org/2433363004/diff/260001/chromeos/dbus/auth_pol...
File chromeos/dbus/auth_policy_client.h (right):

https://codereview.chromium.org/2433363004/diff/260001/chromeos/dbus/auth_pol...
chromeos/dbus/auth_policy_client.h:39: // TODO(rsorokin): Write more descriptive
comment.
Could you resolve this TODO?