Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: May 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 712781



Sign in to add a comment
Chrome OS autotests depend on current Settings WebUI DOM
Project Member Reported by kinaba@chromium.org, Feb 20 2017 Back to list
"Could not locate section in chrome://settings to enable arc. Make sure ARC is available."

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq
https://uberchromegw.corp.google.com/i/chromeos/builders/cyan-chrome-pfq

It is failing to roll 58.0.3016
https://chromium.googlesource.com/chromium/src/+log/58.0.3015.0..58.0.3016.0?pretty=fuller&n=10000
which seems to flip MD settings https://codereview.chromium.org/2699973002 that I suspect at the first glance of the revision log.


+ARC Constables
 
Comment 1 by kinaba@chromium.org, Feb 20 2017
Cc: alemate@chromium.org domlasko...@chromium.org
+Chrome on ChromeOS gardeners
Comment 2 by kinaba@chromium.org, Feb 20 2017
Cc: za...@chromium.org sha...@chromium.org dbeam@chromium.org rongchang@chromium.org
+dbeam@, and +CrOSSheriffs just in case.

MD settings indeed looks changing the DOM structure,
so inevitably making this kind of code
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/client/common_lib/cros/arc_util.py#150
incompatible.

Searching through the code, there are seems to be several other ChromeOS autotests traversing the settings page in this way, not only ARC tests.
Project Member Comment 3 by bugdroid1@chromium.org, Feb 21 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7d491d6a26d61a06f96d8e924c54902e5c52ed4

commit d7d491d6a26d61a06f96d8e924c54902e5c52ed4
Author: kinaba <kinaba@chromium.org>
Date: Tue Feb 21 01:48:50 2017

Revert of MD Settings: enable by default in client (patchset #4 id:60001 of https://codereview.chromium.org/2699973002/ )

Reason for revert:
BUG= 694081 

It broke ChromeOS tests and thus blocked Chrome roll to ChromeOS.
In my personal opinion, what really needs fix are the tests rather than the change,
but the changes to the tests looks far nontrivial.
For now, allow me to revert this change to unblock the divergence between Chrome and ChromeOS ToT...

Original issue's description:
> MD Settings: enable by default in client
>
> R=dpapad@chromium.org
> BUG=478982
>
> Review-Url: https://codereview.chromium.org/2699973002
> Cr-Commit-Position: refs/heads/master@{#451338}
> Committed: https://chromium.googlesource.com/chromium/src/+/a8109d958f144e541a204da6debd820d936d6108

TBR=dpapad@chromium.org,sky@chromium.org,dbeam@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=478982

Review-Url: https://codereview.chromium.org/2703283002
Cr-Commit-Position: refs/heads/master@{#451684}

[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/browser_about_handler_unittest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/chrome_content_browser_client_browsertest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/certificate_manager_browsertest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/language_dictionary_interactive_uitest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/language_options_interactive_uitest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/options_ui_browsertest.cc
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/options_ui_browsertest.h
[modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/common/chrome_features.cc

Comment 4 by kinaba@chromium.org, Feb 21 2017
Labels: -Pri-0 M-58 Pri-1
Summary: MD Settings breaks Chromium OS autotests. (was: Chrome PFQ on ARC bots failing to enable ARC in settings.)
(lowering to P1 since now the revert is in.)
Comment 5 by kinaba@chromium.org, Feb 21 2017
Owner: dbeam@chromium.org
Status: Assigned
I'm feeling sorry about that, but for the health of the tree I reverted the change (as a fill-in constable of dspaid@.)

Now, I'm not sure about the right next step. Passing to dbeam@.
The point is, multiple Chrome OS tests are relying on injecting JS to the chrome://settings page and modifying the settings:

https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/common_lib/cros/arc_util.py#149
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/site_tests/policy_AutoFillEnabled/policy_AutoFillEnabled.py#46
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/cros/enterprise/enterprise_policy_base.py#438
...

Without making these tests robust to the settings page change, the tree will continue rejecting Chrome with MD settings.
Cc: drinkcat@chromium.org lhchavez@chromium.org derat@chromium.org achuith@chromium.org
Owner: steve...@chromium.org
Wow. This is incredibly fragile, we really need to fix the test:

settings_tab.ExecuteJavaScript(
                    'assert(document.getElementById("android-apps-enabled"))'

dbeam@ - I will take ownership of this. You may want to enable MD Settings on ToT for non cros in the meanwhile.

Cc: kathrelk...@chromium.org
Yup, we should use explicit test apis rather than parsing DOM in autotests
Comment 9 by dbeam@chromium.org, Feb 21 2017
stevenjb@: if you wouldn't just delete the test when we roll to settings, it's probably worth updating.  otherwise, I'd just recommend slapping a ScopedFeatureList in that test's SetUp() (or one of the initialization methods) and calling it day

meanwhile, I'm going to enable this by default everywhere but ChromeOS.
dbeam@ - Unfortunately it's more complicated than that. This is a python based autotest in the chromeos code, which is why it did not show up in the waterfall.

It looks like this is used by a bunch of Arc++ tests, so we need an owner and we need a more robust fix. The problems with the current mechanism were discussed here, but not followed up: https://chromium-review.googlesource.com/c/412286/


Cc: elijahtaylor@chromium.org
+elijahtaylor@ - Any suggestions for an owner for this? We really do not want arc (or any) auto tests depending on Chrome UI code; it is very difficult to maintain (and shouldn't be something that the Chrome devs need to be concerned about).


Labels: -Pri-1 Pri-0
Status: Available
Summary: Chrome OS autotests depend on current Settings WebUI DOM (was: MD Settings breaks Chromium OS autotests.)
Renaming and marking available. We need an owner for this who is familiar with the arc autotests.

Also upping the priority because this is preventing us from enabling MD Settings by default on CrOS.

Comment 13 by khmel@chromium.org, Feb 21 2017
Owner: khmel@chromium.org
Status: Started
I am ARC constable this week and can take a look. What is preferable way you think of fixing this? One idea is to make it aware about different Chrome UIs (however this might be not best way). 
achuith@, don''t we have a set of chrome hooks for this sort of thing?

All that code is doing, as near as I can tell, is setting the arc.enabled pref. We shouldn't need to execute UI code to do that.



Cc: scunning...@chromium.org
+scunningham@

Quickly code-searched. Probably the ARC test is the only tree closer, but IIUC enterprise tests are also largely depending on the settings page DOM.
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/cros/enterprise/enterprise_policy_base.py#438
Cc: -scunning...@chromium.org
Scott has left the team, but I'm a good poc for Enterprise autotests.

Using the settings page to enable things is a workaround for features without public APIs.  The Enterprise tests in #15 are checking whether a setting was enabled/disabled after a given policy is set.

A solution is we've used in the past is expanding the autotest_private to include the needed functionality: [Chrome]/src/chrome/browser/extensions/api/autotest_private/.  See the touch setting functions which are used here: [Chrome OS]/src/third_party/autotest/files/client/cros/touch_playback_test_base.py
Comment 17 by khmel@chromium.org, Feb 22 2017
For short ARC fix I suggest: https://chromium-review.googlesource.com/c/446090 
This just switches chrome://settings to chrome://settings-frame (last one is not MD based).

I know, this is quite hacky way but I have not found better way so far. 

#16 looks reasonable but this will take more time.

Cc: victorhsieh@chromium.org
Victor added a notifications api to autotestPrivate: https://codereview.chromium.org/1872873002
This is used by ARC tests.

This is the preferred way for communication between chrome and autotests.

We can have a similar treatment and introduce an api like autotestPrivate.androidAppsEnabled().

Another option that's a lot simpler is to just add a JS call to to the settings page, as I did here for the the chrome://gpu page:
https://codereview.chromium.org/1430993002/

We can add a js method like AndroidAppsEnabledForTesting(), and call that from telemetry.

The first solution is probably a little nicer since it doesn't involve opening the settings page, but it may be more work.

I'd rather not use the chrome://settings-frame approach since it just kicks the problem down the road a bit. It's fine to land to get the tree green.
Please name the API something like autotestPrivate.playStoteEnabled instead
if we decide to go this way.
It may be possible to just use chrome.settingsPrivate.setPref('arc.enabled', true, '', null). You may need to whitelist settingsPrivate for autotests.

Alternatively autotestPrivate.setPlayStoreEnabled(true) would also be fine.

I would very much like to see us move away from loading chrome://setings (or any WebUI page) for autotests. It is both fragile and not especially fast.

Project Member Comment 21 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Comment 22 by khmel@chromium.org, Feb 27 2017
Labels: -Pri-0 Pri-1
Lowering the priority, once quick fix was landed.


Project Member Comment 23 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Project Member Comment 24 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Comment 25 by za...@chromium.org, Feb 27 2017
Cc: -za...@chromium.org
Project Member Comment 26 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Project Member Comment 27 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Project Member Comment 28 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Project Member Comment 29 by bugdroid1@chromium.org, Feb 27 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1

commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1
Author: khmel <khmel@google.com>
Date: Mon Feb 27 18:15:57 2017

arc: Fix enabling ARC from autotest

This use old version chrome://settings-frame that is not MD based.

BUG= chromium:694081 
TEST=Manually
Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8
Reviewed-on: https://chromium-review.googlesource.com/446090
Commit-Ready: Yury Khmel <khmel@google.com>
Tested-by: Yury Khmel <khmel@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>

[modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py

Labels: -M-58 M-60
This needs to be fixed for real in M-60 (preferably before or near the branch point) as we may be removing the old settings (settings-frame) in that dev cycle.

Project Member Comment 32 by bugdroid1@chromium.org, Apr 14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d127ade1e89dd1b221e85170e0dbe4c32072c7c5

commit d127ade1e89dd1b221e85170e0dbe4c32072c7c5
Author: khmel <khmel@chromium.org>
Date: Fri Apr 14 16:55:26 2017

arc: Provide API to control Play Store state from autotests

This provides required API to read and set Play Store state from
autotests since old settings page will deprecate soon and new MD
settings don't provide reliable way to access Play Store settings.

BUG= 694081 
TEST=Manually, together with
     https://chromium-review.googlesource.com/c/470707/

Review-Url: https://codereview.chromium.org/2801173002
Cr-Commit-Position: refs/heads/master@{#464735}

[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/chromeos/arc/arc_util.cc
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/chromeos/arc/arc_util.h
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/extensions/api/autotest_private/autotest_private_api.cc
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/extensions/api/autotest_private/autotest_private_api.h
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/common/extensions/api/autotest_private.idl
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/test/data/extensions/api_test/autotest_private/test.js
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/tools/metrics/histograms/histograms.xml

Blocking: 712781
Project Member Comment 34 by bugdroid1@chromium.org, Apr 27
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba

commit f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba
Author: khmel <khmel@google.com>
Date: Thu Apr 27 22:02:20 2017

arc: Switch to using autotest API to enable Play Store

Need to discard using settings based approach once old
settings expires soon.

BUG= chromium:694081 
TEST=Manually, together with crrev.com/2801173002

Change-Id: I906340935fd2f4faa9fc7f6783925f6b89728050
Reviewed-on: https://chromium-review.googlesource.com/470707
Commit-Ready: Yury Khmel <khmel@chromium.org>
Tested-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>

[modify] https://crrev.com/f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba/client/common_lib/cros/chrome.py
[modify] https://crrev.com/f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba/client/common_lib/cros/arc_util.py

Status: Fixed
Cc: levarum@chromium.org lpique@chromium.org malaykeshav@chromium.org steve...@chromium.org
 Issue 666871  has been merged into this issue.
Sign in to add a comment