If a keyword search is performed from a different place than the locationbar, when using the autocomplete popup for a new search with the same keyword, the purpose parameter will not update. We want all the searches from the autocomplete popup to use the "keyword" purpose. This will introduce a known duplication which will be handled in a separate bug.

I've used URLSearchParams (https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams) to do some query string manipulation, but I saw that after I call .toString(), the order of the parameters is changed and this is not ok. I will change it in a future patch + add tests, I just want some feedback to see how it looks right now and what I should improve.

Comment on attachment 8486241 [details] [diff] [review] WIP Use `keyword` purpose for searches from awesomebar Patch 1 Review of attachment 8486241 [details] [diff] [review]: ----------------------------------------------------------------- Hi Alex, I can leave some quick tips on the patch, however I don't think I'll be able to do a full review promptly, you may try asking Marco or Gavin later, or just wait a few days. ::: netwerk/base/public/nsIBrowserSearchService.idl @@ +216,5 @@ > + > + /** > + * The type of the URL passed in to nsISearchEngine::parseSubmissionURL. > + */ > + readonly attribute AString urlType; This would definitely need a better name and description in the comment, maybe an example of possible value, I couldn't tell what to expect in this parameter. ::: toolkit/components/places/PlacesSearchAutocompleteProvider.jsm @@ +153,5 @@ > > let parseUrlResult = Services.search.parseSubmissionURL(url); > return parseUrlResult.engine && { > engineName: parseUrlResult.engine.name, > + engineUrl: parseUrlResult.engine.wrappedJSObject._getURLOfType(parseUrlResult.urlType), Hm, the call to wrappedJSObject makes me think that we need a different, exposed way to access what we need. ::: toolkit/components/places/UnifiedComplete.js @@ +838,5 @@ > + // If the URL represents a search result, then try to restyle it and change its purpose > + // to "keyword". This is the default engine's "locationbar" purpose. > + if (parseResult) { > + this._maybeRestyleSearchMatch(match, parseResult); > + this._changePurpose(match, parseResult, "keyword"); I don't particularly like that we're passing a parseResult parameter around, on the other hand having separate functions calls make this cleaner, so I don't have a strong opinion here. ::: toolkit/components/search/nsSearchService.js @@ +863,5 @@ > this.mozparams[aObj.name] = aObj; > }, > > + // Change the purpose of the given URL with a given purpose. > + changeSubmissionURLPurpose: function SRCH_EURL_changeSubmissionURLPurpose(aURL, newPurpose) { It would be useful to have a better description explaining why the function exists and what the "purpose" is.

Hi Alex, can you mark this bug as qe-verify+ or qe-verify-.

I've added xpcshell tests and modified the logic for replacing / removing / adding search parameters related to the purpose. Removing the purpose search parameters is easy, but replacing one is not trivial because new search params can be added or removed, depending on the purpose. Example: homepage uses 2 purpose search params (channel & source), but others are using only 1 right now (channel).

Same comment as in the previous attachment. I've also added more comments in the code.

changeSubmissionURLPurpose looks complicated. Why can't you just call getSubmission again with ParseSubmissionResult.terms?

Also, I don't think it's necessary to support non-text/html urlTypes.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6) > changeSubmissionURLPurpose looks complicated. Why can't you just call > getSubmission again with ParseSubmissionResult.terms? Because we need to preserve any extra parameters added by the search engine, for example indicating that this is an image rather than a text search.

Of course, another approach is to pass the original URL as an extra option to getSubmission, which will then just alter it. Not sure which is best.

