Last Comment Bug 666903 - uriloader should use mozilla::Preferences
: uriloader should use mozilla::Preferences
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
Depends on: 656826 670004
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-24 04:19 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
Modified: 2011-10-20 22:26 PDT (History)
5 users (show)
jruderman: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (43.98 KB, patch)
2011-06-24 04:19 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
bzbarsky: review-
Details | Diff | Review
Patch v1.0 with -w (42.61 KB, patch)
2011-06-24 04:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v2.0 (53.12 KB, patch)
2011-06-26 03:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v2.0.1 (51.99 KB, patch)
2011-06-26 03:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
bzbarsky: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-24 04:19:38 PDT
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.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-24 04:20:52 PDT
Created attachment 541658 [details] [diff] [review]
Patch v1.0 with -w
Comment 2 Boris Zbarsky [:bz] 2011-06-24 12:06:45 PDT
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?
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-24 19:26:34 PDT
>>+++ 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 Boris Zbarsky [:bz] 2011-06-24 19:38:05 PDT
OK, makes sense.  What about uriloader/exthandler/unix/nsOSHelperAppService.cpp ?
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-24 20:04:30 PDT
(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
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-26 03:20:47 PDT
Created attachment 542007 [details] [diff] [review]
Patch v2.0
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-26 03:29:52 PDT
Created attachment 542009 [details] [diff] [review]
Patch v2.0.1

removing unnecessary change of v2.0. sorry for the spam.
Comment 8 :Ms2ger 2011-06-26 03:52:44 PDT
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/?
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-28 00:10:03 PDT
> could you do s/precidence/precedence/?

I'll do it after bz's review.
Comment 10 Boris Zbarsky [:bz] 2011-06-30 15:24:17 PDT
Comment on attachment 542009 [details] [diff] [review]
Patch v2.0.1

r=me, but please do file those OS/2 bugs.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-30 23:28:12 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d970a2b0f2c2
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-30 23:37:30 PDT
(In reply to comment #10)
> r=me, but please do file those OS/2 bugs.

filed bug 668725.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-07-01 00:42:20 PDT
temporary backed out from inbound due to strange orange.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-07-01 01:37:22 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7308d42e070d

relanded.
Comment 15 Marco Bonardo [::mak] 2011-07-02 02:37:33 PDT
http://hg.mozilla.org/mozilla-central/rev/7308d42e070d

Note You need to log in before you can comment on or make changes to this bug.