MD Settings: remove unsupported routes from guest-mode.

MD Settings: remove unsupported routes from guest-mode.

Currently in guest mode, there are valid routes that are not visible, which leads to odd behavior when directly visiting those routes. This CL aims to prevent those kinds of routes from being added into settings.Route during initialization.

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

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

[scottchen, 2017-06-28 01:08:13]

Description was changed from

==========
MD Settings: remove unsupported routes from guest-mode.

Currently in guest mode, there are valid routes that are not visible, which
leads to odd behavior when directly visiting those routes. This CL aims to
prevent those kinds of routes from being added into settings.Route during
initialization.

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

to

==========
MD Settings: remove unsupported routes from guest-mode.

Currently in guest mode, there are valid routes that are not visible, which
leads to odd behavior when directly visiting those routes. This CL aims to
prevent those kinds of routes from being added into settings.Route during
initialization.

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

[scottchen, 2017-06-28 01:08:19]

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:08: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:33]

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

[commit-bot: I haz the power, 2017-06-28 01:31:27]

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

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

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 21:37:14]

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

[commit-bot: I haz the power, 2017-06-28 21:37:43]

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, 2017-06-28 23:18:50]

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

[commit-bot: I haz the power, 2017-06-28 23:18:51]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-06-29 00:57:32]

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

https://codereview.chromium.org/2957153003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:118: if
(settings.pageVisibility.appearance !== false) {
Can we add some tests at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/route_te...
to ensure that route.js respects settings.pageVisibility?

[scottchen, 2017-07-01 01:22:07]

Patchset #8 (id:140001) has been deleted

[scottchen, 2017-07-06 00:57:38]

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

[commit-bot: I haz the power, 2017-07-06 00:57:54]

Dry run: CQ is trying da patch.

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

[scottchen, 2017-07-06 01:02:24]

Based on offline discussions, I refactored how routes were setup to have better
test-ability and maintainability moving forward. PTAL.

I also added safeguards in the code for anywhere we're dereferencing a route
that might/might not be defined. You can look through all of them by looking at
diffs between patch 9 and 8.

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

https://codereview.chromium.org/2957153003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:118: if
(settings.pageVisibility.appearance !== false) {
On 2017/06/29 00:57:32, dpapad wrote:
> Can we add some tests at
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/route_te...
> to ensure that route.js respects settings.pageVisibility?

Done.

[commit-bot: I haz the power, 2017-07-06 01:46:28]

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

[commit-bot: I haz the power, 2017-07-06 01:46:29]

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_...)

[dpapad, 2017-07-06 02:07:06]

Mostly nits.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:6: * Specifies available routes in
settings.
Nit: Can we clarify this comment a bit?
s/available/all possible

Per my understanding this specifies all possible routes, without taking into
account the current state (like guest mode). Meaning that the routes in this
typedef are not necessarily existing on |settings.routes| at runtime.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:100: * @param {string} path
Nit: Maybe shorten as follows:

/** @param {string} path */

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:186: /** @private {!SettingsRoutes}
*/
Let's add a comment to differentiate between routes_ and SettingsRoutes.
Something like

"List of available routes. This is populated taking into account current state
(like guest mode)."

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:187: this.routes_ = {};
Not sure if this is worth doing, but mentioning it for discussion.

1) We could simply return a new object from initializeRoutes().
this.routes_ = this.initializeRoutes_();
2) Given on 1, initializeRoutes, does not access |this| anymore, so it does not
even need to be an instance method, so we could simply move it outside of this
class (and possibly rename), as follows:

