Closed
Bug 1503338
Opened 6 years ago
Closed 6 years ago
QuantumBar: Move _loadURL from the controller to the input
Categories
(Firefox :: Address Bar, enhancement, P1)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file)
Currently, in UrlbarController#_loadURL we have various parts that are commented out as TODOs:
https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/browser/components/urlbar/UrlbarController.jsm#302,305-307,336-337,346-347,351-353
These parts are to do with the interactions with UrlbarInput.
When I discussed these with Marco the other day, he suggested that we should have some notifications to the input, along the lines of:
onPreLoadURL
onPostLoadURL
We'll probably want some parameters for those, and we'll need to handle the error case as well. We can work those out as we do the actual patch.
Comment 1•6 years ago
|
||
Before/After usually sounds a bit better than Pre/Post.
Anyway the idea is that loading a url doesn't depend on the view.
Comment 2•6 years ago
|
||
Can you remind me what the upside is of _loadURL living in the controller? :) Can we just move it to UrlbarInput? Seems like this would simplify things.
Comment 3•6 years ago
|
||
Basically it requires the view to implement its own helper to load a url (similar to loadURL). We want to allow experiments (probably WebExtensions) to replace the view, and I don't think we want them to have to go through that.
What we should aim to, imo, is putting things that should be working equally for all of the views into the controller.
Comment 4•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #3)
> Basically it requires the view to implement its own helper to load a url
> (similar to loadURL).
How so?
> We want to allow experiments (probably WebExtensions)
> to replace the view, and I don't think we want them to have to go through
> that.
This is a high-level objective but technically I'm not sure this means allowing them to replace the entire UrlbarView class.
> What we should aim to, imo, is putting things that should be working equally
> for all of the views into the controller.
Well, I said UrlbarInput, not UrlbarView, so I'm not even sure why were talking about the latter.
Comment 5•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #4)
> > We want to allow experiments (probably WebExtensions)
> > to replace the view, and I don't think we want them to have to go through
> > that.
>
> This is a high-level objective but technically I'm not sure this means
> allowing them to replace the entire UrlbarView class.
A view may want to support things like:
1. different ways of showing information in the input field (selection, boxes, tags or side info)
2. different ways of laying out contents in the popup (like multiple columns, grids, details panes)
3. completely controlling what is queried and what is returned (picking given providers or result types)
I'm not sure how all of these would be possible without writing a complete view. And if a piece is replaceable it should not have to reimplement common operations.
> > What we should aim to, imo, is putting things that should be working equally
> > for all of the views into the controller.
>
> Well, I said UrlbarInput, not UrlbarView, so I'm not even sure why were
> talking about the latter.
UrlbarInput seems to have a direct dependency on the view layout itself, if we consider the view made up of an input field and a panel, indeed I was considering it actually part of the replaceable view.
Wether we want to lock down the input field, and only allow replacing popup contents, is a decision to be made. At that point the only replaceable part would be the popup contents and moving _loadURL to UrlbarInput would be ok, I guess.
We may want to discuss this next week when Mike's back.
It's a fair point, we're still defining boundaries.
Comment 6•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #5)
> UrlbarInput seems to have a direct dependency on the view layout itself, if
> we consider the view made up of an input field and a panel, indeed I was
> considering it actually part of the replaceable view.
This the first time I hear about that and it hasn't been part of my plan at all.
Comment 7•6 years ago
|
||
Ok, after discussing it with Mike, the input box is not an experimentation target for now, we'll concentrate on the results.
So, Dao is right, we can move it into the input module, what we'll consider replaceable is the view and results presentation.
Updated•6 years ago
|
Summary: QuantumBar: The controller should notify the input for pre/post loading of a URL → QuantumBar: Move _loadURL from the controller to the input
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee | ||
Comment 8•6 years ago
|
||
Updated•6 years ago
|
Attachment #9023972 -
Attachment description: Bug 1503338 - QuantumBar: Move _loadURL from the controller to the input. r?dao → Bug 1503338 - Move url loading handling functions from controller to input. r?dao
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba1aae6c2949
Move url loading handling functions from controller to input. r=dao
Comment 10•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in
before you can comment on or make changes to this bug.
Description
•