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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 1

[Brian Nicholson (:bnicholson)]

Also filed as bug 709451.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Want to pick this back up?

Comment 4

[:Margaret Leibovic]

Wes, I could also take this if you're busy with other things.

Comment 5

[Wesley Johnston (:wesj)]

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.

Comment 6

[:Margaret Leibovic]

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

Comment 7

[:Margaret Leibovic]

Attachment: 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.

Comment 8

[:Margaret Leibovic]

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

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 1 v2) Pass search term along with "keyword-search" notification

Addressed comments AND updated the browser chrome test.

Comment 12

[:Margaret Leibovic]

Attachment: (Part 2) Remember search terms and show them again when returning to the awesomescreen

Updated to work with new notification.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 735456 [details] [diff] [review]
(Part 1 v2) Pass search term along with "keyword-search" notification

sweet

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 15

[:Margaret Leibovic]

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

Comment 16

[:Margaret Leibovic]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26bc83f8c555
https://hg.mozilla.org/mozilla-central/rev/49b44cd1cdb1