1503338 - QuantumBar: Move _loadURL from the controller to the input

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P1)

Description

[Mark Banner (:standard8)]

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

[Marco Bonardo [:mak]]

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

[Dão Gottwald [:dao]]

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

[Marco Bonardo [:mak]]

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

[Dão Gottwald [:dao]]

(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

[Marco Bonardo [:mak]]

(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

[Dão Gottwald [:dao]]

(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

[Marco Bonardo [:mak]]

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.

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1503338 - Move url loading handling functions from controller to input. r?dao

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba1aae6c2949
Move url loading handling functions from controller to input. r=dao

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ba1aae6c2949