This bug was filed as a result of [1] changing waitForElement, yes?

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1090517

Yes

Integration tests have a deepQueryAllElements API for dealing with shadow DOM, and various tests use it, the QuickView/FilesAppBrowserTest for example.

Many tests use waitForElement, ie., they don't need to look into shadow DOM.  When they do need to query shadow DOM, they use deepQueryAllElements.

The explicit need / intent of the test code is spelled out just by looking at the element query API they use.  Seems like a good thing to me.

So I don't think we should be changing waitForElement.  Ideally you should be using deepQueryAllElements.

I prefer to have consistency, so I'm happy to change the UI test waitForElement to not support shadow DOM, but I feel like this change brings greater consistency overall.

My impression is that the difference between intent for waitForElement and deepQueryAllElements is not about whether shadow DOM is used, but about whether you need to retrieve state of an element back to the test extension context.

Given that deepQueryAllElements and sendEvent (fakeMouseClick, etc) support shadow DOM, I think it would be a useful improvement for waitForElement to also support it.

Luciano, do you have a preference?

I'll push a new version of the patch to remove shadow DOM support for waitForElement and it is easy enough to submit whichever patch is agreed on.

Consistency is why I mentioned this entire issue in the first place.  By copying test harness code out of FilesApp into your thing, you setup for divergence in code from the get go.  Then you go further (change waitforElements here) and that actually creates divergence, ergo inconsistency.

If waitforElements was changed to support shadow DOM, who knows what would break, but since the current integration test version ain't broken, so there's no need to fix it.

An alternate fix would be to add deepQueryAllElements to your test harness, but that does not help solve the code divergence problem either.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1178f181f9203548b4468f4ada49b975213c23c8

commit 1178f181f9203548b4468f4ada49b975213c23c8
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Jun 20 16:00:45 2018

FilesApp remove unused iframeQuery from integration test code

Bug: 852324
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I534f67d93048d4e2e87db2fb3098513511a509ac
Reviewed-on: https://chromium-review.googlesource.com/1098912
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568857}
[modify] https://crrev.com/1178f181f9203548b4468f4ada49b975213c23c8/ui/file_manager/file_manager/background/js/test_util_base.js
[modify] https://crrev.com/1178f181f9203548b4468f4ada49b975213c23c8/ui/file_manager/integration_tests/file_manager/quick_view.js
[modify] https://crrev.com/1178f181f9203548b4468f4ada49b975213c23c8/ui/file_manager/integration_tests/file_manager/share_and_manage_dialog.js
[modify] https://crrev.com/1178f181f9203548b4468f4ada49b975213c23c8/ui/file_manager/integration_tests/remote_call.js

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/050406103346b43f6b318e7891b03c52ba3db41f

commit 050406103346b43f6b318e7891b03c52ba3db41f
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Jun 20 16:31:11 2018

FilesApp keep APIs for UI test and integration tests similar

Remove shadow DOM support from waitForElement used in UI tests.
Reverts part of crosreview.com/1090517

Bug: 852324
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: If64176b0a9eee107afde06a2adb6bc076fb87e24
Reviewed-on: https://chromium-review.googlesource.com/1098910
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568867}
[modify] https://crrev.com/050406103346b43f6b318e7891b03c52ba3db41f/ui/file_manager/file_manager/test/js/test_util.js
[modify] https://crrev.com/050406103346b43f6b318e7891b03c52ba3db41f/ui/file_manager/file_manager/test/quick_view.js

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02db51d71a754b0044f285398ab1af66648595ba

commit 02db51d71a754b0044f285398ab1af66648595ba
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Jun 20 18:11:28 2018

FilesApp inline and remove test.util.sync.getElement_

Initially I planned to make the error message in getElement_ consistent
for the case where contentWindow.document does not exist and
contentWindow.document.querySelector(targetQuery) returns null.
In the end it made more sense to inline this into the 2 callers
that exist.

Bug: 852324
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ia5741cbb9628ada0839faca8d9d108f5535ba9f1
Reviewed-on: https://chromium-review.googlesource.com/1108238
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568927}
[modify] https://crrev.com/02db51d71a754b0044f285398ab1af66648595ba/ui/file_manager/file_manager/background/js/test_util_base.js