Implement UrlbarController::viewContextChanged

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: dao, Assigned: mak)

Tracking

(Blocks 2 bugs)

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

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
Bug 1496476 adds a stub for this.
(Reporter)

Comment 1

8 months ago
Forgot to mention: the nsIAutoCompleteController equivalent for this is resetInternalState.
(Assignee)

Comment 2

7 months ago
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.
(Assignee)

Updated

7 months ago
Blocks: 1502879
(Assignee)

Updated

4 months ago
(Assignee)

Comment 3

4 months ago

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

(Reporter)

Comment 4

4 months ago

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

(Assignee)

Comment 5

4 months ago

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.

(Assignee)

Updated

3 months ago
Summary: Implement UrlbarController::tabContextChanged → Implement UrlbarController::viewContextChanged
(Assignee)

Updated

3 months ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Reporter)

Comment 7

3 months ago

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.

(Assignee)

Comment 8

3 months ago

(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

3 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c486f9e52665
Implement UrlbarController::viewContextChanged. r=dao

Comment 10

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months 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.