1169459 - remove the loadFromJars/jarURIs prefs

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Florian Quèze [:florian]]

There is no good reason for this behavior to be pref-controlled.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

I think this works. I'm not requesting review now because I wrote this above other patches in my queue that I want to land first, but that are not finished yet.

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch v2

Unbitrotted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d010756d7de

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8656024 [details] [diff] [review]
Patch v2

Try seems green; requesting review.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8656024 [details] [diff] [review]
Patch v2

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

Looks good to me!

::: toolkit/components/search/nsSearchService.js
@@ +935,5 @@
> +                                   Services.scriptSecurityManager.getSystemPrincipal(),
> +                                   null, // aTriggeringPrincipal
> +                                   Ci.nsILoadInfo.SEC_NORMAL,
> +                                   Ci.nsIContentPolicy.TYPE_OTHER);
> +  } catch (ex) { }

This would be simplified with NetUtil.newChannel

@@ +4232,4 @@
>    _findJAREngines: function SRCH_SVC_findJAREngines() {
>      LOG("_findJAREngines: looking for engines in JARs")
>  
> +    let chan = makeChannel("resource://search-plugins/list.txt");

Can we make "resource://search-plugins/" a constant up the top and add a comment explaining that we look there for app search plugins.

::: xpcom/io/nsAppFileLocationProvider.cpp
@@ +583,5 @@
>      NS_ADDREF(*aResult);
>      rv = NS_OK;
>    }
>    if (!nsCRT::strcmp(aProp, NS_APP_SEARCH_DIR_LIST)) {
> +    static const char* keys[] = { nullptr, NS_APP_USER_SEARCH_DIR, nullptr };

Looks like Thunderbird and Seamonkey still use this. Any harm in leaving it here or do we need to file bugs to have them update?

Comment 5

[Florian Quèze [:florian]]

Thanks for the review!

(In reply to Dave Townsend [:mossop] from comment #4)

> ::: xpcom/io/nsAppFileLocationProvider.cpp
> @@ +583,5 @@
> >      NS_ADDREF(*aResult);
> >      rv = NS_OK;
> >    }
> >    if (!nsCRT::strcmp(aProp, NS_APP_SEARCH_DIR_LIST)) {
> > +    static const char* keys[] = { nullptr, NS_APP_USER_SEARCH_DIR, nullptr };
> 
> Looks like Thunderbird and Seamonkey still use this. Any harm in leaving it
> here or do we need to file bugs to have them update?

They both override NS_APP_SEARCH_DIR_LIST:
http://mxr.mozilla.org/comm-central/source/mail/components/shell/DirectoryProvider.cpp#194
http://mxr.mozilla.org/comm-central/source/suite/profile/nsSuiteDirectoryProvider.cpp#66

Instantbird may need to start packaging search plugins in omni.ja.

Adding the NS_APP_DISTRIBUTION_SEARCH_DIR_LIST empty enumerator here was actually so that other xul apps don't have to provide that themselves, but it seems at this point Thunderbird has already been updated to support it: http://mxr.mozilla.org/comm-central/source/mail/components/shell/DirectoryProvider.cpp#182
May still benefit SeaMonkey or Instantbird.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/925475a10f08

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/925475a10f08