Closed Bug 1591145 Opened 5 years ago Closed 4 years ago

Remove Document.GetAnonymousElementByAttribute

Categories

(Core :: XBL, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → ajvincent
Status: NEW → ASSIGNED

This is more complicated than it sounds. In removing potential callers, I found references in Rust:
https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/testing/geckodriver/src/marionette.rs#1543

I don't know Rust (yet). I need help with that part of this.

Comment on attachment 9104134 [details]
Bug 1591145, remove document.getAnonymousElementByAttribute, part 1.

feedback: I'm looking for a tryserver push with the full test suite. I really want to get an idea of what this patch breaks, particularly in two areas:
(1) The aforementioned Rust code that may need changes
(2) I added in some throw statements to JS test code, specifically to tell me if that code needs an alternate, or can just be removed.

Attachment #9104134 - Flags: feedback?(bugs)
Flags: needinfo?(bugs)

needinfo from smaug per his request for a tryserver run.

Whoops. In writing my patch for this, I forgot to check comm-central. In that repository, suite/ is chock-full of calls to this method. mail/ also has a couple...

jorgk, any suggestions?

Flags: needinfo?(jorgk)
Flags: needinfo?(bugs)

hmm, but I should have looked at the patch some more before pushing to try.
Doesn't make much sense to push when it has still several FIXMEs. Oh well.

(In reply to Alex Vincent [:WeirdAl] from comment #5)

Whoops. In writing my patch for this, I forgot to check comm-central. In that repository, suite/ is chock-full of calls to this method. mail/ also has a couple...

Thanks for the heads-up. This is part of the de-XBL effort that Magnus is leading for Thunderbird. Looks like we have "a couple" (two) call sites left:
https://searchfox.org/comm-central/search?q=GetAnonymousElementByAttribute&case=false&regexp=false&path=mail%2F
Over to Magnus.

SeaMonkey/Suite sadly has fallen behind the de-overlay and de-XBL effort and has been non-functional for a while.

Flags: needinfo?(jorgk) → needinfo?(mkmelin+mozilla)
See Also: → 1587627

Removing the Thunderbird ones in bug 1591361.

Flags: needinfo?(mkmelin+mozilla)
Depends on: 1587627
See Also: 1587627

Comment on attachment 9104134 [details]
Bug 1591145, remove document.getAnonymousElementByAttribute, part 1.

Oops, didn't clear this when I pushed the patch to try

Attachment #9104134 - Flags: feedback?(bugs)
Assignee: ajvincent → nobody
Status: ASSIGNED → NEW

Mark, I see some consumers of the getAnonymousElementByAttribute API (which now doesn't do anything) in toolkit/mozapps/extensions/test/browser/browser_interaction_telemetry.js (see the list at https://searchfox.org/mozilla-central/search?q=getAnonymousElementByAttribute&path=). That makes me think at least part of that test isn't working as expected. Could you help me figure out what's going on with it and if those calls should be removed or replaced with something else?

Flags: needinfo?(mstriemer)

Matt, I see a consumer of getAnonymousElementByAttribute API (which now doesn't do anything) in UITour.jsm at: https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/browser/components/uitour/UITour.jsm#207. Should that searchPrefsLink target be removed or updated?

Flags: needinfo?(MattN+bmo)

(In reply to Brian Grinstead [:bgrins] from comment #12)

Matt, I see a consumer of getAnonymousElementByAttribute API (which now doesn't do anything) in UITour.jsm at: https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/browser/components/uitour/UITour.jsm#207. Should that searchPrefsLink target be removed or updated?

That's already addressed by the patch: https://phabricator.services.mozilla.com/D50563#change-IkudWfwBJwKb

Someone on the search team would have more input into whether that code should stick around but I already had seen the deletion in the patch and hoped that someone there was already consulted.

Flags: needinfo?(MattN+bmo)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #13)

(In reply to Brian Grinstead [:bgrins] from comment #12)

Matt, I see a consumer of getAnonymousElementByAttribute API (which now doesn't do anything) in UITour.jsm at: https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/browser/components/uitour/UITour.jsm#207. Should that searchPrefsLink target be removed or updated?

That's already addressed by the patch: https://phabricator.services.mozilla.com/D50563#change-IkudWfwBJwKb

Someone on the search team would have more input into whether that code should stick around but I already had seen the deletion in the patch and hoped that someone there was already consulted.

That patch is being split up into component parts and not landed on it's own, so I'm asking here for clarification. Mak, could you confirm if we should remove searchPrefsLink from UITour or update it to not rely on the XBL anonymous content? FWIW this has been broken for a while as it would have stopped working when PopupSearchAutoComplete was moved away from XBL.

Flags: needinfo?(mak)

document.getAnonymousElementByAttribute is dead code now, and even when it wasn't this would
have returned null ever since <toolbarbutton> stopped using XBL.

Attachment #9104134 - Attachment is obsolete: true

My apologies to everyone, but I had to walk away from this bug due to other commitments that I have. I overestimated the amount of time I would be able to work on this. That's why :bgrins is asking all these questions.

(In reply to Brian Grinstead [:bgrins] from comment #14)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #13)
Mak, could you confirm if we should remove searchPrefsLink from UITour or update it to not rely on the XBL anonymous content? FWIW this has been broken for a while as it would have stopped working when PopupSearchAutoComplete was moved away from XBL.

It looks like this UITour target was pointing to the search settings button on the new one-off buttons, when they were initially introduced on the search bar. The search bar is no more our primary SAP, so it wouldn't make sense to continue doing that. If we should ever need this for the urlbar, we'll just add a new target, for now I don't think it's necessary, one-off buttons were introduced quite some time ago.
Feel free to remove it.

Flags: needinfo?(mak)

(In reply to Brian Grinstead [:bgrins] from comment #11)

Mark, I see some consumers of the getAnonymousElementByAttribute API (which now doesn't do anything) in toolkit/mozapps/extensions/test/browser/browser_interaction_telemetry.js (see the list at https://searchfox.org/mozilla-central/search?q=getAnonymousElementByAttribute&path=). That makes me think at least part of that test isn't working as expected. Could you help me figure out what's going on with it and if those calls should be removed or replaced with something else?

This is dead test code related to the old XUL about:addons. I filed bug 1594082 to remove it.

Flags: needinfo?(mstriemer)

(In reply to Mark Striemer [:mstriemer] from comment #19)

(In reply to Brian Grinstead [:bgrins] from comment #11)

Mark, I see some consumers of the getAnonymousElementByAttribute API (which now doesn't do anything) in toolkit/mozapps/extensions/test/browser/browser_interaction_telemetry.js (see the list at https://searchfox.org/mozilla-central/search?q=getAnonymousElementByAttribute&path=). That makes me think at least part of that test isn't working as expected. Could you help me figure out what's going on with it and if those calls should be removed or replaced with something else?

This is dead test code related to the old XUL about:addons. I filed bug 1594082 to remove it.

Thanks! Will track that here.

Depends on: 1594082
Depends on: 1594110

Comment on attachment 9106383 [details]
Bug 1591145 - Remove unused searchPrefsLink UI Tour target

Revision D51741 was moved to bug 1594110. Setting attachment 9106383 [details] to obsolete.

Attachment #9106383 - Attachment is obsolete: true
Depends on: 1594123

Comment on attachment 9106357 [details]
Bug 1591145 - Fix lookup of toolbarbutton-icon in TabsList

Revision D51727 was moved to bug 1594123. Setting attachment 9106357 [details] to obsolete.

Attachment #9106357 - Attachment is obsolete: true
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c179bdbb0d2a
Remove Document.GetAnonymousElementByAttribute r=webidl,baku
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.