Closed Bug 915355 Opened 8 years ago Closed 8 years ago

"keyword-search" event triggers on default search engine searches

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcomella, Assigned: mcomella)

Details

(Whiteboard: [qa-])

Attachments

(3 files, 2 obsolete files)

This was originally discovered in the work for bug 913713 (specifically https://bugzilla.mozilla.org/show_bug.cgi?id=913713#c7).

This affects both Desktop and Android.

The patches from parts 2-4 in bug 913713 are being transferred here.
rnewman mentioned in bug 913713 that this review is blocked on part 1 (https://bugzilla.mozilla.org/show_bug.cgi?id=913713#c18).
Attachment #803301 - Flags: review?(rnewman)
Comment on attachment 803300 [details] [diff] [review]
01: Rename keyword-search to default-search

This doesn't really look right. I assume the relevant reasoning is bug 913713 comment 7.

"Keyword search" is the name of the feature that enables searching from the address bar in fx-desktop (and I thought also for Android?), and so that's why "keyword-search" observer notification was added. It has nothing to do with the "default search engine", aside from the fact that that's what's currently used for such searches. It was intended to distinguish the specific UI used to trigger the search, not to track searches with a specific search engine, so  "default-search" seems like a more confusing name.

It sounds like maybe Android's use of this topic is what needs fixing, or maybe you need a separate set of notifications for some reason given Android's search UI differences? Not sure.
Attachment #803300 - Flags: review?(gavin.sharp) → review-
I think the confusion stems from the use of the term "keyword" - while I don't think the Android platform has a formal definition for these terms, I intuited the term "keyword search" to be a bookmark keyword search ("barkeyword" in Android FHR) and used "default search" to describe a search using the default search engine ("bartext" in Android FHR), which are differentiated in FHR despite being activated from the same location. 

> "Keyword search" is the name of the feature that enables searching
> from the address bar in fx-desktop

What is the motivation behind this term choice? Is there any reason it isn't called "address bar search" or something similar?
Hysterical raisins! I can see how it's a bit confusing how all of this stuff has come together over time, somewhat haphazardly, but to be fair the naming occurred over the course of multiple decades :) keyword.URL and co. date from 1999, (see bug 11869 / http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/modules/libpref/src/init&command=DIFF_FRAMESET&file=all.js&rev2=3.34&rev1=3.33), probably from before bookmark keywords (or their use from the location bar) existed.
Oi. :)

While it is confusing, I'm assuming it is difficult and not worth attempting to change the event name and any other references there may be to "keyword-search" (nevermind confusing some people). Would you agree?

If so, I would like to add a comment where the event is created and broadcasted from (docshell/base/nsDefaultURIFixup.cpp) to at least help clarify for future travellers, as well as reword the related Android code to be more specific. Does this sound reasonable?
Attachment #803301 - Flags: review?(rnewman)
Attachment #803300 - Attachment is obsolete: true
Attachment #803301 - Attachment is obsolete: true
Attachment #803303 - Attachment is obsolete: true
Attachment #807293 - Flags: review?(gavin.sharp)
Comment on attachment 803303 [details] [diff] [review]
03: Update comment

I realized this is still relevant!
Attachment #803303 - Attachment is obsolete: false
I decided not to change the code/events at all since I think it's more important to be consistent with desktop, so I added some comments at key points in the code to add clarity.
Attachment #807314 - Flags: review?(rnewman)
Comment on attachment 807293 [details] [diff] [review]
01: Add comment clarifying "keyword-search"

I'm not sure this clarifies much, since from my perspective this code (nsDefaultURIFixup) is fairly obviously not related to bookmark keywords; the entire purpose of this code is for "fixing up" text entered into the location bar. I guess it doesn't hurt.
Attachment #807293 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)

> I'm not sure this clarifies much, since from my perspective this code
> (nsDefaultURIFixup) is fairly obviously not related to bookmark keywords…

The Android equivalent method is:

* You typed something.
* Was it a URL? If so, load it.
* If not, did it start with a bookmark keyword? If so, do the appropriate search.
* If not, dispatch it to Gecko to handle, presumably as a search term.

which will lead to incorrect expectations when a non-desktop engineer ends up in the desktop 'equivalent'. Breadcrumbs for the uninformed!
Attachment #807314 - Flags: review?(rnewman) → review+
Whiteboard: [fixed in fx-team][qa+]
https://hg.mozilla.org/mozilla-central/rev/d725f6930fbd
https://hg.mozilla.org/mozilla-central/rev/50a9cd1bc956
https://hg.mozilla.org/mozilla-central/rev/06c93b10958f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team][qa+] → [qa+]
Target Milestone: --- → Firefox 27
Realized that this turned into a comment-only bug, so no [qa+].
Whiteboard: [qa+] → [qa-]
Target Milestone: Firefox 27 → ---
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.