Closed Bug 408251 Opened 17 years ago Closed 11 years ago

When middle+ or shift+clicking suggestion entries in the search bar autocomplete popup, the selected search term isn't entered into the searchbar history

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: Gavin, Assigned: giovanni_marquez_cs)

Details

(Whiteboard: [mentor=gavin])

Attachments

(1 file, 3 obsolete files)

The patch for bug 361735 (and the patch in bug 405269 that re-fixed it after it
was broken) made us support Shift+Click and Ctrl+Click in the searchbar autocomplete popup to open the search in a new window/tab. This bypasses handleSearchCommand, though, so it means we don't put the selected search term in the autocomplete history. This only affects suggestions, because the other entries are already in the form history :)

We could potentially fix this by moving the call to textBox._formHistSvc.addEntry() into doSearch.
Whiteboard: [mentor=gavin]
Hello. Me and my partner would like to take this bug for our class assignment, if we may.
Welcome! if you have any questions about the bug, feel free to ask here.  You can get general Mozilla development help in #introduction or at https://developer.mozilla.org/en-US/docs/Introduction
Assignee: nobody → giovanni_marquez_cs
Status: NEW → ASSIGNED
Sorry for the delay. I have begun working on this bug but I am overwhelmed by the amount of code and am having a hard time finding the code that adds a search to the history. I assume it's under browser, but from there I've been looking and just can't find it. Is there any way you could help me narrow down my search?

Thank you.
Hi Giovanni - thanks for following up!

Comment 0 has a good lead: it mentions "doSearch" and "textBox._formHistSvc.addEntry". That second code snippet has since been changed a bit, but if you search for doSearch on MXR there are a few relevant hits:

http://mxr.mozilla.org/mozilla-central/search?string=doSearch

The relevant code in this case is the call in browser/base/content/urlbarBindings.xml, and the function definition in browser/components/search/content/search.xml

The "handleSearchCommand" function referred to in comment 0 can also be found that way:
http://mxr.mozilla.org/mozilla-central/search?string=handleSearchCommand
Thank you Gavin, that was extremely helpful!

Having found handleSearchCommand, I believe this snippet of code handles adding a query to the history, which is followed by a call to doSearch:

// Save the current value in the form history
          if (textValue && !PrivateBrowsingUtils.isWindowPrivate(window)) {
            this.FormHistory.update(
              { op : "bump",
                fieldname : textBox.getAttribute("autocompletesearchparam"),
                value : textValue },
              { handleError : function(aError) {
                  Components.utils.reportError("Saving search to form history failed: " + aError.message);
              }});
          }


I'm assuming this is the modified version of textBox._formHistSvc.addEntry. Is this what you would like moved over to the doSearch function?

