Last Comment Bug 661101 - widget should use mozilla::Preferences (except win/cocoa/gtk2/xpwidgets)
: widget should use mozilla::Preferences (except win/cocoa/gtk2/xpwidgets)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 656826
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-01 01:44 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-08-31 05:48 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (15.77 KB, patch)
2011-06-01 01:44 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v1.1 (15.74 KB, patch)
2011-06-01 01:50 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Fix the nit in nsDeviceContextSpecOS2.cpp (706 bytes, patch)
2011-06-07 02:25 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-01 01:44:06 PDT
Created attachment 536541 [details] [diff] [review]
Patch v1.0

I'd like OS/2 guys to check this patch whether not causes bustage.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-01 01:50:17 PDT
Created attachment 536542 [details] [diff] [review]
Patch v1.1

Fix a nit for OS/2. We shouldn't use %s for nsPrintfCString.
Comment 2 Peter Weilbacher 2011-06-01 03:50:31 PDT
Comment on attachment 536542 [details] [diff] [review]
Patch v1.1

Since I no longer build on OS/2 I'd like to defer my feedback to Dave.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-06 19:49:30 PDT
Comment on attachment 536542 [details] [diff] [review]
Patch v1.1

If we didn't get feedback from OS/2 developers, I would drop the OS/2 change and land the other parts.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-06 20:46:21 PDT
Comment on attachment 536542 [details] [diff] [review]
Patch v1.1

Review of attachment 536542 [details] [diff] [review]:
-----------------------------------------------------------------

Just land it.
Comment 5 Rich Walsh 2011-06-06 21:07:11 PDT
(In reply to comment #0)
> I'd like OS/2 guys to check this patch whether not causes bustage.
(In reply to comment #3)
> If we didn't get feedback from OS/2 developers, I would drop the OS/2 change
> and land the other parts.

Sorry for the delay in responding.  Go ahead with the OS/2 changes - it looks a lot simpler that the existing code.  And, if it breaks something.. well, we're used to that :-)

BTW... I seem to remember that Prefs was not thread-safe and should only be accessed from the primary thread.  Is that still the case?
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-07 00:51:18 PDT
http://hg.mozilla.org/mozilla-central/rev/9014863a7e2a
http://hg.mozilla.org/mozilla-central/rev/66cbdac56469

# the second backed out a part of 9014863a7e2a because it included another bug's draft patch accidentally.
Comment 7 :Ms2ger 2011-06-07 01:56:27 PDT
Comment on attachment 536542 [details] [diff] [review]
Patch v1.1

>--- a/widget/src/os2/nsDeviceContextSpecOS2.cpp
>+++ b/widget/src/os2/nsDeviceContextSpecOS2.cpp
>@@ -657,9 +659,8 @@ nsresult GlobalPrinters::InitializeGloba
>   if (!mGlobalPrinterList) 
>      return NS_ERROR_OUT_OF_MEMORY;
> 
>-  nsresult rv;
>-  nsCOMPtr<nsIPrefBranch> pPrefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>-  BOOL prefFailed = NS_FAILED(rv); // don't return on failure, optional feature
>+  // don't return on failure, optional feature
>+  BOOL prefFailed = (Preferences::GetRootBranch() != nsnull);

!= nsnull or == nsnull?
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-07 02:13:24 PDT
(In reply to comment #7)
> Comment on attachment 536542 [details] [diff] [review] [review]
> Patch v1.1
> 
> >--- a/widget/src/os2/nsDeviceContextSpecOS2.cpp
> >+++ b/widget/src/os2/nsDeviceContextSpecOS2.cpp
> >@@ -657,9 +659,8 @@ nsresult GlobalPrinters::InitializeGloba
> >   if (!mGlobalPrinterList) 
> >      return NS_ERROR_OUT_OF_MEMORY;
> > 
> >-  nsresult rv;
> >-  nsCOMPtr<nsIPrefBranch> pPrefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
> >-  BOOL prefFailed = NS_FAILED(rv); // don't return on failure, optional feature
> >+  // don't return on failure, optional feature
> >+  BOOL prefFailed = (Preferences::GetRootBranch() != nsnull);
> 
> != nsnull or == nsnull?

Oh... I'll post a followup patch.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-07 02:25:55 PDT
Created attachment 537758 [details] [diff] [review]
Fix the nit in nsDeviceContextSpecOS2.cpp
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-07 02:34:57 PDT
Comment on attachment 537758 [details] [diff] [review]
Fix the nit in nsDeviceContextSpecOS2.cpp

Review of attachment 537758 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-07 02:45:45 PDT
http://hg.mozilla.org/mozilla-central/rev/7d641625692f
Comment 12 Vlad [QA] 2011-08-31 05:48:45 PDT
Hi.
Is there any way I can test this? Do you have some STR?
Thanks

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