Add internals page for DICE

Add internals page for DICE

This CL adds an internals page that will be used to build and debug
the desktop identity consistency feature. The current implementation
just does the piping and presents a simple page that has an Enable
Sync button that does not have any action attached to it.

This initial goal is to be able to make progress on the feature before
the mocks are final and approved by UI review.

BUG=733226
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2943093002
Cr-Commit-Position: refs/heads/master@{#480855}
Committed: https://chromium.googlesource.com/chromium/src/+/c3d4e9cac52f4280d430bd0098b6aee35f355e14

[msarda, 2017-06-16 09:25:11]

Description was changed from

==========
Add internals page for DICE

This CL adds an internals page that will be used to build and debug
the desktop identity consistency feature. The current implementation
just does the piping and presents a simple page that has an Enable
Sync button that does not have any action attached to it.

This initial goal is to be able to make progress on the feature before
the mocks are final and approved by UI review.

BUG=733226
==========

to

==========
Add internals page for DICE

This CL adds an internals page that will be used to build and debug
the desktop identity consistency feature. The current implementation
just does the piping and presents a simple page that has an Enable
Sync button that does not have any action attached to it.

This initial goal is to be able to make progress on the feature before
the mocks are final and approved by UI review.

BUG=733226
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[msarda, 2017-06-16 12:50:41]

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

[commit-bot: I haz the power, 2017-06-16 12:50:45]

Dry run: CQ is trying da patch.

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

[msarda, 2017-06-16 12:53:22]

Patchset #1 (id:1) has been deleted

[msarda, 2017-06-16 12:54:39]

msarda@chromium.org changed reviewers:
+ bauerb@chromium.org, droger@chromium.org

[msarda, 2017-06-16 12:54:39]

David: Please do a review of the entire CL.

Bernhard: Please review changes in files:
  chrome\browser\browser_resources.grd
 
chrome\browser\resources\signin\signin_dice_internals\signin_dice_internals.html
  chrome\browser\resources\signin\signin_dice_internals\signin_dice_internals.js
  chrome\browser\ui\webui\chrome_web_ui_controller_factory.cc

Thank you.

[commit-bot: I haz the power, 2017-06-16 13:09:21]

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

[commit-bot: I haz the power, 2017-06-16 13:09:22]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[msarda, 2017-06-16 13:22:40]

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

[commit-bot: I haz the power, 2017-06-16 13:22:52]

Dry run: CQ is trying da patch.

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

[droger, 2017-06-16 13:29:15]

lgtm

Note that I'm not expert with html and javascript files.

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:465: <include
name="IDR_SIGNIN_DICE_INTERNALS_JS"
file="resources\signin\signin_dice_internals\signin_dice_internals.js"
type="BINDATA" />
I don't know what BINDATA means exactly, but I see that a lot of other files
have "chrome_html" instead. Do you know what is the difference?

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.js
(right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.js:1:
/* Copyright 2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc (right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc:25: //
TODO(msard): Implement start syncing.
msarda

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_dice_internals_ui.h (right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_dice_internals_ui.h:14: explicit
SigninDiceInternalsUI(content::WebUI* web_ui);
Nit: forward declaration for WebUI.

[Bernhard Bauer, 2017-06-16 14:16:08]

High-level comment: "DICE" out of context is a bit hard to understand. I think
it would be friendly to at least make the connection between the acronym and its
expansion in a few more places where someone might come across it and not know
what it refers to.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html
(right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html:1:
<!doctype html>
Doesn't this also need a copyright header?

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html:2:
<html i18n-values="dir:textdirection;lang:language">
This page isn't localized, right? Then you can remove the i18n-values attribute.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html:3:
<head>
Usually we just skip the indentation for <head> and <body>.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.js
(right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.js:17:
initialize: initialize
Nit: Maybe add a comma at the end so you can add more entries later without
having to touch unrelated methods.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:400: // Desktop
Identity Consistency is only availble on Windows, Linux and macOS.
Nit: "available".

https://codereview.chromium.org/2943093002/diff/40001/chrome/common/url_const...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/common/url_const...
chrome/common/url_constants.cc:164: const char
kChromeUISigninDiceInternalsHost[] = "signin-dice-internals";
I think hosts go in the block below (and kChromeUIDiscardsHost is Doing It
Wrong).

[commit-bot: I haz the power, 2017-06-16 15:10:20]

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

[commit-bot: I haz the power, 2017-06-16 15:10:21]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-06-20 10:13:54]

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

[commit-bot: I haz the power, 2017-06-20 10:14:09]

Dry run: CQ is trying da patch.

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

[msarda, 2017-06-20 10:16:44]

David: I'll use the new flag ENABLE_DICE_SUPPORT that you just added instead of
platform if-defs once your CL lands.

Bernhard: You are right that the DICE acronym is cryptic - it means Desktop
Identity Consistency. We'll try to me more explicit in the future. However, the
design also involves changes to internal systems so we cannot make it public
now.
We send a while back a design document for this on an internal group. You can
also find out more at go/project-dice.

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:465: <include
name="IDR_SIGNIN_DICE_INTERNALS_JS"
file="resources\signin\signin_dice_internals\signin_dice_internals.js"
type="BINDATA" />
On 2017/06/16 13:29:15, droger wrote:
> I don't know what BINDATA means exactly, but I see that a lot of other files
> have "chrome_html" instead. Do you know what is the difference?

I do not know, but the nearby html files all use BINDATA. I'll keep it as is.

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.js
(right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.js:1:
/* Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/06/16 13:29:15, droger wrote:
> 2017

Done.

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc (right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc:25: //
TODO(msard): Implement start syncing.
On 2017/06/16 13:29:15, droger wrote:
> msarda

Done.

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_dice_internals_ui.h (right):

https://codereview.chromium.org/2943093002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_dice_internals_ui.h:14: explicit
SigninDiceInternalsUI(content::WebUI* web_ui);
On 2017/06/16 13:29:15, droger wrote:
> Nit: forward declaration for WebUI.

Not needed s it includes WebUIController that already has it.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html
(right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html:1:
<!doctype html>
On 2017/06/16 14:16:08, Bernhard Bauer wrote:
> Doesn't this also need a copyright header?

The other HTML files in chrome/browser/resources/signin do not have a copyright
header. See
https://cs.chromium.org/search/?q=chrome/browser/resources/+html+package:%5Ec...

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html:2:
<html i18n-values="dir:textdirection;lang:language">
On 2017/06/16 14:16:08, Bernhard Bauer wrote:
> This page isn't localized, right? Then you can remove the i18n-values
attribute.

Done.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_dice_internals/signin_dice_internals.html:3:
<head>
On 2017/06/16 14:16:08, Bernhard Bauer wrote:
> Usually we just skip the indentation for <head> and <body>.

The other files in chrome/browser/resources/signin have the indent. I'll keep it
here for consistency.

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:400: // Desktop
Identity Consistency is only availble on Windows, Linux and macOS.
On 2017/06/16 14:16:08, Bernhard Bauer wrote:
> Nit: "available".

Done.

https://codereview.chromium.org/2943093002/diff/40001/chrome/common/url_const...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/2943093002/diff/40001/chrome/common/url_const...
chrome/common/url_constants.cc:164: const char
kChromeUISigninDiceInternalsHost[] = "signin-dice-internals";
On 2017/06/16 14:16:08, Bernhard Bauer wrote:
> I think hosts go in the block below (and kChromeUIDiscardsHost is Doing It
> Wrong).

Done.

[commit-bot: I haz the power, 2017-06-20 10:26:19]

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

[commit-bot: I haz the power, 2017-06-20 10:26:19]

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

[Bernhard Bauer, 2017-06-20 12:49:17]

lgtm

[msarda, 2017-06-20 15:25:00]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-06-20 15:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2943093002/#ps80001
(title: "Fix compile error")

[commit-bot: I haz the power, 2017-06-20 15:25:13]

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, 2017-06-20 16:45:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497972300247340,
"parent_rev": "ecbe022c24e066b460128af99a5eca75ee9f1690", "commit_rev":
"c3d4e9cac52f4280d430bd0098b6aee35f355e14"}

[commit-bot: I haz the power, 2017-06-20 16:46:06]

Description was changed from

==========
Add internals page for DICE

This CL adds an internals page that will be used to build and debug
the desktop identity consistency feature. The current implementation
just does the piping and presents a simple page that has an Enable
Sync button that does not have any action attached to it.

This initial goal is to be able to make progress on the feature before
the mocks are final and approved by UI review.

BUG=733226
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add internals page for DICE

This CL adds an internals page that will be used to build and debug
the desktop identity consistency feature. The current implementation
just does the piping and presents a simple page that has an Enable
Sync button that does not have any action attached to it.

This initial goal is to be able to make progress on the feature before
the mocks are final and approved by UI review.

BUG=733226
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2943093002
Cr-Commit-Position: refs/heads/master@{#480855}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3d4e9cac52f4280d430bd0098b6...
==========

[commit-bot: I haz the power, 2017-06-20 16:46:08]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c3d4e9cac52f4280d430bd0098b6...