reset the search engine on the about:home page #2

Open
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants

I have added the code to bootstrap.js as proposed in the SUMO contributors forum
https://support.mozilla.org/forums/contributors/708557#post-48144

I have added a try and catch with an alert just in case because they have changed getCodebasePrincipal to getNoAppCodebasePrincipal in Firefox 17

cornelisvl added some commits Aug 15, 2012

@cornelisvl cornelisvl reset the search engine on the about:home page 178ecaf
@cornelisvl cornelisvl fixed error alert parameter, locked prefs test
Notice that I had the wrong order for the alert parameters.
Took the opportunity to do some testing with prefs locked via mozilla.cfg that prevented finishing properly.
Added some DEBUG code for testing.
I think that it is working now.
55f17f3

@gavinsharp gavinsharp commented on the diff Aug 15, 2012

bootstrap.js
+ if (originalDefaultEngine) {
@gavinsharp

gavinsharp Aug 15, 2012

Owner

You shouldn't need to add this check - after having reset the defaultenginename pref, originalDefaultEngine is garanteed to be present and represent the the build's default engine, unless its default value is corrupt due to e.g. an addon, but I don't think we should try to handle that case.

@gavinsharp gavinsharp commented on an outdated diff Aug 15, 2012

@@ -11,6 +12,10 @@ function startup(aData, aReason) {
// Helper function that backs up and then clears a pref, if it has a user-set
// value.
function resetPref(prefName) {
+ if (Services.prefs.prefIsLocked(prefName)){
@gavinsharp

gavinsharp Aug 15, 2012

Owner

Did you see this occurring in practice? I don't quite recall how this would behave if the prefs are locked, but I don't think we need to handle that case, since that would only occur in heavily customized Firefox builds.

@gavinsharp gavinsharp commented on an outdated diff Aug 15, 2012

// Flush changes to disk
Services.prefs.savePrefFile(null);
+ // reset the search engine used on the about:home page
+ // code reused from AboutHomeUtils
+ // http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#838
+ let defaultEngine = Services.search.originalDefaultEngine;
@gavinsharp

gavinsharp Aug 15, 2012

Owner

You can just re-use originalDefaultEngine from above.

@gavinsharp gavinsharp and 1 other commented on an outdated diff Aug 15, 2012

@@ -3,6 +3,7 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */
var Cu = Components.utils;
+var DEBUG = 0;
@gavinsharp

gavinsharp Aug 15, 2012

Owner

I think we should avoid adding this - it's easy enough to re-add during development, don't need to have it checked in.

@cornelisvl

cornelisvl Aug 16, 2012

I just added some code for testing and left it in case you want to verify.

So leaving out the var DEBUG = 0; is OK and leaving the alerts in is
also OK for you?

I did some testing and locked the affected prefs to check its affect.

If I lock the browser.search.defaultenginename pref then
originalDefaultEngine (and defaultEngine) seems to be null or
undefined and the script aborts leaving the extension installed
(about:addons).
At least now the script finishes properly in the cases that I've
tested and it is only two extra lines.

lockPref("browser.search.defaultenginename", "Wikipedia (en)");

Locking prefs doesn't effect the reset operation (prefHasUserValue
returns false), but of course the reset to the unlocked default will
fail.
I know that this is a very rare case and might never expose, but I
consider code that work in as much cases as possible important.

"You can just re-use originalDefaultEngine from above."
I know . I left this because that make it easier to maintain
(compare) the code should any change be necessary and it makes that
code independent from the rest of the code in the extension.

If you still want to to make your proposed changes in addition to
removing DEBUG=0 then I will do that.

Here is some code for the web console or scratchpad that you can use
to test the reset of the about:home search engine.
https://support.mozilla.org//en-US/questions/933343

Should it be added to the README file that a reload of the about:home
page is required if that page is currently open?

Maybe also mention the prefix name (searchreset.backup.*) of the
backup prefs explicitly to make it easier to spot those backup prefs.

On 8/15/12, Gavin Sharp notifications@github.com wrote:

@@ -3,6 +3,7 @@

I think we should avoid adding this - it's easy enough to re-add during
development, don't need to have it checked in.


Reply to this email directly or view it on GitHub:
https://github.com/gavinsharp/SearchReset/pull/2/files#r1384719

@cornelisvl

cornelisvl Aug 18, 2012

I've edited the previous patch with some of the proposed changes and
some others.
Placed both under one if (originalDefaultEngine) {} as I still prefer
to keep it.

@cornelisvl cornelisvl New proposal that addresses most changes.
New proposal that addresses most changes.
Kept "if (originalDefaultEngine) {}" and moved the about:home code under it.
Improved the DEBUG info under resetPref().
browser.startup.homepage and browser.search.defaultenginename are complex values and thus are reset to chrome://browser-region/locale/region.properties
7812ed7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment