Remove `Anon` and `AnonAttribute` strategies from "WebDriver:FindElement" and "WebDriver:FindElements" command
Categories
(Remote Protocol :: Marionette, task, P1)
Tracking
(firefox72 fixed)
Tracking | Status | |
---|---|---|
firefox72 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Today all the XBL bindings got removed. But we still have XBL support in-tree.
Once bug 1566221 has been fixed we should get started to remove the Anon
and AnonAttribute
strategies from the find element(s) commands.
Personally I would wait and also sync with the Thunderbird team. I don't know what's the status is for that project. Setting needinfo for Samuel and Geoff.
Comment 1•5 years ago
|
||
The thunderbird marionette part doesn't use them.
Assignee | ||
Comment 3•5 years ago
|
||
Thanks for those quick replies. Lets also get some feedback from Brian.
Comment 4•5 years ago
|
||
Thanks for tracking this. This can for sure go once we don't ship MOZ_XBL anymore in Firefox (Bug 1583314). Also, for reference Thunderbird doesn't use XBL anymore either (https://mail.mozilla.org/pipermail/tb-planning/2019-September/006954.html).
I'm going to flip the relationship on Bug 1566221 to consider this as blocking the "remove XBL support" work (I should rename that bug to not be Gecko-specific, since we've morphed it to track i.e. frontend cleanups as well).
There are a couple of other text hits for "XBL" that are worth investigating too to see if there's any cleanup possible: https://searchfox.org/mozilla-central/search?q=xbl&path=testing%2Fmarionette.
Assignee | ||
Comment 5•5 years ago
|
||
So we already build with XBL disabled? See also bug 1587662 for some failing Marionette tests, which your query also refers to. As I have read we only want to build without XBL on Android for now, but I also see on MacOS.
If it is ok to just remove all the XBL specific tests for Marionette, and the anon support right now, I can go ahead and make the change.
Comment 6•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #5)
So we already build with XBL disabled? See also bug 1587662 for some failing Marionette tests, which your query also refers to. As I have read we only want to build without XBL on Android for now, but I also see on MacOS.
If it is ok to just remove all the XBL specific tests for Marionette, and the anon support right now, I can go ahead and make the change.
We don't build with XBL disabled in desktop yet (only in GeckoView), but for bug 1587662 I think we might be hitting some weirdness with artifact builds (see https://bugzilla.mozilla.org/show_bug.cgi?id=1510785#c21). That failure should probably be fixable by adding a skip-if = !xbl
, but at this point I think it's fine to just remove them. We aren't going to re-add any bindings in desktop, and so we don't need to use Marionette to test them.
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Ok, so lets wait until the artifact build issue has been fixed, so we can easily verify the fix. Afterward we can remove all the Anon code. Thanks.
Comment 8•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #7)
Ok, so lets wait until the artifact build issue has been fixed, so we can easily verify the fix. Afterward we can remove all the Anon code. Thanks.
This should be good to go now, the artifact build issue has been fixed.
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
Given that this blocks the final remove of xbl implementations including methods like GetAnonymousElementByAttribute
, I'm going to do the removal now.
Assignee | ||
Comment 10•5 years ago
|
||
I actually stumbled over a comment in driver.js which makes me wonder how to proceed. Brian, can you give some feedback?
// Shadow nodes also show up in getAnonymousNodes, we should // ignore them.
How do I have to handle shadow nodes?
Comment 11•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #10)
I actually stumbled over a comment in driver.js which makes me wonder how to proceed. Brian, can you give some feedback?
// Shadow nodes also show up in getAnonymousNodes, we should // ignore them.
How do I have to handle shadow nodes?
Based on https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/dom/base/Document.cpp#7996 I think that comment is incorrect, but forwarding to bz to be sure.
Comment 12•5 years ago
|
||
getAnonymousNodes
in fact does not include shadow stuff.
However, it's not obvious to me that the code linked in comment 10 is using getAnonymousNodes
in a way that would be relevant to that comment. It's got a wantedFrame
, which is a node it got from ... somewhere? It does document.getBindingParent(wantedFrame)
, which for a shadow DOM element will in fact return the shadow host; see https://searchfox.org/mozilla-central/rev/7536d7f480a7f18c941a590a2d4c5119d9f52770/dom/base/ShadowRoot.cpp#68 which then propagates to all the shadow DOM bits.
So if wantedFrame
is in a shadow tree, then parent
will be non-null; this code wants to not run the getAnonymousNodes
bit in that case, so checks for the shadowRoot
bits.
Presumably the right update to not having XBL is to remove that whole block?
Assignee | ||
Comment 13•5 years ago
|
||
Probably yes. When checking for the addition of this code I ended-up with bug 1237698 and specifically: https://hg.mozilla.org/mozilla-central/rev/ca74b06769ce. Best is to just revert those changes, which is in accordance what Boris proposed.
Assignee | ||
Comment 14•5 years ago
|
||
Assignee | ||
Comment 15•5 years ago
|
||
The patch removes the usage of elements based on anonymouse nodes,
which are soon to be completely removed. Also code which is no longer
in use by Firefox-UI tests can simply be removed.
Assignee | ||
Comment 16•5 years ago
|
||
Depends on D50803
Updated•5 years ago
|
Comment 17•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
|
||
Can we please backout the second patch (https://hg.mozilla.org/integration/autoland/rev/79be8382d57a) given that it causes failures in browser-chrome tests. Thanks.
Comment 19•5 years ago
|
||
Backed out 79be8382d57a for bc failures on browser_all_files_referenced.js. (Bug 1592220)
Backout link: https://hg.mozilla.org/integration/autoland/rev/2521937495c4611096a75ac684f77e1dea610ca7
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273441237&repo=autoland&lineNumber=2155
Assignee | ||
Comment 20•5 years ago
|
||
The problem here is that test.xul
is no longer referenced since it was only referenced in test_anonymous_content.xul
. Also the other two referenced files are part of test.xul
, and as such three files are not referenced. The short-term fix will be to keep a reference to test.xul
in browser_all_files_referenced.js
.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1dea418bcf51fbe48925e716b2562878a84e0e5
Comment 21•5 years ago
|
||
Comment 22•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ab59c714f29
https://hg.mozilla.org/mozilla-central/rev/f77279b36116
Updated•2 years ago
|
Description
•