The default bug view has changed. See this FAQ.

editor should use mozilla::Preferences

RESOLVED FIXED in mozilla7

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Attachment #539529 - Flags: review?(roc)
(Assignee)

Comment 1

6 years ago
Created attachment 539531 [details] [diff] [review]
Part.1 with -w
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!
Attachment #539529 - Flags: review?(roc)
(Assignee)

Comment 3

6 years ago
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.
Attachment #539529 - Attachment is obsolete: true
Attachment #539531 - Attachment is obsolete: true
Attachment #539744 - Flags: review?(ehsan)
Comment on attachment 539744 [details] [diff] [review]
Patch

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

Looks great, thanks!
Attachment #539744 - Flags: review?(ehsan) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f81579f78f92
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/f81579f78f92
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Depends on: 665359
You need to log in before you can comment on or make changes to this bug.