Remember search terms and show them again when returning to the awesomescreen

RESOLVED FIXED in Firefox 23

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: wesj, Assigned: Margaret)

Tracking

Trunk
Firefox 23
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 23+)

Details

(Whiteboard: [search])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 694035 [details] [diff] [review]
WIP Patch

When you type a search term or url into the awesomescreen, and then realize you typed something wrong and want to go back and change it, its kinda a pain right now. When you return to the awesomescreen, you're shown the url you're at and not the search you typed. So for instance if I search for "red sweater" and click a link and then realize its the wrong one, I have to retype red sweater again to look for a new entry. It seems like it would be better to show the user typed term back to the user.

This is a patch I was playing with every now and then that I think is ready for review. But I should note there's a security issue here, as there's no way to see the url of a site that you got to through a user-entered search...

Note we also probably want to reperform the search in this case, whereas when we're just showing the current page's url, we likely don't. I don't address that here yet.
(Reporter)

Updated

5 years ago
Attachment #694035 - Attachment is patch: true
Also filed as bug 709451.
Duplicate of this bug: 709451
Wes - Want to pick this back up?
Flags: needinfo?(wjohnston)
(Assignee)

Comment 4

5 years ago
Wes, I could also take this if you're busy with other things.
(Reporter)

Comment 5

5 years ago
Certainly. mfinkle told me once that desktop stores this info somwhere in the session data. I need to look and see where that is.

I'm going to try to keep my assigned list to things that I'm actively working on, so if I start on this again, I'll assign to myself. Until then its up for grabs.
Flags: needinfo?(wjohnston)
(Assignee)

Comment 6

5 years ago
I can take this. I'll look into what desktop does, but I'm not sure they store a search term anywhere (at least the UI doesn't do anything about it the way we'd want our awesomescreen to work).
QA Contact: margaret.leibovic
(Assignee)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
QA Contact: margaret.leibovic

Updated

5 years ago
Whiteboard: [search]
(Assignee)

Comment 7

5 years ago
Created attachment 735286 [details] [diff] [review]
Bug 823230 - (Part 1) Pass search term along with "keyword-search" notification

In order to fix this bug The Right Way, and only show the search term only when the user actually performs a search, we need Gecko to tell us when a search happens. In the default case, this happens through loadURIWithFlags. There's already a "keyword-search" notification that fires when this happens, and this patch just hacks that notification to also send us the search term.

Gavin, it looks like you reviewed the patch that added this notification. Let me know if you think there's a more suitable reviewer.
Attachment #694035 - Attachment is obsolete: true
Attachment #735286 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

5 years ago
Created attachment 735293 [details] [diff] [review]
(Part 2) Remember search terms and show them again when returning to the awesomescreen

This is the real patch for this bug. I decided to store the searchTerm state on the tab, then just pass it along with the "Content:LocationChange" message.

This patch handles the two different ways a search can happen, either through tapping on a search entry in the awesomescreen, or through the loadURIWithFlags to "keyword-search" notification case. The first case is a little messy because we don't necessarily have a tab when we go through that data.engine check, so I just tacked it onto the addTab/loadURI params.

My only concern here is that my "keyword-search" handler assumes that the search is happening on the selected tab. I'm having a tough time thinking of a time when this isn't the case, but it's something we should think about.
Attachment #735293 - Flags: review?(mark.finkle)
Comment on attachment 735286 [details] [diff] [review]
Bug 823230 - (Part 1) Pass search term along with "keyword-search" notification

>diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp

>+                    nsresult rv;
>+                    nsCOMPtr<nsISupportsCString> searchTerm =
>+                        do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID, &rv);

Looks good, but it would be cleaner to just use the non-rv variant and null check. Also, doesn't matter much in practice, but it's slightly more efficient to use an nsISupportsString and do the conversion here (once) rather than forcing the conversion at the xpconnect layer for every potential notification observer in JS:
nsCOMPtr<nsISupportsString> searchTerm(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));
if (searchTerm) {
    searchTerm->SetData(NS_ConvertUTF8toUTF16(keyword).get());
}
obsSvc->NotifyObservers(searchTerm, "keyword-search", name.get());

r=me with those changes.
Attachment #735286 - Flags: review?(gavin.sharp) → review+
Hrm, though actually, maybe it would be better to switch the arguments around - pass defaultEngine as the "aSubject" argument, and pass the keyword as the aData argument. That would require switching the FHR observer of keyword-search to use aSubject.name instead of aData, but that shouldn't be a big deal.
(Assignee)

Comment 11

5 years ago
Created attachment 735456 [details] [diff] [review]
(Part 1 v2) Pass search term along with "keyword-search" notification

Addressed comments AND updated the browser chrome test.
Attachment #735286 - Attachment is obsolete: true
Attachment #735456 - Flags: review?(gavin.sharp)
(Assignee)

Comment 12

5 years ago
Created attachment 735458 [details] [diff] [review]
(Part 2) Remember search terms and show them again when returning to the awesomescreen

Updated to work with new notification.
Attachment #735293 - Attachment is obsolete: true
Attachment #735293 - Flags: review?(mark.finkle)
Attachment #735458 - Flags: review?(mark.finkle)
Comment on attachment 735456 [details] [diff] [review]
(Part 1 v2) Pass search term along with "keyword-search" notification

sweet
Attachment #735456 - Flags: review?(gavin.sharp) → review+
Comment on attachment 735458 [details] [diff] [review]
(Part 2) Remember search terms and show them again when returning to the awesomescreen

I was toying with a suggestion of calling it "UserTyped", like desktop, but this is really explicitly the user entered search.

I am getting caught up on "searchTerm". I don't know why I don't like the name. I'd like to tie it to something the user did explicitly, and that it's not singular. I see we already track "userEntered" as a boolean.

Can we change "searchTerm" to "userSearch" ?
Attachment #735458 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 15

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #14)
> Comment on attachment 735458 [details] [diff] [review]
> (Part 2) Remember search terms and show them again when returning to the
> awesomescreen
> 
> I was toying with a suggestion of calling it "UserTyped", like desktop, but
> this is really explicitly the user entered search.
> 
> I am getting caught up on "searchTerm". I don't know why I don't like the
> name. I'd like to tie it to something the user did explicitly, and that it's
> not singular. I see we already track "userEntered" as a boolean.
> 
> Can we change "searchTerm" to "userSearch" ?

Sounds good to me. I agree it's smart to indicate that this is something the user entered.
(Assignee)

Comment 16

5 years ago
Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=c70ea92a1aca

https://hg.mozilla.org/integration/mozilla-inbound/rev/26bc83f8c555
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b44cd1cdb1
https://hg.mozilla.org/mozilla-central/rev/26bc83f8c555
https://hg.mozilla.org/mozilla-central/rev/49b44cd1cdb1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

4 years ago
relnote-firefox: --- → ?
relnote-firefox: ? → 23+
(Assignee)

Updated

3 years ago
Depends on: 1022464
Duplicate of this bug: 1204958
You need to log in before you can comment on or make changes to this bug.