Closed Bug 1169459 Opened 6 years ago Closed 5 years ago

remove the loadFromJars/jarURIs prefs

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [fxsearch][hijacking])

Attachments

(1 file, 1 obsolete file)

There is no good reason for this behavior to be pref-controlled.
Attached patch Patch (obsolete) — Splinter Review
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.
Whiteboard: [fxsearch][hijacking]
Rank: 7
Flags: firefox-backlog+
Priority: -- → P1
Comment on attachment 8656024 [details] [diff] [review]
Patch v2

Try seems green; requesting review.
Attachment #8656024 - Flags: review?(markh)
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?
Attachment #8656024 - Flags: review?(markh) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/925475a10f08
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.