1954799 - Move new tab, new window, home button drag/drop handlers out of browser.js into a separate module

Status: RESOLVED FIXED

(Firefox :: General, task, P3)

Description

[:Gijs (he/him)]

All of this code doesn't need to live in browser.js . There's also quite a bit of code duplication in there.

To fix this, I think we want to:

1. create a ToolbarDropHandler module file (sys.mjs) in browser/components/customizableui/, and add a lazy import for it at the top of browser.js
2. move the homeButtonObserver, newTabButtonObserver and newWindowButtonObserver code (and any functions only called from there) into that module. It probably makes sense to only export 1 object from that module and expose whatever methods we need on there. Note that we'll need to update consumers in browser/base/content/navigator-toolbox.js and various other places.
3. update all the callsites of the various browserDragAndDrop methods to call Services.droppedLinkHandler's methods directly, except for onDragOver which we can move to ToolbarDropHandler.
4. check tests still pass
5. update browser.js.globals :-)

This should remove about 150 lines of code from browser.js and avoid that code being loaded until/unless the user actually uses drag/drop on the affected widgets.

Comment 1

[:Gijs (he/him)]

Shane, I'm assigning you this in lieu of bug 1882774, sorry for the confusion there and I hope this is OK. Let me know if you have questions / run into issues!

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::New Tab Page' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Shane Ziegler]

Hi, I’m a new contributor, and I’ll be submitting a patch for this bug soon.

Comment 4

[Shane Ziegler]

For functions like browserDragAndDrop.dropLinks(), which simply call Services.droppedLinkHandler.dropLinks(), is it best practice to keep a _dropLinks() in our new ToolbarDropHandler object for the onDrop() or other internal object function calls or can we just get rid of dropLinks and also do direct calls to Services.droppedLinkHandler.dropLinks()?

Comment 5

[Shane Ziegler]

How do we find which tests to run?

Comment 6

[:Gijs (he/him)]

(In reply to Shane Ziegler from comment #4)

For functions like browserDragAndDrop.dropLinks(), which simply call Services.droppedLinkHandler.dropLinks(), is it best practice to keep a _dropLinks() in our new ToolbarDropHandler object for the onDrop() or other internal object function calls or can we just get rid of dropLinks and also do direct calls to Services.droppedLinkHandler.dropLinks()?

I think where other places are currently calling browserDragAndDrop.dropLinks() they could simply call Services.droppedLinkHandler.dropLinks() directly instead.

(In reply to Shane Ziegler from comment #5)

How do we find which tests to run?

In this case I'm not sure there are many specific tests. For a trypush I'd probably just run with the mochitest-bc preset (./mach try fuzzy --preset mochitest-bc). I also ended up looking in searchfox, and from this query I think at least the following tests:

browser/base/content/test/general/browser_homeDrop.js
browser/base/content/test/general/browser_newTabDrop.js
browser/base/content/test/general/browser_newWindowDrop.js
browser/components/downloads/test/browser/browser_indicatorDrop.js
browser/components/downloads/test/browser/browser_libraryDrop.js
browser/components/tabbrowser/test/browser/tabs/browser_tab_dragdrop2.js

would be ones I'd suggest you run locally.

Let me know if you have any other questions or run into issues with that!

Comment 7

[Shane Ziegler]

How can I test the functionality these functions in Nightly? I am not sure what they do exactly or how to drag and drop a url and force some of them to get called.

I got some of the tests to run fine, but the others end up hanging on me and eventually timing out, so trying to figure out what is going on.

Comment 8

[Shane Ziegler]

Attachment: Bug 1954799 - Move new tab, new window, home button drag/drop handlers out of browser.js into ToolbarDropHandler.sys.mjs r?Gijs!

Comment 9

[Shane Ziegler]

I used some maybe crude/manual ways to get at globals for use of some classes, window, and setTimeout. Like const win = aEvent.target.ownerGlobal is there a better way to get access to those?

The 6 tests you gave above are now passing and it passed a build on the try push server.

Comment 10

[:Gijs (he/him)]

(In reply to Shane Ziegler from comment #9)

I used some maybe crude/manual ways to get at globals for use of some classes, window, and setTimeout. Like const win = aEvent.target.ownerGlobal is there a better way to get access to those?

If the event inherits from UIEvent then aEvent.view may be available (https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view ), but aEvent.target.ownerGlobal may also be fine.

The 6 tests you gave above are now passing and it passed a build on the try push server.

Ah, can you share the link to try? I'm a bit surprised because as I noted on your patch not all the files appear to be there, which I'd expect to cause failures... but maybe not?

Comment 11

[Shane Ziegler]

I noticed a change that still needs to be made on line 133 of ToolbarDropHandler.sys.mjs. I will be fixing it and re-submitting soon.

Comment 12

[Shane Ziegler]

I have made the requested changes.

The latest treeherder link is (https://treeherder.mozilla.org/jobs?repo=try&revision=54569b80420f1f3cd92ffb64825fe02b39cda00d)
The try push was still running at the time of this comment.

I have a couple questions:

• In ToolbarDropHandler.sys.mjs, on line 19, the call to this._canDropLink could be turned into a direct inline call and allow us to get rid of _canDropLink on line 16, but the current way it's defined/used adds some readability to the code. Should it be kept how it is now or turned into a direct inline call?

• Lines 25-30 have calls to win.gNavigatorBundle.getString, win is passed to the method by other code in this module. Is there a better way to access the gNavigatorBundle object from browser.js?

Comment 13

[:Gijs (he/him)]

(In reply to Shane Ziegler from comment #12)

I have made the requested changes.

The latest treeherder link is (https://treeherder.mozilla.org/jobs?repo=try&revision=54569b80420f1f3cd92ffb64825fe02b39cda00d)
The try push was still running at the time of this comment.

Edit: nice! That looks good (there's some orange, ie failed tests, but none of it related to your change so they're intermittents + unfortunately this bug

I have a couple questions:

• In ToolbarDropHandler.sys.mjs, on line 19, the call to this._canDropLink could be turned into a direct inline call and allow us to get rid of _canDropLink on line 16, but the current way it's defined/used adds some readability to the code. Should it be kept how it is now or turned into a direct inline call?

Eh, yeah, I don't feel super strongly, so leaving it like this is also fine by me.

• Lines 25-30 have calls to win.gNavigatorBundle.getString, win is passed to the method by other code in this module. Is there a better way to access the gNavigatorBundle object from browser.js?

We could create our own reference to the bundle, but that's not really much better. I think instead, it would be nice to convert these strings to fluent, put them in a separate file (browser.properties which is what gNavigatorBundle points to, is kind of a dumping ground) and then we can remove this dependency entirely and source the strings from just inside the module when a drag/drop actually happens. But let's do that in a separate bug as a separate refactor, that will make it a bit easier to deal with. So I'll file that bug and then if you want you could work on that next if you fancied it?

Comment 14

[Shane Ziegler]

I removed the random 1; typo in the code and resubmitted it.

Comment 15

[:Gijs (he/him)]

(In reply to Shane Ziegler from comment #14)

I removed the random 1; typo in the code and resubmitted it.

Great! I've queued the patch for landing.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1bc58e8ac8d
Move new tab, new window, home button drag/drop handlers out of browser.js into ToolbarDropHandler.sys.mjs r=Gijs

Comment 17

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/e1bc58e8ac8d