1492226 - Move UnifiedComplete.js to browser/components/urlbar

Status: RESOLVED WONTFIX

(Firefox :: Address Bar, enhancement, P1)

Description

[Drew Willcoxon :adw]

With bug 1490527, UnifiedComplete.js imports browser/components/urlbar/UrlbarPrefs.jsm.  That's not great since UnifiedComplete lives in toolkit, and toolkit shouldn't really depend on things in browser -- although UnifiedComplete already depends on a bunch of browser.urlbar preferences.  UnifiedComplete is very Firefox specific at this point anyway, so we should investigate moving it to browser/components/urlbar.

Comment 1

[Jorg K (CEST = GMT+2)]

In bug 1492379 we'll be disabling some toolkit tests run by Thunderbird since they now fail; apart from 'unifiedcomplete' also four more, see attachment 9010205 [details] [diff] [review] (pending review).

Comment 2

[Marco Bonardo [:mak]]

we should also move those 4 additional tests to the browser component.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1492226 - Move UnifiedComplete.js to browser/components/urlbar

Comment 4

[Drew Willcoxon :adw]

This was fairly straightforward.  There are a couple of linting issues: (1) UnifiedComplete.js and a bunch of the tests fail a bunch of linting rules in browser/components/urlbar, so I've disabled those rules for these files for now.  We should clean them up in a separate bug, if at all.  (2) Even with those rules disabled, there were two failures in UnifiedComplete.js due to shadowing variables, but they're easily fixed.

In the tests, it looks like we already just import head_common.js from toolkit for the browser/components/urlbar/tests/unit tests, so I did that here too.

Tests pass locally.  I've pushed to try and then I'll request review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c27777756d3a9bbb76dd5ce224ba2f439455c5b

Comment 5

[Drew Willcoxon :adw]

Oh one more thing, I originally had this in the moz.build:

 with Files('UnifiedComplete.js'):
     BUG_COMPONENT = ('Toolkit', 'Places')

But I removed it.  Should I add it back?  Probably not super important either way, but how/where should we handle UnifiedComplete bugs from now on?  Should we move existing bugs to Firefox?  That's not necessary or productive IMO, but I'm not sure about the first question.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #5)
> Oh one more thing, I originally had this in the moz.build:
> 
>  with Files('UnifiedComplete.js'):
>      BUG_COMPONENT = ('Toolkit', 'Places')
> 
> But I removed it.  Should I add it back?  Probably not super important
> either way, but how/where should we handle UnifiedComplete bugs from now on?

The urlbar components is being files under Firefox / Address Bar I think, and that's a good place also for unifiedcomplete bugs.

> Should we move existing bugs to Firefox?  That's not necessary or productive
> IMO, but I'm not sure about the first question.

We could do P1s/P2s, but it's not super critical, I'd say to move bugs when we touch them for other reasons or when we are actively working on them

Comment 7

[Drew Willcoxon :adw]

Hopefully fixed now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5287a056f0ab51e3e8184e181cbd36b63ea84d79

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 9012423 [details]
Bug 1492226 - Move UnifiedComplete.js to browser/components/urlbar

Marco Bonardo [::mak] has approved the revision.

Comment 9

[Drew Willcoxon :adw]

Just a comment on why I haven't landed this yet -- I'd like to land the various other bookmark autofill bugs first because I want to uplift them, and that seems easier if the m-c and beta patches are pretty much the same.  I don't want to have to port changes in browser/components/urlbar/UnifiedComplete.js (m-c) to toolkit/components/places/UnifiedComplete.js (beta).

Those bugs are:

* bug 1489060 (already landed)
* bug 1494471 (needs review)
* possibly bug 1493636 (needs feedback)

Comment 10

[Drew Willcoxon :adw]

PlacesSearchAutocompleteProvider.jsm should probably be moved, too.

Comment 11

[Marco Bonardo [:mak]]

oh yes, any autocomplete provider should be moved, you are right.

Comment 12

[Mark Banner (:standard8)]

Drew, could this be landed now?

Comment 13

[Drew Willcoxon :adw]

It needs to be unbitrotted, but it might be easier at this point to start over and hg mv the file again so that we're sure not to miss any changes between when the patch was written and now. Nothing is blocking moving it now afaik; previously I was landing and uplifting @search keyword patches that would have made landing and uplifting it painful, but that's stopped for now.

Plus comment 10 and comment 11 need to be incorporated.

But I wonder whether moving it is even worth it now given the progress on quantumbar. There's no big reason to do it imo, and while it's low risk, the risk probably isn't 0% that there won't be unforeseen follow ups. Marco, what do you think now?

Comment 14

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #13)

But I wonder whether moving it is even worth it now given the progress on quantumbar. There's no big reason to do it imo, and while it's low risk, the risk probably isn't 0% that there won't be unforeseen follow ups. Marco, what do you think now?

The main reason was to compact all the code in urlbar, to make it easier to move tests around from legacy to new code. But in the end this is not a strict necessity, and we're still making deep changes to this code, so it doesn't sound compelling at this time.

It would be far more useful to move all address bar related browser-chrome tests instead, they are spread around in browser/base yet.
So, feel free to drop this.

Comment 15

[Drew Willcoxon :adw]

Dropping this as discussed. I'll wontfix it too. We can reopen it if we want.