this.routes_ = computeAvailableRoutes();

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:211: initializeRoutes() {
Can this be private?

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:538: var Router_ = new Router();
Typically only class names start with a capital letter. Let's rename this to
router_ or instance_ (denoting the singleton instance).

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:540: var getRouter = function() {
Is this used anywhere?

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:590: // out why doing so causes a
closure compiler error.
Maybe update the comment to mention our newest finding. Something like
"ChromePass limits the syntax of what can be returned from cr.define()."

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:592: 
Maybe add a TODO as follows?

"Stop exposing all those methods directly on settings.*, and instead update all
clients to use the router_ instance directly".

[scottchen, 2017-07-07 20:58:34]

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

[scottchen, 2017-07-07 20:58:45]

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:6: * Specifies available routes in
settings.
On 2017/07/06 02:07:06, dpapad wrote:
> Nit: Can we clarify this comment a bit?
> s/available/all possible
> 
> Per my understanding this specifies all possible routes, without taking into
> account the current state (like guest mode). Meaning that the routes in this
> typedef are not necessarily existing on |settings.routes| at runtime.

Done.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:100: * @param {string} path
On 2017/07/06 02:07:06, dpapad wrote:
> Nit: Maybe shorten as follows:
> 
> /** @param {string} path */

Done.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:186: /** @private {!SettingsRoutes}
*/
On 2017/07/06 02:07:06, dpapad wrote:
> Let's add a comment to differentiate between routes_ and SettingsRoutes.
> Something like
> 
> "List of available routes. This is populated taking into account current state
> (like guest mode)."

Done.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:187: this.routes_ = {};
On 2017/07/06 02:07:06, dpapad wrote:
> Not sure if this is worth doing, but mentioning it for discussion.
> 
> 1) We could simply return a new object from initializeRoutes().
> this.routes_ = this.initializeRoutes_();
> 2) Given on 1, initializeRoutes, does not access |this| anymore, so it does
not
> even need to be an instance method, so we could simply move it outside of this
> class (and possibly rename), as follows:
> 
> this.routes_ = computeAvailableRoutes();

I was going to use Router_.initializeRoutes() in the test to re-initiate all the
routes after fiddling with settings.pageVisibility, in which case having it as
an instance method made sense. 

But then I realized it didn't reset this.routes_ to start from scratch anyway,
so I changed to just make a whole new Router() in the test instead. Given this,
I can change this part to be as you suggested.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:211: initializeRoutes() {
On 2017/07/06 02:07:06, dpapad wrote:
> Can this be private?

Moving this method out of the class definition.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:538: var Router_ = new Router();
On 2017/07/06 02:07:06, dpapad wrote:
> Typically only class names start with a capital letter. Let's rename this to
> router_ or instance_ (denoting the singleton instance).

Done. Changed to routerInstance because apparently presubmit doesn't like
lowercaseWithUnderscore_.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:540: var getRouter = function() {
On 2017/07/06 02:07:06, dpapad wrote:
> Is this used anywhere?

not anymore, removing.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:590: // out why doing so causes a
closure compiler error.
On 2017/07/06 02:07:06, dpapad wrote:
> Maybe update the comment to mention our newest finding. Something like
> "ChromePass limits the syntax of what can be returned from cr.define()."

Done.

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:592: 
On 2017/07/06 02:07:06, dpapad wrote:
> Maybe add a TODO as follows?
> 
> "Stop exposing all those methods directly on settings.*, and instead update
all
> clients to use the router_ instance directly".

Done.

[commit-bot: I haz the power, 2017-07-07 20:59:01]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-07-07 21:19:30]

LGTM

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2957153003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:187: this.routes_ = {};
On 2017/07/07 at 20:58:44, scottchen wrote:
> On 2017/07/06 02:07:06, dpapad wrote:
> > Not sure if this is worth doing, but mentioning it for discussion.
> > 
> > 1) We could simply return a new object from initializeRoutes().
> > this.routes_ = this.initializeRoutes_();
> > 2) Given on 1, initializeRoutes, does not access |this| anymore, so it does
not
> > even need to be an instance method, so we could simply move it outside of
this
> > class (and possibly rename), as follows:
> > 
> > this.routes_ = computeAvailableRoutes();
> 
> I was going to use Router_.initializeRoutes() in the test to re-initiate all
the routes after fiddling with settings.pageVisibility, in which case having it
as an instance method made sense. 
> 
> But then I realized it didn't reset this.routes_ to start from scratch anyway,
so I changed to just make a whole new Router() in the test instead. Given this,
I can change this part to be as you suggested.