Comment on attachment 8486850 [details] [diff] [review] WIP Use `keyword` purpose for searches from awesomebar Patch 2 Review of attachment 8486850 [details] [diff] [review]: ----------------------------------------------------------------- Here are some high-level comments. I'm still looking at the changeSubmissionURLPurpose implementation, so I'll comment on that next and leave the feedback? for now. Paolo is right that using wrappedJSObject is no good. I think we should either add a new URL param to nsISearchEngine.getSubmission like Paolo suggests, or add a changePurpose method and a parsedURL attribute to nsISearchParseSubmissionResult. The nsISearchParseSubmissionResult would use parsedURL to reconstruct the URL with the new submission. (But see my comment below about PlacesSearchAutocompleteProvider.parseSubmissionURL.) I'm not sure which is better. I guess nsISearchEngine.getSubmission is, since you may have a URL you're not interested in parsing but whose purpose you want to change. ::: netwerk/base/public/nsIBrowserSearchService.idl @@ +194,5 @@ > */ > readonly attribute nsISearchEngine engine; > > /** > + * String containing the sought terms. This can be an empty string in case no It looks like you're only removing a space here? I don't think that's necessary. ::: toolkit/components/places/UnifiedComplete.js @@ +775,5 @@ > // lower to higher. > this._frecencyMatches.sort((a, b) => a.frecency - b.frecency); > }, > > + _maybeRestyleSearchMatch: function (match, parseResult) { Instead of changing this method to take a parseResult, I think you should keep it the way it is, and move the body of your _changePurpose here, and then finally rename this method something like _maybeFixUpSearchMatch since it's not only changing the style. Not a big deal, though. @@ -778,5 @@ > > - _maybeRestyleSearchMatch: function (match) { > - // Return if the URL does not represent a search result. > - let parseResult = > - PlacesSearchAutocompleteProvider.parseSubmissionURL(match.value); I wonder why we call PlacesSearchAutocompleteProvider.parseSubmissionURL here and not Services.search.parseSubmissionURL... The PlacesSearchAutocompleteProvider version returns a simple JS object instead of an nsISearchParseSubmissionResult. In fact it looks like this is the only caller of PlacesSearchAutocompleteProvider.parseSubmissionURL in the tree... I don't understand why it exists at all.

(In reply to Drew Willcoxon :adw from comment #10) > I'm not sure which is better. I guess nsISearchEngine.getSubmission is, > since you may have a URL you're not interested in parsing but whose purpose > you want to change. whose purpose you want to *set*, rather.

Thank you for the feedback and comments. changeSubmissionURLPurpose is complicated indeed. I tried to add enough comments and make it efficient and I'm thinking of adding more xpcshell tests, for cases when a URL has many other parameters. But even if it's complicated, it does its job for this use case. Unfortunately, URLSearchparams doesn't work as expected (it is implemented as a Map and it will mix the order of search params), so I basically had to write an implementation for it which preserves the search param order. I'd be more than happy to simplify it, but also keep its functionality. I've also added a patch for bug 1063542 , which is based on this one. With both patches applied, duplicated results when searching from different places will not appear anymore, so the main idea behind those implementations is good (as it was expected).

Comment on attachment 8486850 [details] [diff] [review] WIP Use `keyword` purpose for searches from awesomebar Patch 2 Review of attachment 8486850 [details] [diff] [review]: ----------------------------------------------------------------- Hey Alex, sorry this took so long. I think changeSubmissionURLPurpose is a little hard to follow. Basically I think all that should happen is that we iterate over the passed-in URL's params, keeping params that aren't purpose params, updating params that are purpose params that match the new purpose, and discarding params that are purpose params that don't match the new purpose. Then you add the remaining new purpose params to the end. I started describing things in English but pretty soon it just became easier to write code to explain. This is untested: // First, set up three helper structures. // an array: [[name_0, val_0], [name_1, val_1], ...] let urlPairs = urlQueryString.split("&").map(pairStr => pairStr.split("=")); // a Map: param.name => (param.purpose => param.value) let allPurposeParams = engineURL.params.reduce((memo, param) => { if (param.purpose !== undefined) { let map = memo.get(param.name) || new Map(); map.set(param.purpose, value); memo.set(param.name, map); } return memo; }, new Map()); // a Map: param.name => param.value let newPurposeParams = engineURL.params.reduce((memo, param) => { if (param.purpose !== undefined && param.purpose == newPurpose) { memo.set(param.name, param.value); } return memo; }, new Map()); // Now, copy/replace params from the passed-in URL. This the main part. let finalPairs = []; for (let [name, val] of urlPairs) { if (!allPurposeParams.has(name)) { finalPairs.push([name, val]); } else if (newPurposeParams.has(name)) { finalPairs.push([name, newPurposeParams.get(name)]); newPurposeParams.delete(name); } } finalPairs.push(...newPurposeParams); // Finally, make the new URL. let finalQueryStr = finalPairs.map(pair => pair.join("=")).join("&"); let finalURL = urlBeforeQueryStr + "?" + finalQueryStr;

Hmm, not sure what I was thinking with allPurposeParams. It doesn't need to be that complex Map. Just a Set: let allPurposeParamNames = engineURL.params.reduce((memo, param) => { if (param.purpose !== undefined) { memo.add(param.name); } return memo; }, new Set());

(In reply to Drew Willcoxon :adw from comment #10) > I wonder why we call PlacesSearchAutocompleteProvider.parseSubmissionURL > here and not Services.search.parseSubmissionURL... The > PlacesSearchAutocompleteProvider version returns a simple JS object instead > of an nsISearchParseSubmissionResult. In fact it looks like this is the > only caller of PlacesSearchAutocompleteProvider.parseSubmissionURL in the > tree... I don't understand why it exists at all. Just as a side note, we are trying to make PlacesSearchAutocompleteProvider a layer of abstraction around search service, so that we can change its underlying implementation without complicating our life too much (and cause it's sort of a dependency on another service that is not in Places)

And also the fact we ensure PlacesSearchAutocompleteProvider must be initialized and thus we can take care of search service async init behavior internally.

(In reply to :Paolo Amadini from comment #8) > (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6) > > changeSubmissionURLPurpose looks complicated. Why can't you just call > > getSubmission again with ParseSubmissionResult.terms? > > Because we need to preserve any extra parameters added by the search engine, > for example indicating that this is an image rather than a text search. I think we need to step back here and consider the overall goal and review which cases we care to handle. Modifying e.g. Google image searches to use the "correct" purpose is not something I would consider in scope. I might even argue that we should ignore all searches that contain parameters outside of the set of parameters that ship with our search plugins, both for the "modify purpose" and the "highlight as a search" behaviors. I think that should simplify things significantly.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17) > I might > even argue that we should ignore all searches that contain parameters > outside of the set of parameters that ship with our search plugins, both for > the "modify purpose" and the "highlight as a search" behaviors. I think that > should simplify things significantly. then likely we will style and parse only the searches started from Firefox itself, any other search would stay un-parsable by the user's eyes.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17) > I might > even argue that we should ignore all searches that contain parameters > outside of the set of parameters that ship with our search plugins, both for > the "modify purpose" and the "highlight as a search" behaviors. I think that > should simplify things significantly. I think the search engine may actually redirect our searches to a similar but different URL, that has more parameters, like session identifiers. In this case, not even the searches started from Firefox would be identified. If we only, or mainly, care about searches started from Firefox itself, a totally different architecture would involve adding some metadata in our history at the time the search is executed, independently from the URL.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #15) > Just as a side note, we are trying to make PlacesSearchAutocompleteProvider > a layer of abstraction around search service, so that we can change its > underlying implementation without complicating our life too much (and cause > it's sort of a dependency on another service that is not in Places) Hmm, if that's the case then Alex will need to add a method on PlacesSearchAutocompleteProvider, or on the object returned by PlacesSearchAutocompleteProvider.parseSubmissionURL, that ends up calling a new changePurpose method on the nsISearchEngine, as Paolo and I have suggested. At that point it seems simpler for PlacesSearchAutocompleteProvider.parseSubmissionURL to simply return the nsISearchParseSubmissionResult directly, or for Alex to call Services.search.parseSubmissionURL directly. (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17) > I think we need to step back here and consider the overall goal and review > which cases we care to handle. Modifying e.g. Google image searches to use > the "correct" purpose is not something I would consider in scope. I might > even argue that we should ignore all searches that contain parameters > outside of the set of parameters that ship with our search plugins, both for > the "modify purpose" and the "highlight as a search" behaviors. I think that > should simplify things significantly. I echo what Marco and Paolo said. It's not that complicated to update query params as necessary, thereby treating all search URLs the same regardless of where they originated. And it's the right thing to do from a UX perspective. I don't know why we wouldn't do it.

I've used most of the suggestions and added more tests. For me, it looks better now and the new implementation of changePurpose is simpler to read & understand.

Comment on attachment 8488875 [details] [diff] [review] Use `keyword` purpose for searches from awesomebar Patch 3 Review of attachment 8488875 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIBrowserSearchService.idl @@ +194,5 @@ > */ > readonly attribute nsISearchEngine engine; > > /** > + * String containing the sought terms. This can be an empty string in case no This isn't necessary. @@ +214,5 @@ > */ > readonly attribute long termsLength; > + > + /** > + * The URL passed in to nsISearchEngine::parseSubmissionURL in order to be parsed Just say, The URL passed in to nsISearchEngine::parseSubmissionURL. @@ +224,5 @@ > + * The type of the URL passed in to nsISearchEngine::parseSubmissionURL in order > + * to obtain an EngineURL instance from an Engine object. > + * example: "text/html". > + */ > + readonly attribute AString urlType; I don't think we need to expose this in the interface after all. Engine.getURLParsingInfo gets the EngineURL, by calling this._getURLOfType(responseType). It should just return that EngineURL in the object it returns. Then SearchService._buildParseSubmissionMap would add urlParsingInfo.engineURL to the mapValueForEngine object. Finally, SearchService.parseSubmissionURL would pass mapEntry.engineURL to new ParseSubmissionResult(). @@ +227,5 @@ > + */ > + readonly attribute AString urlType; > + > + /** > + * Changes the purpose of the URL with a given purpose. The purpose represents Changes the purpose-related parameters of the parsed URL to the parameters associated with the given purpose. Also, this should mention that (1) params unrelated to search are not modified or removed, and (2) pass the empty string to remove all params the from the parsed URL that are related to the purpose. @@ +232,5 @@ > + * the location from which the user performed a search (e.g: homepage, newtab, searchbar). > + * > + * Example 1: > + * url: https://...?q=mozilla&channel=sb > + * aPurpose: keyword Please put the aPurpose string in quotes, here and in the other examples, e.g., "keyword". @@ +242,5 @@ > + * aPurpose: keyword > + * Returns: > + * https://../?q=mozilla&channel=fflib > + * > + * Example 3: Please add a fourth example showing what happens when you pass the empty string. @@ +248,5 @@ > + * aPurpose: homepage > + * Returns: > + * https://../?q=mozilla&channel=np&source=hp > + * > + * @see .../searchplugins/google.xml Please remove this. @@ +252,5 @@ > + * @see .../searchplugins/google.xml > + * > + * @param {String} aPurpose > + * A string representing the new purpose. > + * @returns {String} A string, representing the initial url but with the query string Please remove these two {String}s. It's not necessary to document types in IDL because IDL is self-documenting in that regard. Also, nit: Please remove the trailing space at the end of this line. @@ +255,5 @@ > + * A string representing the new purpose. > + * @returns {String} A string, representing the initial url but with the query string > + * purpose parameters replaced. > + **/ > + AString changePurpose(in AString aPurpose); Nit: Please call this param aNewPurpose. ::: toolkit/components/places/UnifiedComplete.js @@ +830,5 @@ > // Restyle past searches, unless they are bookmarks or special results. > if (match.style == "favicon") { > + let parseResult = PlacesSearchAutocompleteProvider.parseSubmissionURL(match.value); > + // If the URL represents a search result, then try to restyle it and change its purpose > + // to "keyword". This is the default engine's "locationbar" purpose. Nit: Try to keep lines to 80 chars. ::: toolkit/components/search/nsSearchService.js @@ +2879,5 @@ > + return this.url; > + } > + // Extract the search params into an array of [name, value] arrays > + // e.g.: [[name_0, val_0], [name_1, val_1], ...] > + let queryStrPairs = this.url.slice(offset).split("&").map(pairStr => pairStr.split("=")); Nit: I'd prefer the name queryPairs -- queryStrPairs says to me that the elements in the array are strings, due to the "Str" part of the name. But I realize that may just be me, so you don't have to change it if you disagree. @@ +2898,5 @@ > + return memo; > + }, new Map()); > + > + // Copy / replace params from the passed-in URL > + let finalQueryStrPairs = []; Same nit with this name. ::: toolkit/components/search/tests/xpcshell/test_changePurpose.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ This isn't exactly the right header: the indentation on the second line is off. Please use the right header character for character: https://www.mozilla.org/MPL/headers/ Also, this file has a lot of trailing spaces. Please remove them. @@ +1,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/* > + * Tests method from nsISearchParseSubmissionResult::changePurpose. Tests method nsISearchParseSubmissionResult::changePurpose. @@ +14,5 @@ > + > + run_next_test(); > +} > + > +add_task(function* test_changeSubmissionURLPurpose() { Nit: Should call this test_changePurpose. @@ +16,5 @@ > +} > + > +add_task(function* test_changeSubmissionURLPurpose() { > + let url = "http://www.google.com/search?q=firefox&channel=sb"; > + let result = Services.search.parseSubmissionURL(url); Please use a mock search engine instead of relying on the real Google URL and params, like test_purpose.js does: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_purpose.js#21 Actually you might even add your test to test_purpose.js instead of making a new file.

