Closed Bug 666903 Opened 10 years ago Closed 10 years ago

uriloader should use mozilla::Preferences

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
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)
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.
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
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #541657 - Attachment is obsolete: true
Attachment #541658 - Attachment is obsolete: true
Attachment #542007 - Flags: review?(bzbarsky)
Attached patch Patch v2.0.1Splinter Review
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 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+
(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/mozilla-central/rev/7308d42e070d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Blocks: 670004
No longer blocks: 670004
Depends on: 670004
Whiteboard: [inbound]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.