661101 - widget should use mozilla::Preferences (except win/cocoa/gtk2/xpwidgets)

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

I'd like OS/2 guys to check this patch whether not causes bustage.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Fix a nit for OS/2. We shouldn't use %s for nsPrintfCString.

Comment 2

[Peter Weilbacher]

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] (he/him)(JST, +0900)]

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) (email my personal email if necessary)]

Comment on attachment 536542 [details] [diff] [review]
Patch v1.1

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

Just land it.

Comment 5

[Rich Walsh]

(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] (he/him)(JST, +0900)]

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 (he/him; ⌚ UTC+1/+2)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: Fix the nit in nsDeviceContextSpecOS2.cpp

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/7d641625692f

Comment 12

[Vlad [QA]]

Hi.
Is there any way I can test this? Do you have some STR?
Thanks