SG. Consider this optional.

https://codereview.chromium.org/2957153003/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/settings/route_tests.js (right):

https://codereview.chromium.org/2957153003/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/route_tests.js:218: var router = new
settings.Router();
Nit (optional): Maybe avoid repetition as follows?

var hasRoute = route => router.getRoutes().hasOwnProperty(route);

assertTrue(hasRoute('BASIC'));
...

[commit-bot: I haz the power, 2017-07-07 21:22:32]

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

[commit-bot: I haz the power, 2017-07-07 21:22:33]

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-07-07 22:15:27]

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

[scottchen, 2017-07-07 22:15:30]

https://codereview.chromium.org/2957153003/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/settings/route_tests.js (right):

https://codereview.chromium.org/2957153003/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/route_tests.js:218: var router = new
settings.Router();
On 2017/07/07 21:19:30, dpapad wrote:
> Nit (optional): Maybe avoid repetition as follows?
> 
> var hasRoute = route => router.getRoutes().hasOwnProperty(route);
> 
> assertTrue(hasRoute('BASIC'));
> ...

Done.

[commit-bot: I haz the power, 2017-07-07 22:15:48]

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, 2017-07-07 23:31:33]

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

[commit-bot: I haz the power, 2017-07-07 23:31:34]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-07-08 00:05:35]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-07-08 00:05:35]

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/2957153003/#ps260001
(title: "fix compiler error")

[commit-bot: I haz the power, 2017-07-08 00:06:18]

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-07-08 00:18:59]

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

[commit-bot: I haz the power, 2017-07-08 00:19:00]

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

[scottchen, 2017-07-10 17:49:47]

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

[scottchen, 2017-07-10 17:49:47]

+ stevenjb@, needs owner LG for
"chrome/browser/resources/options/compiled_resources2.gyp".

[stevenjb, 2017-07-10 18:01:25]

chromeos/compiled_resources2.gyp lgtm

https://codereview.chromium.org/2957153003/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2957153003/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:612: Router: Router,          // The
class definition.
tiny nit: S/The class/Class/ for both of these since there is more than one
class?

[scottchen, 2017-07-10 18:19:33]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-07-10 18:19:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2957153003/#ps280001
(title: "merge")

[commit-bot: I haz the power, 2017-07-10 18:19:43]

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-07-10 19:52:44]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499710773808820,
"parent_rev": "90a387f2b11ef858d6cff5f5b8779f380954b170", "commit_rev":
"c31781b0a4f7137b4b51919086092404594bce24"}

[commit-bot: I haz the power, 2017-07-10 19:52:58]

Description was changed from

==========
MD Settings: remove unsupported routes from guest-mode.

Currently in guest mode, there are valid routes that are not visible, which
leads to odd behavior when directly visiting those routes. This CL aims to
prevent those kinds of routes from being added into settings.Route during
initialization.

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

to

==========
MD Settings: remove unsupported routes from guest-mode.

Currently in guest mode, there are valid routes that are not visible, which
leads to odd behavior when directly visiting those routes. This CL aims to
prevent those kinds of routes from being added into settings.Route during
initialization.

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

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

[commit-bot: I haz the power, 2017-07-10 19:53:00]

Committed patchset #14 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/c31781b0a4f7137b4b5191908609...

[scottchen, 2017-07-10 20:54:36]

https://codereview.chromium.org/2957153003/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2957153003/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:612: Router: Router,          // The
class definition.
On 2017/07/10 18:01:25, stevenjb wrote:
> tiny nit: S/The class/Class/ for both of these since there is more than one
> class?

Acknowledged.