I hope I touched almost all points from previous feedback.

Comment on attachment 8489816 [details] [diff] [review] Use `keyword` purpose for searches from awesomebar Patch 4 Review of attachment 8489816 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/UnifiedComplete.js @@ +866,5 @@ > + // change its purpose to "keyword". This is the default engine's > + // "locationbar" purpose. > + if (parseResult) { > + this._maybeRestyleSearchMatch(match, parseResult); > + match.value = parseResult.changePurpose("keyword"); is there a reason this code can't stay inside _maybeRestyleSearchMatch? I don't like to complicate the code here if we can delegate to an helper. Why can't _maybeRestyleSearchMatch just change match.value?

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #24) > is there a reason this code can't stay inside _maybeRestyleSearchMatch? I > don't like to complicate the code here if we can delegate to an helper. Why > can't _maybeRestyleSearchMatch just change match.value? _maybeRestyleSearchMatch should be able to change match.value. We will eventually need to have the parseResult outside _maybeRestyleSearchMatch to first remove the purpose, then change it to keyword (bug 1063542).

I've used Marco suggestion and also removed engineURL from the idl.

Hi Marco, what is your take on this bug? Is this something we should still be considering.

(In reply to :shell escalante from comment #27) > Hi Marco, what is your take on this bug? Is this something we should still > be considering. Let me recap a little bit here, since it's very unclear what's up here. This was originally introduced as a dependency of a feature, that is currently preffed-off by browser.urlbar.restyleSearches hidden pref. The scope of the feature is to restyle past searches, done with installed search engines, so that instead of an unreadable url, the user gets a nicely parsed entry like "search terms - Engine Name" in awesomebar entries. The feature itself has some downsides that have not yet been technically solved, more specifically the fact it's not possible at the moment to distinguish a text search from an image/video search, when the engine uses the same domain and just changes some params (params differ for each engine too). For the user all of the searches would look the same :( This technical problem is what keeps the feature disabled. Discussion whether we still want that feature should likely happen elsewhere, I just reported it here for completeness. Though, regardless of the feature above, we have another problem that is what this bug is about. By re-executing a query, we pass the old "purpose" to the search engine, that means tracking the search origin is broken. For example if you search from about:home, and then re-execute the same search from the awesomebar result, it will still pass the about:home purpose, so it will look like you searched from about:home. This is what this bug is aimed to fix. (note this is a longstanding bug). We might still want to replace the purpose param of urls selected from the awesomebar to properly fix tracking of search origins, but at this point it's more a marketing/PM decision, than a technical problem. Considered this happens from a long time and we are unlikely to parse search urls shortly (feature above) it's likely not a priority. Bug 1063542 (https://bugzilla.mozilla.org/show_bug.cgi?id=1063542#c7) also states there were policy reasons (not sure which ones) to avoid de-duplication of results based on purpose, so there might be a marketing/legal issue I don't know about(?!).

bug 1071361 is related in the sense if we decide to introduce a "keyword" purpose here, we should try to use it everywhere the locationbar causes a search through an installed engine.