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 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!

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 on attachment 539744 [details] [diff] [review] Patch Review of attachment 539744 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks!