Last Comment Bug 408251 - When middle+ or shift+clicking suggestion entries in the search bar autocomplete popup, the selected search term isn't entered into the searchbar history
: When middle+ or shift+clicking suggestion entries in the search bar autocompl...
Status: RESOLVED FIXED
[mentor=gavin]
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 24
Assigned To: giovanni_marquez_cs
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-13 13:10 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2013-06-05 03:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Moved formHistory.update from handleSearchCommand to doSearch (2.38 KB, patch)
2013-05-24 15:52 PDT, giovanni_marquez_cs
gavin.sharp: feedback+
Details | Diff | Splinter Review
update to last one (2.39 KB, patch)
2013-05-31 16:12 PDT, giovanni_marquez_cs
no flags Details | Diff | Splinter Review
recreated patch (2.39 KB, patch)
2013-05-31 20:09 PDT, giovanni_marquez_cs
gavin.sharp: review+
Details | Diff | Splinter Review
Updated description of bug. (2.41 KB, patch)
2013-06-04 18:33 PDT, giovanni_marquez_cs
gavin.sharp: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 13:10:04 PST
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.
Comment 1 giovanni_marquez_cs 2013-04-18 00:54:29 PDT
Hello. Me and my partner would like to take this bug for our class assignment, if we may.
Comment 2 Matthew N. [:MattN] (In Taipei until Sep. 23) 2013-04-18 00:58:14 PDT
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
Comment 3 giovanni_marquez_cs 2013-05-18 15:52:04 PDT
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.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-20 13:13:27 PDT
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
Comment 5 giovanni_marquez_cs 2013-05-20 23:02:11 PDT
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?
Comment 6 Colby Russell :crussell (no longer Mozilla-ing) 2013-05-21 07:43:42 PDT
(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
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-21 14:13:50 PDT
(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 :)
Comment 8 Colby Russell :crussell (no longer Mozilla-ing) 2013-05-21 16:40:10 PDT
(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.
Comment 9 giovanni_marquez_cs 2013-05-21 19:48:16 PDT
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.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-23 10:43:07 PDT
(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.
Comment 11 giovanni_marquez_cs 2013-05-24 15:14:43 PDT
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.
Comment 12 giovanni_marquez_cs 2013-05-24 15:52:39 PDT
Created attachment 753998 [details] [diff] [review]
Moved formHistory.update from handleSearchCommand to doSearch
Comment 13 giovanni_marquez_cs 2013-05-24 15:53:50 PDT
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.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-24 15:56:01 PDT
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.
Comment 15 giovanni_marquez_cs 2013-05-24 16:30:44 PDT
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
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-24 16:54:41 PDT
https://wiki.mozilla.org/IRC has some extra info. I don't see you online at the moment.
Comment 17 giovanni_marquez_cs 2013-05-24 17:15:50 PDT
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?
Comment 18 giovanni_marquez_cs 2013-05-28 20:12:46 PDT
Have you gotten a chance to review the patch, Gavin?
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-29 14:28:10 PDT
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 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-29 14:30:16 PDT
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.
Comment 21 giovanni_marquez_cs 2013-05-29 21:33:18 PDT
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?
Comment 22 giovanni_marquez_cs 2013-05-31 16:08:27 PDT
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.
Comment 23 giovanni_marquez_cs 2013-05-31 16:12:27 PDT
Created attachment 756843 [details] [diff] [review]
update to last one
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-31 16:20:22 PDT
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.
Comment 25 giovanni_marquez_cs 2013-05-31 20:09:31 PDT
Created attachment 756907 [details] [diff] [review]
recreated patch
Comment 26 giovanni_marquez_cs 2013-05-31 20:13:48 PDT
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.
Comment 27 giovanni_marquez_cs 2013-06-03 21:39:06 PDT
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 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-04 12:52:01 PDT
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)
Comment 29 giovanni_marquez_cs 2013-06-04 18:33:10 PDT
Created attachment 758340 [details] [diff] [review]
Updated description of bug.

I hope you get a chance to review this on Wednesday (tomorrow) so I can meet the deadline. Thank you Gavin!
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-04 22:31:23 PDT
Comment on attachment 758340 [details] [diff] [review]
Updated description of bug.

a+ :)
Comment 31 giovanni_marquez_cs 2013-06-04 22:57:19 PDT
Great! Thank you so much! It's been a pleasure working with you. :)
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-04 23:22:22 PDT
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!
Comment 33 Ed Morley [:emorley] 2013-06-05 03:44:15 PDT
https://hg.mozilla.org/mozilla-central/rev/c346868fe2c3

Note You need to log in before you can comment on or make changes to this bug.