Last Comment Bug 664437 - editor should use mozilla::Preferences
: editor should use mozilla::Preferences
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (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 665359
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-15 08:16 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
Modified: 2011-06-19 11:15 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part.1 (29.42 KB, patch)
2011-06-15 08:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Part.1 with -w (26.17 KB, patch)
2011-06-15 08:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch (31.90 KB, patch)
2011-06-16 01:07 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
ehsan: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-15 08:16:06 PDT
Created attachment 539529 [details] [diff] [review]
Part.1

There are some strange code in editor. They use pref related result for its result. However, sometimes, they ignore the result (e.g., continue to do its job) but don't reset the variables. Therefore, this patch includes some strange logic.

I think that if Ehsan thinks I should fix them this bug, I'll add a patch.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-15 08:16:42 PDT
Created attachment 539531 [details] [diff] [review]
Part.1 with -w
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2011-06-15 11:57:57 PDT
Comment on attachment 539529 [details] [diff] [review]
Part.1

The patch looks really good.  I think we could be less conservative.  See below.

> // Get the default browser background color if we need it for GetCSSBackgroundColorState
> nsresult
> nsHTMLCSSUtils::GetDefaultBackgroundColor(nsAString & aColor)
> {
>-  nsresult result;
>-  nsCOMPtr<nsIPrefBranch> prefBranch =
>-    do_GetService(NS_PREFSERVICE_CONTRACTID, &result);
>-  NS_ENSURE_SUCCESS(result, result);
>+  NS_ENSURE_TRUE(Preferences::GetRootBranch(), NS_ERROR_FAILURE);

This is not desired.  If the pref service is not available (why would it not be?!) we'd just use white.

>   aColor.AssignLiteral("#ffffff");
>-  nsXPIDLCString returnColor;
>-  if (prefBranch) {
>-    PRBool useCustomColors;
>-    result = prefBranch->GetBoolPref("editor.use_custom_colors", &useCustomColors);
>-    NS_ENSURE_SUCCESS(result, result);
>-    if (useCustomColors) {
>-      result = prefBranch->GetCharPref("editor.background_color",
>-                                       getter_Copies(returnColor));
>-      NS_ENSURE_SUCCESS(result, result);
>-    }
>-    else {
>-      PRBool useSystemColors;
>-      result = prefBranch->GetBoolPref("browser.display.use_system_colors", &useSystemColors);
>-      NS_ENSURE_SUCCESS(result, result);
>-      if (!useSystemColors) {
>-        result = prefBranch->GetCharPref("browser.display.background_color",
>-                                         getter_Copies(returnColor));
>-        NS_ENSURE_SUCCESS(result, result);
>-      }
>-    }
>+
>+  PRBool useCustomColors;
>+  nsresult rv =
>+    Preferences::GetBool("editor.use_custom_colors", &useCustomColors);
>+  NS_ENSURE_SUCCESS(rv, rv);

No need to check the result of GetBool, just default to false.

>+  if (useCustomColors) {
>+    rv = Preferences::GetString("editor.background_color", &aColor);

Same here, default to white.

>+    NS_ENSURE_SUCCESS(rv, rv);
>+    return rv;
>   }
>-  if (returnColor) {
>-    CopyASCIItoUTF16(returnColor, aColor);
>+
>+  PRBool useSystemColors;
>+  rv = Preferences::GetBool("browser.display.use_system_colors",
>+                            &useSystemColors);

Ditto, default to false.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (useSystemColors) {
>+    return NS_OK;
>   }
>-  return NS_OK;
>+
>+  rv = Preferences::GetString("browser.display.background_color", &aColor);

Same here.

>-  nsresult result;
>-  nsCOMPtr<nsIPrefBranch> prefBranch =
>-    do_GetService(NS_PREFSERVICE_CONTRACTID, &result);
>-  NS_ENSURE_SUCCESS(result, result);
>+  NS_ENSURE_TRUE(Preferences::GetRootBranch(), NS_ERROR_FAILURE);
>   aLengthUnit.AssignLiteral("px");
>-  if (NS_SUCCEEDED(result) && prefBranch) {
>-    nsXPIDLCString returnLengthUnit;
>-    result = prefBranch->GetCharPref("editor.css.default_length_unit",
>-                                     getter_Copies(returnLengthUnit));
>-    NS_ENSURE_SUCCESS(result, result);
>-    if (returnLengthUnit) {
>-      CopyASCIItoUTF16(returnLengthUnit, aLengthUnit);
>-    }
>-  }
>+  nsresult rv =
>+    Preferences::GetString("editor.css.default_length_unit", &aLengthUnit);
>+  NS_ENSURE_SUCCESS(rv, rv); // If failed, aLengthUnit must not be modified.

Here, too, we should default to "px".

>-  nsCOMPtr<nsIPrefBranch> prefBranch =
>-    do_GetService(NS_PREFSERVICE_CONTRACTID, &res);
>-  NS_ENSURE_SUCCESS(res, res);
>-
>-  char *returnInEmptyLIKillsList = 0;
>-  res = prefBranch->GetCharPref("editor.html.typing.returnInEmptyListItemClosesList",
>-                                &returnInEmptyLIKillsList);
>-
>-  if (NS_SUCCEEDED(res) && returnInEmptyLIKillsList)
>-  {
>-    if (!strncmp(returnInEmptyLIKillsList, "false", 5))
>+  NS_ENSURE_TRUE(Preferences::GetRootBranch(), NS_ERROR_FAILURE);
>+
>+  static const char kPrefName[] =
>+    "editor.html.typing.returnInEmptyListItemClosesList";
>+  nsAdoptingCString returnInEmptyLIKillsList =
>+    Preferences::GetCString(kPrefName);

Same here, default to false.  (Why was this pref designed as a string and not bool? :( )

>-  nsresult result;
>-  nsCOMPtr<nsIPrefBranch> prefBranch =
>-    do_GetService(NS_PREFSERVICE_CONTRACTID, &result);
>-  if (NS_SUCCEEDED(result) && prefBranch && preserveRatio) {
>-    result = prefBranch->GetBoolPref("editor.resizing.preserve_ratio", &preserveRatio);
>+  nsresult result =
>+    (Preferences::GetRootBranch() != nsnull) ? NS_OK : NS_ERROR_FAILURE;
>+  if (preserveRatio) {
>+    // XXX this result would be used for this method's result if
>+    //     mMouseMotionListenerP wasn't null.  It's odd...
>+    result = Preferences::GetBool("editor.resizing.preserve_ratio",
>+                                  &preserveRatio);

Here, too, we should default to true.

>-  PRBool deleteBidiImmediately = PR_FALSE;
>-  nsCOMPtr<nsIPrefBranch> prefBranch =
>-    do_GetService(NS_PREFSERVICE_CONTRACTID, &res);
>-  if (NS_SUCCEEDED(res))
>-    prefBranch->GetBoolPref("bidi.edit.delete_immediately",
>-                            &deleteBidiImmediately);
>-  mDeleteBidiImmediately = deleteBidiImmediately;
>+  // XXX Is this intentional?
>+  if (!Preferences::GetRootBranch()) {
>+    res = NS_ERROR_FAILURE;
>+  }
>+
>+  mDeleteBidiImmediately =
>+    Preferences::GetBool("bidi.edit.delete_immediately", PR_FALSE);

No need to check for root branch here either!
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-16 01:07:08 PDT
Created attachment 539744 [details] [diff] [review]
Patch

Okay.

I think we should add nscolor Preferences::GetColor() which can be shared between editor and xpwidgets at least. But we need to change the caller and its callers in editor for that. So, it should be another bug.
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-06-16 13:09:15 PDT
Comment on attachment 539744 [details] [diff] [review]
Patch

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

Looks great, thanks!
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-16 18:03:05 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f81579f78f92
Comment 6 Philipp von Weitershausen [:philikon] 2011-06-17 07:40:05 PDT
http://hg.mozilla.org/mozilla-central/rev/f81579f78f92

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