QuantumBar: Move _loadURL from the controller to the input

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

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.
Before/After usually sounds a bit better than Pre/Post.
Anyway the idea is that loading a url doesn't depend on the view.
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.
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.
(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.
(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.
(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.
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.
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: nobody → standard8
Status: NEW → ASSIGNED
Priority: P2 → P1
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

Comment 9

6 months ago
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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba1aae6c2949
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Depends on: 1509610
You need to log in before you can comment on or make changes to this bug.