Closed Bug 1496685 Opened 2 years ago Closed 2 years ago

Implement UrlbarController::viewContextChanged

Categories

(Firefox :: Address Bar, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: dao, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1496476 adds a stub for this.
Forgot to mention: the nsIAutoCompleteController equivalent for this is resetInternalState.
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.
Blocks: 1502879

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

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

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.

Summary: Implement UrlbarController::tabContextChanged → Implement UrlbarController::viewContextChanged
Assignee: nobody → mak77
Status: NEW → ASSIGNED

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.

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

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c486f9e52665
Implement UrlbarController::viewContextChanged. r=dao
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1530985
You need to log in before you can comment on or make changes to this bug.