Closed Bug 1492226 Opened 3 years ago Closed 2 years ago
Complete .js to browser/components/urlbar
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.
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).
we should also move those 4 additional tests to the browser component.
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
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.
(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 on attachment 9012423 [details] Bug 1492226 - Move UnifiedComplete.js to browser/components/urlbar Marco Bonardo [::mak] has approved the revision.
Attachment #9012423 - Flags: review+
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)
PlacesSearchAutocompleteProvider.jsm should probably be moved, too.
oh yes, any autocomplete provider should be moved, you are right.
Drew, could this be landed now?
Flags: needinfo?(adw) → needinfo?(mak77)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.