Closed Bug 1380771 Opened 2 years ago Closed 2 years ago

Add ability to set suggest_url to addEngineWithDetails

Categories

(Firefox :: Search, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(3 files)

Chrome supports setting a search URL when adding an engine. In order for us to do that, we need to update addEngineWithDetails to support a suggest URL
Per our discussion, I've modified addEngineWithDetails to take an object as the second parameter to specify all parameters.

I also thought that maybe since name and template were the only required params that they should be the first two parameters with object as the third, but the code would look strange (using iconURL as the template).

Anyway, looking for validation that this is what you were thinking.

As far as tests go, should I just duplicate the addEngineWithDetails test to pass an object? Or actually change those tests?
Attachment #8886312 - Flags: feedback?(florian)
Comment on attachment 8886312 [details] [diff] [review]
Preliminary patch

Review of attachment 8886312 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is the direction I suggested :).

Tests should cover both ways to call this method, so some of them will need to be duplicated, yes.

::: netwerk/base/nsIBrowserSearchService.idl
@@ +355,3 @@
>     */
>    void addEngineWithDetails(in AString name,
> +                            [optional] in jsval iconURL,

You'll need to explain this magic 'jsval' in the comment of course :)

::: toolkit/components/search/nsSearchService.js
@@ +1821,5 @@
>                  "Can't call _initFromMetaData on a readonly engine!",
>                  Cr.NS_ERROR_FAILURE);
>  
> +                Components.utils.reportError(aParams.template);
> +    let method = aParams.method ? aParams.method : "GET";

let method = aParams.method || "GET";

@@ +4028,5 @@
> +    var params;
> +
> +    if (typeof arguments[1] == "object") {
> +      params = aIconURL;
> +    } else {

We should check arguments.length here, and throw if we don't have enough parameters.

@@ +4030,5 @@
> +    if (typeof arguments[1] == "object") {
> +      params = aIconURL;
> +    } else {
> +      params = {
> +        iconURL: aIconURL,

if you rename the parameters to no longer have the 'a' prefixes, you can simplify to:
 params = {
   iconURL,
   alias,
 ...
Attachment #8886312 - Flags: feedback?(florian) → feedback+
> We should check arguments.length here, and throw if we don't have enough parameters.

I tried that, but because the IDL specifies 7 parameters (even optional), we get 7 as the arguments.length every time. Is that expected?
This patch doesn't include the WebExtensions change to support this, just the change to addEngineWithDetails.
Duplicate of this bug: 1374698
Comment on attachment 8886358 [details]
Bug 1380771 - Add support for suggest_url to addEngineWithDetails.

https://reviewboard.mozilla.org/r/157100/#review164268

::: netwerk/base/nsIBrowserSearchService.idl:333
(Diff revision 1)
>     *        The search engine's name. Must be unique. Must not be null.
>     *
>     * @param iconURL
>     *        Optional: A URL string pointing to the icon to be used to represent
>     *        the engine.
> +   *        This is a jsval so that a object can be passed to replace the

'an object'?

::: netwerk/base/nsIBrowserSearchService.idl:356
(Diff revision 1)
>     *
>     * @param extensionID [optional]
>     *        Optional: The correct extensionID if called by an add-on.
> +   *
> +   * Alternatively, all of these parameters except for name can be
> +   * passed an an object in place of parameter two.

as an

Maybe give an example here?

::: netwerk/base/nsIBrowserSearchService.idl:359
(Diff revision 1)
> +   *
> +   * Alternatively, all of these parameters except for name can be
> +   * passed an an object in place of parameter two.
>     */
>    void addEngineWithDetails(in AString name,
> -                            in AString iconURL,
> +                            [optional] in jsval iconURL,

This parameter shouldn't be optional, right?

::: toolkit/components/search/nsSearchService.js:1824
(Diff revision 1)
> -                                                    aTemplate, aExtensionID) {
>      ENSURE_WARN(!this._readOnly,
>                  "Can't call _initFromMetaData on a readonly engine!",
>                  Cr.NS_ERROR_FAILURE);
>  
> -    this._urls.push(new EngineURL(URLTYPE_SEARCH_HTML, aMethod, aTemplate));
> +                Components.utils.reportError(aParams.template);

Cu

::: toolkit/components/search/nsSearchService.js:1827
(Diff revision 1)
>                  Cr.NS_ERROR_FAILURE);
>  
> -    this._urls.push(new EngineURL(URLTYPE_SEARCH_HTML, aMethod, aTemplate));
> +                Components.utils.reportError(aParams.template);
> +    let method = aParams.method || "GET";
> +    this._urls.push(new EngineURL(URLTYPE_SEARCH_HTML, method, aParams.template));
> +    if (aParams.suggest_url) {

This is the only parameter name with an _. Should it be suggestURL instead?

The comment in the idl interface should document that it's possible to add an engine with a sugguestions url.

Your test needs to cover this new feature.

::: toolkit/components/search/tests/xpcshell/xpcshell.ini:99
(Diff revision 1)
>  [test_currentEngine_fallback.js]
>  [test_require_engines_in_cache.js]
>  [test_svg_icon.js]
>  [test_searchReset.js]
>  [test_addEngineWithDetails.js]
> +[test_addEngineWithDetailsNew.js]

"New" isn't really descriptive here, maybe use "Object" instead?
Attachment #8886358 - Flags: review?(florian) → review-
Attached patch Local diffSplinter Review
Adding an hg export diff since I can't use MozReview right now for Florian.
Attachment #8890525 - Flags: review?(florian)
Comment on attachment 8890525 [details] [diff] [review]
Local diff

Review of attachment 8890525 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, sorry for the delay.

::: toolkit/components/search/tests/xpcshell/test_addEngineWithDetailsObject.js
@@ +30,5 @@
> +
> +  // An engine added with addEngineWithDetails should have a load path, even
> +  // though we can't point to a specific file.
> +  let engine = Services.search.getEngineByName(kSearchEngineID);
> +  do_check_eq(engine.wrappedJSObject._loadPath, "[other]addEngineWithDetails");

This looks like a good opportunity to test your changes from bug 1384709.
Attachment #8890525 - Flags: review?(florian) → review+
> This looks like a good opportunity to test your changes from bug 1384709.

Definitely. I'll check that patch in first and then update this test.

Thanks!
Comment on attachment 8886358 [details]
Bug 1380771 - Add support for suggest_url to addEngineWithDetails.

https://reviewboard.mozilla.org/r/157100/#review172906
Attachment #8886358 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/16c09e5f2758
Add support for suggest_url to addEngineWithDetails. r=florian
null is an object. New patch submitted. I'll try build this one.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/b3291d3c624d
Add support for suggest_url to addEngineWithDetails. r=florian
https://hg.mozilla.org/mozilla-central/rev/b3291d3c624d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1390153
You need to log in before you can comment on or make changes to this bug.