MD Settings: cr/cros - Guest mode page visibility

chrome/browser/resources/settings/settings_ui/settings_ui_types.js

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview Closure typedefs for settings_ui.
+ */
+
+/**
+ * Specifies page visibility in guest mode in cr and cros.
+ * @typedef {{
+ *   advancedSettings: (boolean|undefined),

[dpapad, 2016/07/22 18:33:04]

Why are all top-level boolean on this object, undefined or a boolean? Can't we
always give them an appropriate default value, such that nothing is ever
undefined?

[Moe, 2016/07/22 20:53:50]

I've seen this elsewhere too. Look at sync_browser_proxy.js for instance. I
think it's a lot less maintenance to have 'undefined' in the type rather than
maintaining default values for pageVisbility everywhere it is initialized. wdyt?

[dpapad, 2016/07/22 22:40:48]

My thinking was that there is only one place where a GuestModePageVisibility
would be initialized and that is in settings_ui.js. Everywhere else it would be
undefined until the data binding from parent to child takes effect. I also find
it a bit counter-intuitive that a visibility value of undefined means "show this
element". 

+ *   appearance: (boolean|undefined|{
+ *     bookmarksBar: boolean,
+ *     homeButton: boolean,
+ *     pageZoom: boolean,
+ *     setTheme: boolean,
+ *     setWallpaper: boolean,
+ *   }),
+ *   dateTime: (boolean|undefined|{
+ *     timeZoneSelector: boolean,
+ *   }),
+ *   defaultBrowser: (boolean|undefined),
+ *   downloads: (undefined|{
+ *     googleDrive: boolean,
+ *   }),
+ *   onStartup: (boolean|undefined),
+ *   passwordsAndForms: (boolean|undefined),
+ *   people: (boolean|undefined),
+ *   privacy: (undefined|{

[dpapad, 2016/07/22 18:33:04]

If you decide to pass subsets of the GuestModePageVisibility object to specific
pages (instead of always passing the entire object), then it is better to
extract some smaller types here, for example

/** @typedef {{networkPrediction: boolean, searchPrediction: boolean}} */
var PrivacyPageVisibility;

Otherwise how are you going to type annotate |pageVisibility| property of the
privacy page?

[Moe, 2016/07/22 20:53:49]

That's a good idea! but I'm not sure if it's needed now. Currently individual
sections only use pageVisibility in the html to hide/disable controls. Can we
postpone adding the subtypes when they're needed?

[dpapad, 2016/07/22 22:40:48]

I am seeing several |pageVisibility| variables in different HTML files, and they
all have different types (for example date_time_page.html, privacy_page.html).
As a future reader, I will have to trace all the way back to what subset of the
initial GuestModePageVisibility object was assigned to a page's |pageVisibility|
variable to understand its type. I am suggesting either of the following:

1) Pass the entire GuestModePageVisibility object everywhere and declare
|pageVisibility| with an @type {!GuestModePageVisibility} annotation, even if
only a specific path is used. Or
2) Declare smaller types in this file, keep passing subsets of the
GuestModePageVisibility to sub-pages, and annotate them with the appropriate
smaller type.

[Moe, 2016/07/24 23:54:26]

I'd go with option 2. The first option won't work because basic-page and
advanced-page have the pageVisibility property from the SettingsPageVisibility
behavior which is initialized with an empty object. If we bind pageVisibility to
pageVisibility of their children, [[pageVisibility.foo.bar]] will be evaluated
to false and not won't be ignored (which is the behavior we want)

+ *     networkPrediction: boolean,
+ *     searchPrediction: boolean,
+ *   }),
+ *   reset:(boolean|undefined),
+ * }}
+ */
+var GuestModePageVisibility;