Also, I really hate to ask this since I'm sure the information is out there, but I've spent quite some time reading the guides and still can't figure out the easiest way to run a new build. I've used the ./mach build command followed by .mach run, but unless I'm changing the wrong thing, it doesn't seem to be running a version with the changes I've made. What is the easiest way to test my changes?
(In reply to giovanni_marquez_cs from comment #5)
> I've spent quite some time reading the guides and still can't figure out
> the easiest way to run a new build. I've used the ./mach build command
> followed by .mach run, but unless I'm changing the wrong thing, it doesn't
> seem to be running a version with the changes I've made.

You'll want to do development in a separate profile from your default.

(If you already have Firefox open and just do

> ./mach run

then you should get the "Firefox is already running" dialog.  Is this what you're getting?)

So create a new profile with the profile manager UI:

> ./mach run -P

Don't use "Start Nightly" after you go through the profile creation wizard.  That will set the selected profile as your default.  Instead, exit the profile manager and re-run your build by passing in the name of the new profile: e.g.,

> ./mach run -P nightly
(In reply to giovanni_marquez_cs from comment #5)
> I'm assuming this is the modified version of textBox._formHistSvc.addEntry.
> Is this what you would like moved over to the doSearch function?

Yep, exactly.

> What is the easiest way to test my changes?

As Colby mentions, if you have an existing version of Firefox running, the newly invoked one will just open a new window in that existing instance. To avoid that, you can use the -no-remote and/or -P arguments, as Colby suggests. Or you could also just close the other Firefox instances :)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> you can use the -no-remote

When I do "mach run", it uses -no-remote by default, FWIW.
I actually closed my regular Firefox and nightly before running, but it was indeed running in default. I've created a new profile and when I run it does indeed no longer carry over the information from the regular firefox build. But whenever I build it keeps telling me something about requiring a clobber build. I'm not sure if that's because I've introduced a compiling error or what. I did an hg pull -u in hopes that it would undo all my changes so i could start over, but it didn't.

This is what I am doing. I'm opening search.xml in Visual Studio C++ 2010 Express and making my changes. I then open the file start-msvc10 in mozilla-build. This is where I am running the ./mach build and then ./mach run -p myVersion. When I run it, it doesn't have the changes I've made, and I think that's because my build didn't work, but I can't tell if it's a compiling error. I've tried undoing everything I did, but I still get that clobber message. I'm not sure what to do.
(In reply to giovanni_marquez_cs from comment #9)
> But whenever I build it keeps telling me something about requiring a clobber
> build. I'm not sure if that's because I've introduced a compiling error or
> what. I did an hg pull -u in hopes that it would undo all my changes so i
> could start over, but it didn't.

That's not your fault, just means someone checked in a change that requires a clobber since you last built. Just follow the instructions in the prompt (either remove your object directory, or touch the CLOBBER file).

> I think that's because my build didn't work, but I can't tell if it's a
> compiling error. I've tried undoing everything I did, but I still get that
> clobber message. I'm not sure what to do.

./mach build will take a while, if it's returning very quickly with the clobber message, you haven't built. Follow the instructions in that message to address the problem and then try ./mach build again.
Okay, I've successfully fixed the bug and created a patch file, the problem is that I edited another file, then undid the changes but it's still showing up on the patch file. The file that has changes but shouldn't is urlBarBindings.xml. How do I restore this file? I tried a pull it it didn't do the trick.
I ended up manually removing the changes to urlBarBindings from the patch, but I would still like to know how I can return it to how it was.
It depends somewhat on how you are generating the patch. It may be best to find me on IRC (gavin on irc.mozilla.org) so we can chat in real-time.
I've never used IRC before. I just downloaded mIRC and joined irc.mozilla.org but I don't know how to find you. My user name is GiovanniMarquez.

As for how I'm generating the patch, I'm using this tutorial.

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://wiki.mozilla.org/IRC has some extra info. I don't see you online at the moment.
I'm trying to send you a message but nothing is happening. My nickname seems to have been cut down to GiovanniMarq. If you can't see me online then I don't know how to work this thing. Can we just use email instead?
Have you gotten a chance to review the patch, Gavin?
Apologies for the delayed response. It's important to remember for future patches that you should request review on the patch, to make sure it doesn't get lost. You can do that by setting the "review" flag to "?" and entering the reviewer's email address. https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch has more info about this.

For future patches, you should make sure you set up your Mercurial.ini according to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You should always feel free to email me, sure. But I get a lot of email, and debugging this stuff asynchronously can be difficult - asking in #introduction or #fx-team on IRC really probably is the easiest way to find help quickly.
Comment on attachment 753998 [details] [diff] [review]
Moved formHistory.update from handleSearchCommand to doSearch

This patch looks good. You should omit the changes to <body><![CDATA indentation since that's not related to this fix.

Also you removed a space after the "//" comment marker, can you re-add it?

You should attach a new patch that addresses these comments, and includes the configuration changes from my previous comment.
Attachment #753998 - Flags: feedback+
I followed the instructions from that link initially, so I must not be understanding how to work this. I opened that hgrc file and copy/pasted that information on there, just adding my name. Then I ran hg qnew name.patch to create the patch. What am I missing?

Also, I wanted to change the name of the patch and the new one only had the header. I'm assuming since it's a new patch, it doesn't carry over the old changes, so I don't know what to do to. Is there a simple way to just start from scratch? I want do undo all of the changes I've made and do it all over again. What is the easiest way to do it?
Well, I don't know what the configuration changes are, since I followed the directions as I stated above, and I have no clue what is wrong. Also, I cannot figure out how to start over besides deleting all the code, and re-downloading it and rebuilding. hg pull -u doesn't even work now all of a sudden. It tells me there is no default directory. So I am manually making all the changes to the patch file. If there is still something missing, can you tell me how I can add it manually? Sorry for all the trouble, but I've never created a patch before.
Attached patch update to last one (obsolete) — Splinter Review
Attachment #753998 - Attachment is obsolete: true
Attachment #756843 - Flags: review?(gavin.sharp)
It sounds like perhaps you didn't update the right file. On Windows, you need to edit Mercurial.ini in your "home" directory (%userprofile%).

You should never need to re-pull all of the code. You can generally use hg qpop and hg qremove to remove existing patches. https://developer.mozilla.org/en-US/docs/Mercurial_Queues has more detailed information, and as I mentioned before #introduction on irc.mozilla.org is a great place to get 1-1 help.
Attached patch recreated patch (obsolete) — Splinter Review
Attachment #756843 - Attachment is obsolete: true
Attachment #756843 - Flags: review?(gavin.sharp)
Attachment #756907 - Flags: review?(gavin.sharp)
hg qpop did the trick. I just poped all patches and did it from scratch.

Also, i found mecurial.ini and I placed the following in that file before creating the patch.

[ui]
username = My name <myemail>

[defaults]
qnew = -Ue

[extensions]
mq =

[diff]
git = 1
showfunc = 1
unified = 8 


With, of course, my actual name and email. I hope that's what I needed to do and that the patch is good now.

Thank you for all your help and patience.
I hate to rush you, but I need this patch accepted by this Wednesday to get credit in my class. I hope you get a chance to review it before then. Thank you.
Comment on attachment 756907 [details] [diff] [review]
recreated patch

Sorry for the delay, Giovanni!

This looks great - one minor nit: you should include the bug number in the commit message, something like:

Bug 408251: move updating of form history from handleSearchCommand to doSearch to make sure that mouse-triggered search bar queries end up recorded in form history, r=gavin

(You can update the commit message on an existing patch using hg qref -e or hg qref -m)
Attachment #756907 - Flags: review?(gavin.sharp) → review+
I hope you get a chance to review this on Wednesday (tomorrow) so I can meet the deadline. Thank you Gavin!
Attachment #756907 - Attachment is obsolete: true
Attachment #758340 - Flags: review?(gavin.sharp)
Comment on attachment 758340 [details] [diff] [review]
Updated description of bug.

a+ :)
Attachment #758340 - Flags: review?(gavin.sharp) → review+
Great! Thank you so much! It's been a pleasure working with you. :)
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c346868fe2c3

The patch should get merged to mozilla-central (and thus into the next Nightly build) within the next 24 hours, and assuming all goes well will ship with Firefox 24.

Thanks for the contribution!
Keywords: checkin-needed
Target Milestone: --- → Firefox 24
https://hg.mozilla.org/mozilla-central/rev/c346868fe2c3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: