Closed Bug 1492226 Opened 6 years ago Closed 6 years ago

Move UnifiedComplete.js to browser/components/urlbar

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox64 --- affected

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 obsolete file)

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)
Depends on: 1495327
Whiteboard: [fxsearch]
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)

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?

Flags: needinfo?(adw) → needinfo?(mak77)

(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.

Flags: needinfo?(mak77)

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Attachment #9012423 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: