847181 - Prune unused printing-related nsresult codes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

There are a whole bunch of no-longer-used printing error codes, and there are a couple of error codes that exist solely to make a distinction between "during print" and "during print preview", which is already information available at the point where we present the error to the user.  This patch removes the unused error codes, exposes the remaining codes in Components.results (nothing uses this now but bug 650960 will want it), and improves the language used for these error messages as long as I'm messing with things.

I'm splitting this off of bug 650960 primarily because it touches ErrorList.h and therefore I want to get it landed ASAP to minimize the number of world rebuilds I have to sit through.

Feel free to reassign review to someone more appropriate.

Comment 1

[Zack Weinberg (:zwol)]

https://tbpl.mozilla.org/?tree=Try&rev=911aa01bf21b
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1cb0a12d5146

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 720427 [details] [diff] [review]
patch

>@@ -631,17 +631,18 @@ nsPrintEngine::DoCommonPrint(bool       
>           }
>         } else if (rv == NS_ERROR_NOT_IMPLEMENTED) {
>           // This means the Dialog service was there,
>           // but they choose not to implement this dialog and
>           // are looking for default behavior from the toolkit
>           rv = NS_OK;
>         }
>       } else {
>-        rv = NS_ERROR_GFX_NO_PRINTROMPTSERVICE;
>+        // No dialog service available, carry on silently
>+        rv = NS_OK;
Please, no functionality changes in this kind of patch which is otherwise just clean up.
So, keep NS_ERROR_GFX_NO_PRINTROMPTSERVICE


>-#define NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(nserr) case nserr: stringName.AssignLiteral(#nserr); break;
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_CMD_NOT_FOUND)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_CMD_FAILURE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_NO_PRINTER_AVAILABLE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_NAME_NOT_FOUND)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_ACCESS_DENIED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_INVALID_ATTRIBUTE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_PRINTER_NOT_READY)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_OUT_OF_PAPER)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_PRINTER_IO_ERROR)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_COULD_NOT_OPEN_FILE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_FILE_IO_ERROR)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_PRINTPREVIEW)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_UNEXPECTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_OUT_OF_MEMORY)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_NOT_IMPLEMENTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_NOT_AVAILABLE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_ABORT)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_STARTDOC)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_ENDDOC)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_STARTPAGE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_ENDPAGE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_PRINT_WHILE_PREVIEW)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_PAPER_SIZE_NOT_SUPPORTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_ORIENTATION_NOT_SUPPORTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_COLORSPACE_NOT_SUPPORTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_TOO_MANY_COPIES)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_DRIVER_CONFIGURATION_ERROR)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_DOC_IS_BUSY_PP)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_DOC_WAS_DESTORYED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_NO_PRINTDIALOG_IN_TOOLKIT)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_NO_PRINTROMPTSERVICE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_NO_XUL)   // Temporary code for Bug 136185 / bug 240490
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_PLEX_NOT_SUPPORTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_DOC_IS_BUSY)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTING_NOT_IMPLEMENTED)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_COULD_NOT_LOAD_PRINT_MODULE)
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_RESOLUTION_NOT_SUPPORTED)
>-
>+#define NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(nserr) case nserr: stringName.AssignLiteral(#nserr); break
>     default:
>-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_FAILURE)
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_FAILURE);
>+
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_ABORT);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_NOT_AVAILABLE);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_NOT_IMPLEMENTED);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_OUT_OF_MEMORY);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_UNEXPECTED);
>+
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_NO_PRINTER_AVAILABLE);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_NAME_NOT_FOUND);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_COULD_NOT_OPEN_FILE);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_STARTDOC);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_ENDDOC);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_STARTPAGE);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_DOC_IS_BUSY);
>+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_GFX_PRINTER_NO_XUL);  // bug 136185 / bug 240490
I don't understand this all. Why you move default: to such odd place?

Comment 3

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #2)

> >       } else {
> >-        rv = NS_ERROR_GFX_NO_PRINTROMPTSERVICE;
> >+        // No dialog service available, carry on silently
> >+        rv = NS_OK;
> Please, no functionality changes in this kind of patch which is otherwise
> just clean up.
> So, keep NS_ERROR_GFX_NO_PRINTROMPTSERVICE

This is the only place in the entire tree where NS_ERROR_GFX_NO_PRINTROMPTSERVICE is used (not just mentioned).  I want to avoid changing ErrorList.h more than once, so I want to remove this use so that the error code can also be removed.

In context failing here is clearly a mistake (it's inconsistent with logic immediately above and below), but for the time being I'd be willing to change it to NS_ERROR_NOT_IMPLEMENTED instead.  This is not the only error in this chunk of code, and it's getting rototilled in bug 629500 anyway.

> >     default:
> >-      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_FAILURE)
> >+      NS_ERROR_TO_LOCALIZED_PRINT_ERROR_MSG(NS_ERROR_FAILURE);
...
> I don't understand this all. Why you move default: to such odd place?

I frequently find that it makes more sense in context to place the default: label at the top of a switch.  In this case, the desired default behavior is to use the error message associated with NS_ERROR_FAILURE.  It makes more sense in context to have NS_ERROR_FAILURE at the top of the list along with the other non-NS_ERROR_GFX_* codes.  Therefore, the default label moves too.

This is also code that I plan to remove Real Soon (in bug 650960) so I don't mind putting it back at the bottom if you insist.

Please advise how I should proceed.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

NS_ERROR_NOT_IMPLEMENTED sounds ok.

But don't understand your explanation why default: is moved to top.
That is not common coding style in Gecko.

Comment 5

[Zack Weinberg (:zwol)]

I always prioritize local contextual readability over general coding-style guidelines.  This particular function is more comprehensible with the default: at the top of the switch, so that is where it goes, and I don't care whether or not it's the "common coding style".

Anyway, new patch will turn up in a day or so.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: patch v2

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 722253 [details] [diff] [review]
patch v2

Why do we need the stuff on xpc.msg ?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Oh, Components.results

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 722253 [details] [diff] [review]
patch v2


>+#define GET_LOCALIZED_STRING(name_, msg_) \
>+  (nsContentUtils::GetLocalizedString(nsContentUtils::ePRINTING_PROPERTIES, \
>+                                      (name_), (msg_)))
Could you just call the method, and not use a macro.



>+  if (!aIsPrinting) {
>+    // Try first with _PP suffix.
>+    stringName.AppendLiteral("_PP");
>+    rv = GET_LOCALIZED_STRING(stringName.get(), msg);
>+    if (NS_FAILED(rv)) {
>+      stringName.Truncate(stringName.Length() - 3);
>+      rv = GET_LOCALIZED_STRING(stringName.get(), msg);
>+    }
>+  } else {
>+    rv = GET_LOCALIZED_STRING(stringName.get(), msg);
>+  }
Perhaps
nsresult rv = NS_OK;
if (!aIsPrinting) {
  // Try first with _PP suffix.
  stringName.AppendLiteral("_PP");
  rv = nsContentUtils::GetLocalizedString(nsContentUtils::ePRINTING_PROPERTIES,
                                          stringName + NS_LITERAL_STRING("_PP"),
                                          msg);
}
if (aIsPrinting || NS_FAILED(rv))
  rv = nsContentUtils::GetLocalizedString(nsContentUtils::ePRINTING_PROPERTIES,
                                          stringName, msg);
}

Comment 10

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #9)
> Perhaps
> nsresult rv = NS_OK;
> if (!aIsPrinting) {
>   // Try first with _PP suffix.
>   stringName.AppendLiteral("_PP");
>   rv =
> nsContentUtils::GetLocalizedString(nsContentUtils::ePRINTING_PROPERTIES,
>                                           stringName +
> NS_LITERAL_STRING("_PP"),
>                                           msg);
> }
> if (aIsPrinting || NS_FAILED(rv))
>   rv =
> nsContentUtils::GetLocalizedString(nsContentUtils::ePRINTING_PROPERTIES,
>                                           stringName, msg);
> }

So, the way I had it in the first patch, then? :-)  I thought the logic of the if-sequence was a little confusing that way, but it does mean we don't have to say nsContentUtils::GetLocalizedString(...) quite so many times.  Anyhow, fine by me.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: patch as landed

https://hg.mozilla.org/integration/mozilla-inbound/rev/071aab148869

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/071aab148869

Comment 13

[Rimas Kudelis [use needinfo to ping]]

Hi,
just wanted to point out: you've changed localizable strings without changing their entities. Many locales won't notice that change and catch up with it.

Comment 14

[Zack Weinberg (:zwol)]

I'm aware.  See bug 849633.

Comment 15

[Rimas Kudelis [use needinfo to ping]]

Oh, I didn't notice. Sorry for the noise.