MD Settings: Elim SettingsSubpageBrowsertest and SettingsPageVisibility

MD Settings: Elim SettingsSubpageBrowsertest and SettingsPageVisibility

This CL:
* Removes SettingsSubpageBrowsertest which required extra complexity
  in the Settings page visibility code and only tested coarse load
  times of subpages; this testing no longer has much value.
* Eliminates SettingsPageVisibility which was creating multiple
  definitions of pageVisibility and generally confusing things.
* Defines pageVisibility in all cases (not just guest mode). This will
  allow  the use of dom-if and allows conditional pages
  (i.e. androidApps) to use the same pageVisibility system. Pages still
   default to visibile unless explicity set to 'false'.

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

Review-Url: https://codereview.chromium.org/2852433003
Cr-Commit-Position: refs/heads/master@{#468702}
Committed: https://chromium.googlesource.com/chromium/src/+/1362453f54271dc1036fc104a01cad436c3da26c

[stevenjb, 2017-04-27 17:15:54]

Description was changed from

==========
MD Settings: Elim SettingsSubpageBrowsertest and SettingsPageVisibility

This CL:
* Removes SettingsSubpageBrowsertest which required extra complexity
  in the Settings page visibility code and only tested coarse load
  times of subpages; this testing no longer has much value.
* Eliminates SettingsPageVisibility which was creating multiple
  definitions of pageVisibility and generally confusing things.
* Defines pageVisibility in all cases (not just guest mode). This will
  allow  the use of dom-if and allows conditional pages
  (i.e. androidApps) to use the same pageVisibility system. Pages still
   default to visibile unless explicity set to 'false'.

BUG=709442
==========

to

==========
MD Settings: Elim SettingsSubpageBrowsertest and SettingsPageVisibility

This CL:
* Removes SettingsSubpageBrowsertest which required extra complexity
  in the Settings page visibility code and only tested coarse load
  times of subpages; this testing no longer has much value.
* Eliminates SettingsPageVisibility which was creating multiple
  definitions of pageVisibility and generally confusing things.
* Defines pageVisibility in all cases (not just guest mode). This will
  allow  the use of dom-if and allows conditional pages
  (i.e. androidApps) to use the same pageVisibility system. Pages still
   default to visibile unless explicity set to 'false'.

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

[stevenjb, 2017-04-27 17:22:44]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org, dpapad@chromium.org, hcarmona@chromium.org,
michaelpg@chromium.org

[stevenjb, 2017-04-27 17:22:45]

dbeam@ / dpapad@, please review.
hcarmina@ for settings_menu
michaelpg@ - FYI. This *might* help reduce flakiness in
language_page_browsertest.js?

[hcarmona, 2017-05-01 18:18:27]

settings_menu/* LGTM

[stevenjb, 2017-05-01 18:51:30]

stevenjb@chromium.org changed reviewers:
- dbeam@chromium.org

[stevenjb, 2017-05-01 18:51:31]

dpapad@, could you take point on this? Thanks!

[dpapad, 2017-05-01 21:02:22]

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

https://codereview.chromium.org/2852433003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:77:
pageVisibility: Object,
Can you run tryjobs? I am expecting that there might be some more fixes to be
made because some tests might fail. For example
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/settings....

The previous code ensured that |pageVisibility| is initialized, even if you only
create a settings-main element (without a settings-ui parent), such that all
bindings are executing. The new code leaves pageVisibility undefined which might
affect those tests.

https://codereview.chromium.org/2852433003/diff/1/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (left):

https://codereview.chromium.org/2852433003/diff/1/chrome/test/BUILD.gn#oldcod...
chrome/test/BUILD.gn:1044:
"data/webui/settings/settings_subpage_browsertest.js",
Need to resolve conflits with https://codereview.chromium.org/2841863004. This
code has moved to chrome/test/data/webui/BUILD.gn

[stevenjb, 2017-05-01 23:42:58]

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

https://codereview.chromium.org/2852433003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:77:
pageVisibility: Object,
On 2017/05/01 21:02:22, dpapad wrote:
> Can you run tryjobs? I am expecting that there might be some more fixes to be
> made because some tests might fail. For example
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/settings....
> 
> The previous code ensured that |pageVisibility| is initialized, even if you
only
> create a settings-main element (without a settings-ui parent), such that all
> bindings are executing. The new code leaves pageVisibility undefined which
might
> affect those tests.
Ah, I thought I ran all tests locally, but it looks like CrSettingsMainPageTest
is disabled. 

Testing with --gtest_also_run_disabled_tests reveals that test is broken on ToT
and probably has been for a while. I don't see how loading settings-main without
settings-ui would work given all of our prefs dependencies.

I'll "fix" settings_main_test.js, but frankly I don't know whether fixing the
test is worth the effort.

https://codereview.chromium.org/2852433003/diff/1/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (left):

https://codereview.chromium.org/2852433003/diff/1/chrome/test/BUILD.gn#oldcod...
chrome/test/BUILD.gn:1044:
"data/webui/settings/settings_subpage_browsertest.js",
On 2017/05/01 21:02:22, dpapad wrote:
> Need to resolve conflits with https://codereview.chromium.org/2841863004. This
> code has moved to chrome/test/data/webui/BUILD.gn

Acknowledged.

[stevenjb, 2017-05-01 23:43:06]

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

[commit-bot: I haz the power, 2017-05-01 23:43:47]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-05-01 23:56:34]

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

https://codereview.chromium.org/2852433003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:77:
pageVisibility: Object,
> I'll "fix" settings_main_test.js, but frankly I don't know whether fixing the
test is worth the effort.

If the test is disabled, I don't think you have to fix it as part of this CL.
I've seen this pattern a few times now:

1) Test is flaky.
2) Sheriff disables flaky test.
3) Time passes, test remains disabled while code changes.
4) Someone starts looking into re-enabling the test, only to find out that the
test is no longer flaky, it fails consistently instead (the underlying flakiness
root cause might still be there though).

[commit-bot: I haz the power, 2017-05-02 01:04:17]

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

[commit-bot: I haz the power, 2017-05-02 01:04:17]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-05-02 16:39:34]

On 2017/05/01 23:56:34, dpapad wrote:
>
https://codereview.chromium.org/2852433003/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/settings_main/settings_main.js (right):
> 
>
https://codereview.chromium.org/2852433003/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/settings_main/settings_main.js:77:
> pageVisibility: Object,
> > I'll "fix" settings_main_test.js, but frankly I don't know whether fixing
the
> test is worth the effort.
> 
> If the test is disabled, I don't think you have to fix it as part of this CL.
> I've seen this pattern a few times now:
> 
> 1) Test is flaky.
> 2) Sheriff disables flaky test.
> 3) Time passes, test remains disabled while code changes.
> 4) Someone starts looking into re-enabling the test, only to find out that the
> test is no longer flaky, it fails consistently instead (the underlying
flakiness
> root cause might still be there though).

I have also, and it's really out fault for not prioritizing fixing these.

That said, isn't it better to make the change so that if we decide to fix this
we don't also have to chase down this issue?

[dpapad, 2017-05-02 17:37:33]

LGTM

[stevenjb, 2017-05-02 17:41:11]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-05-02 17:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
hcarmona@chromium.org
Link to the patchset: https://codereview.chromium.org/2852433003/#ps20001
(title: "Rebase + fix settings_main_test.js")

[commit-bot: I haz the power, 2017-05-02 17:41:26]

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-05-02 17:47:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493746871126480,
"parent_rev": "91779f820563506601cfc7eedbbb2c6992e33e31", "commit_rev":
"1362453f54271dc1036fc104a01cad436c3da26c"}

[commit-bot: I haz the power, 2017-05-02 17:47:49]

Description was changed from

==========
MD Settings: Elim SettingsSubpageBrowsertest and SettingsPageVisibility

This CL:
* Removes SettingsSubpageBrowsertest which required extra complexity
  in the Settings page visibility code and only tested coarse load
  times of subpages; this testing no longer has much value.
* Eliminates SettingsPageVisibility which was creating multiple
  definitions of pageVisibility and generally confusing things.
* Defines pageVisibility in all cases (not just guest mode). This will
  allow  the use of dom-if and allows conditional pages
  (i.e. androidApps) to use the same pageVisibility system. Pages still
   default to visibile unless explicity set to 'false'.

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

to

==========
MD Settings: Elim SettingsSubpageBrowsertest and SettingsPageVisibility

This CL:
* Removes SettingsSubpageBrowsertest which required extra complexity
  in the Settings page visibility code and only tested coarse load
  times of subpages; this testing no longer has much value.
* Eliminates SettingsPageVisibility which was creating multiple
  definitions of pageVisibility and generally confusing things.
* Defines pageVisibility in all cases (not just guest mode). This will
  allow  the use of dom-if and allows conditional pages
  (i.e. androidApps) to use the same pageVisibility system. Pages still
   default to visibile unless explicity set to 'false'.

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

Review-Url: https://codereview.chromium.org/2852433003
Cr-Commit-Position: refs/heads/master@{#468702}
Committed:
https://chromium.googlesource.com/chromium/src/+/1362453f54271dc1036fc104a01c...
==========

[commit-bot: I haz the power, 2017-05-02 17:47:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1362453f54271dc1036fc104a01c...

[stevenjb, 2017-05-04 17:40:02]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2866463002/ by stevenjb@chromium.org.

The reason for reverting is: This caused pageVisibility sub-objects to no longer
be undefined which (counter intuitively) causes the sub sections to no longer be
visible.

There was some discussion about various approaches to fix this here:
https://codereview.chromium.org/2861443003/

However no agreement was reached, so I am reverting this CL.
.