MD Settings: decouple page visibility from settings_ui.html.

MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace, so
that it's easier to manage and can be accessed by other parts of the program. This is
the beginning of reworking guest-mode routing in order to fix 733511, and there will
be a follow-up CL to further utilize the new page_visibility files.

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

Review-Url: https://codereview.chromium.org/2962823002
Cr-Commit-Position: refs/heads/master@{#483471}
Committed: https://chromium.googlesource.com/chromium/src/+/03a2ed59818d98677f9f6f6992da70a328edcf6c

[scottchen, 2017-06-28 00:05:35]

Description was changed from

==========
MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace,
so that it's easier to manage and can be accessed by other parts of the program.
This is the beginning of reworking guest-mode routing in order to fix 733511,
and there will be a follow-up CL to further utilize the new page_visibility
files.

BUG=733511
==========

to

==========
MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace,
so that it's easier to manage and can be accessed by other parts of the program.
This is the beginning of reworking guest-mode routing in order to fix 733511,
and there will be a follow-up CL to further utilize the new page_visibility
files.

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

[scottchen, 2017-06-28 01:09:17]

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

[commit-bot: I haz the power, 2017-06-28 01:09:43]

Dry run: CQ is trying da patch.

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

[scottchen, 2017-06-28 01:10:17]

scottchen@chromium.org changed reviewers:
+ dpapad@chromium.org

[dpapad, 2017-06-28 01:16:12]

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/page_visibility.js (right):

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/page_visibility.js:5: cr.define('settings',
function() {
Can you add a compile target for this file?

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/page_visibility.js:10: PageVisibility =
Object.assign(PageVisibility, {
Is the call to Object.assign necessary here? Isn't PageVisibility always empty
at this point?

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/page_visibility.js:40: return {PageVisibility:
PageVisibility};
Nit: Usually upper case is used to name a class, not an instance of a class.
Should this be
{pageVisibility: pageVisibility} instead?

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_ui/settings_ui.js:59: value:
function() {
Is the function() wrapper necessary? Can this be simply

value: settings.PageVisibility,

[commit-bot: I haz the power, 2017-06-28 01:34:22]

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

[commit-bot: I haz the power, 2017-06-28 01:34:23]

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

[scottchen, 2017-06-28 20:51:43]

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

[scottchen, 2017-06-28 20:51:49]

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/page_visibility.js (right):

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/page_visibility.js:5: cr.define('settings',
function() {
On 2017/06/28 01:16:11, dpapad wrote:
> Can you add a compile target for this file?

Done.

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/page_visibility.js:10: PageVisibility =
Object.assign(PageVisibility, {
On 2017/06/28 01:16:11, dpapad wrote:
> Is the call to Object.assign necessary here? Isn't PageVisibility always empty
> at this point?

Acknowledged.

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/page_visibility.js:40: return {PageVisibility:
PageVisibility};
On 2017/06/28 01:16:11, dpapad wrote:
> Nit: Usually upper case is used to name a class, not an instance of a class.
> Should this be
> {pageVisibility: pageVisibility} instead?

Done.

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_ui/settings_ui.js (right):

https://codereview.chromium.org/2962823002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_ui/settings_ui.js:59: value:
function() {
On 2017/06/28 01:16:11, dpapad wrote:
> Is the function() wrapper necessary? Can this be simply
> 
> value: settings.PageVisibility,

Done.

[commit-bot: I haz the power, 2017-06-28 20:52:13]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-06-28 21:48:41]

LGTM. Perhaps make the CL description limited to 80columns (not sure if this is
a hard rule, but looking at git history, most CLs follow that convention).

https://codereview.chromium.org/2962823002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/route.html (right):

https://codereview.chromium.org/2962823002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.html:3: <link rel="import"
href="page_visibility.html">
This should be in the next CL, since route.js is not referring to pageVisibility
yet.

[commit-bot: I haz the power, 2017-06-28 22:19:27]

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

[commit-bot: I haz the power, 2017-06-28 22:19:29]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-06-29 18:57:08]

Description was changed from

==========
MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace,
so that it's easier to manage and can be accessed by other parts of the program.
This is the beginning of reworking guest-mode routing in order to fix 733511,
and there will be a follow-up CL to further utilize the new page_visibility
files.

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

to

==========
MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace,
so
that it's easier to manage and can be accessed by other parts of the program.
This is
the beginning of reworking guest-mode routing in order to fix 733511, and there
will
be a follow-up CL to further utilize the new page_visibility files.

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

[scottchen, 2017-06-29 18:57:21]

https://codereview.chromium.org/2962823002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/route.html (right):

https://codereview.chromium.org/2962823002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.html:3: <link rel="import"
href="page_visibility.html">
On 2017/06/28 21:48:41, dpapad wrote:
> This should be in the next CL, since route.js is not referring to
pageVisibility
> yet.

Acknowledged.

[scottchen, 2017-06-29 18:57:26]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-06-29 18:57:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2962823002/#ps60001
(title: "moving import out of this CL")

[commit-bot: I haz the power, 2017-06-29 18:57:58]

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-29 20:29:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498762646775800,
"parent_rev": "7bda9a5b4c91eb33d4ffe8e6bf63d188c2b75a11", "commit_rev":
"f04bf571e06ecbcdb147db6d3910cc57b6ea6f04"}

[commit-bot: I haz the power, 2017-06-29 20:30:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498762646775800,
"parent_rev": "81e96954be78ebe12476c548b678986db0fc04b4", "commit_rev":
"03a2ed59818d98677f9f6f6992da70a328edcf6c"}

[commit-bot: I haz the power, 2017-06-29 20:30:25]

Description was changed from

==========
MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace,
so
that it's easier to manage and can be accessed by other parts of the program.
This is
the beginning of reworking guest-mode routing in order to fix 733511, and there
will
be a follow-up CL to further utilize the new page_visibility files.

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

to

==========
MD Settings: decouple page visibility from settings_ui.html.

Move PageVisibility out of settings_ui and into the global 'settings' namespace,
so
that it's easier to manage and can be accessed by other parts of the program.
This is
the beginning of reworking guest-mode routing in order to fix 733511, and there
will
be a follow-up CL to further utilize the new page_visibility files.

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

Review-Url: https://codereview.chromium.org/2962823002
Cr-Commit-Position: refs/heads/master@{#483471}
Committed:
https://chromium.googlesource.com/chromium/src/+/03a2ed59818d98677f9f6f6992da...
==========

[commit-bot: I haz the power, 2017-06-29 20:30:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/03a2ed59818d98677f9f6f6992da...