Note: There are a few cases of duplicates in user autocompletion which are being worked on.

uriloader should use mozilla::Preferences

RESOLVED FIXED in mozilla7

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 541657 [details] [diff] [review]
Patch v1.0

CCing OS/2 folks.

This patch includes OS/2 part. And looks like there're some bugs. It took a non-root pref branch. But passes the branch path to Get*Pref() again. I don't change them in v1.0 because some users may use the strange pref name for customizing.
Attachment #541657 - Flags: review?(bzbarsky)
Created attachment 541658 [details] [diff] [review]
Patch v1.0 with -w

Comment 2

6 years ago
Comment on attachment 541657 [details] [diff] [review]
Patch v1.0

>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
>-static const char NEVER_ASK_FOR_SAVE_TO_DISK_PREF[] = "saveToDisk";
>-static const char NEVER_ASK_FOR_OPEN_FILE_PREF[]    = "openFile";
>+static const char SAVE_TO_DISK_PREF[] = "browser.helperApps.neverAsk.saveToDisk";
>+static const char OPEN_FILE_PREF[]    = "browser.helperApps.neverAsk.openFile";

Please keep the old macro names.

>+++ b/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp
>+  // XXX Is this intentional?

I would think not.  File a bug, please.

>+++ b/uriloader/exthandler/os2/nsOSHelperAppService.cpp
>-    rv = prefBranch->GetComplexValue(aPrefName,
>-                                     NS_GET_IID(nsISupportsString),
>-                                     getter_AddRefs(prefFileName));
>+      NS_SUCCEEDED(Preferences::GetString(aPrefName, aFileLocation))) {

Is that equivalent?

>+  // XXX Is this intentionally?

Again, I would think not, in both cases.

>+++ b/uriloader/exthandler/unix/nsOSHelperAppService.cpp

The changes to this file don't make sense.

>+++ b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp
>+        static const char kPrefName[] = "offline-apps.allow_by_default";
>+        if (aPrefBranch) {
>+            aPrefBranch->GetBoolPref(kPrefName, aAllowed);

I believe this is not equivalent to the old code when the pref is not set.

In general, this aPrefBranch thing seems to be an optimization that is not needed anymore.  Just file a followup bug to rip it out?
Attachment #541657 - Flags: review?(bzbarsky) → review-
>>+++ b/uriloader/exthandler/os2/nsOSHelperAppService.cpp
>>-    rv = prefBranch->GetComplexValue(aPrefName,
>>-                                     NS_GET_IID(nsISupportsString),
>>-                                     getter_AddRefs(prefFileName));
>>+      NS_SUCCEEDED(Preferences::GetString(aPrefName, aFileLocation))) {
> 
> Is that equivalent?

Yes. nsISupportsString returns UTF16 string which is converted from char pref value which was assumed as UTF8.

>>+++ b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp
>>+        static const char kPrefName[] = "offline-apps.allow_by_default";
>>+        if (aPrefBranch) {
>>+            aPrefBranch->GetBoolPref(kPrefName, aAllowed);
> 
> I believe this is not equivalent to the old code when the pref is not set.

|*aAllowed = PR_FALSE;| is done in top of the method. So, If GetBoolPref() is failed, *aAllowed isn't modified, i.e., it keeps PR_FALSE. The old code is redundant.

Comment 4

6 years ago
OK, makes sense.  What about uriloader/exthandler/unix/nsOSHelperAppService.cpp ?
(In reply to comment #4)
> OK, makes sense.  What about
> uriloader/exthandler/unix/nsOSHelperAppService.cpp ?

I forgot to change in the file. I'm testing it now.
http://tbpl.mozilla.org/?tree=Try&pusher=masayuki@d-toybox.com&rev=8b1741fe90b8
Created attachment 542007 [details] [diff] [review]
Patch v2.0
Attachment #541657 - Attachment is obsolete: true
Attachment #541658 - Attachment is obsolete: true
Attachment #542007 - Flags: review?(bzbarsky)
Created attachment 542009 [details] [diff] [review]
Patch v2.0.1

removing unnecessary change of v2.0. sorry for the spam.
Attachment #542007 - Attachment is obsolete: true
Attachment #542009 - Flags: review?(bzbarsky)
Attachment #542007 - Flags: review?(bzbarsky)
Comment on attachment 542009 [details] [diff] [review]
Patch v2.0.1

>--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
> NS_IMETHODIMP nsExternalHelperAppService::IsExposedProtocol(const char * aProtocolScheme, PRBool * aResult)
> {
>+  // check the per protocol setting first.  it always takes precidence.

While you're here, could you do s/precidence/precedence/?
> could you do s/precidence/precedence/?

I'll do it after bz's review.

Comment 10

6 years ago
Comment on attachment 542009 [details] [diff] [review]
Patch v2.0.1

r=me, but please do file those OS/2 bugs.
Attachment #542009 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/d970a2b0f2c2
Whiteboard: [inbound]
(In reply to comment #10)
> r=me, but please do file those OS/2 bugs.

filed bug 668725.
temporary backed out from inbound due to strange orange.
Whiteboard: [inbound]
http://hg.mozilla.org/integration/mozilla-inbound/rev/7308d42e070d

relanded.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/7308d42e070d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Updated

6 years ago
Blocks: 670004

Updated

6 years ago
No longer blocks: 670004
Depends on: 670004
Whiteboard: [inbound]

Updated

6 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.