1496685 - Implement UrlbarController::viewContextChanged

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Dão Gottwald [::dao]]

Bug 1496476 adds a stub for this.

Comment 1

[Dão Gottwald [::dao]]

Forgot to mention: the nsIAutoCompleteController equivalent for this is resetInternalState.

Comment 2

[Marco Bonardo [:mak]]

I'd probably name this in a more generic way, like viewContextChanged, leaving the fact it could be a tab to the view. For a different view it may not be a tab.
This will be useful when we pass a previousContext in the queryContext representing the last search that was run, and on view context change we should clear that.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #2)

I'd probably name this in a more generic way, like viewContextChanged,
leaving the fact it could be a tab to the view. For a different view it may
not be a tab.

Since Mark asked on irc, what I meant here is "for the urlbar view the context is represented by a tab, but for another view it may be something else"

Comment 4

[Dão Gottwald [::dao]]

I'm still not sure what this means since we're only dealing with urlbar views. That said, viewContextChanged sounds fine to me too.

Comment 5

[Marco Bonardo [:mak]]

During the project creation we discussed about having an urlbarview in-content (thus inside a single tab), and in that case a few things would differ. I'm not saying we'll do something like that, but we can keep naming just a bit more generic anyway.

Comment 6

[Marco Bonardo [:mak]]

Attachment: Bug 1496685 - Implement UrlbarController::viewContextChanged. r=dao!

Comment 7

[Dão Gottwald [::dao]]

The original intent here was to port nsAutoCompleteController::ResetInternalState():

https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/toolkit/components/autocomplete/nsAutoCompleteController.cpp#145

Do we not need this after all? Should we wontfix this?

The current patch resets the UI (input and view) which seems quite different from what nsAutoCompleteController did, is definitely redundant in some cases, and potentially harmful. See my review comments in phabricator for details.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [::dao] from comment #7)

Do we not need this after all? Should we wontfix this?

I think we want this, because, even if currently we don't have lots of caching, we'll need it for things like Bug 1529931, where the input needs some cache of the previouly filled value, and likely once we start reusing results we still want to clear them on context change, to avoid leaking of visual info that may alarm the user (even if it's benign).
The current patch is not great because I went too far away, the scope should just be to clear information that is no more relevant when the context changes.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c486f9e52665
Implement UrlbarController::viewContextChanged. r=dao

Comment 10

[Natalia Csoregi [:nataliaCs]]

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