Closed Bug 847181 Opened 7 years ago Closed 7 years ago

Prune unused printing-related nsresult codes

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: zwol, Assigned: zwol)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #720427 - Flags: review?(bugs)
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?
Attachment #720427 - Flags: review?(bugs) → review-
(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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #720427 - Attachment is obsolete: true
Attachment #722253 - Flags: review?(bugs)
Comment on attachment 722253 [details] [diff] [review]
patch v2

Why do we need the stuff on xpc.msg ?
Oh, Components.results
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);
}
Attachment #722253 - Flags: review?(bugs) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/071aab148869
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 849633
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.
I'm aware.  See bug 849633.
Oh, I didn't notice. Sorry for the noise.
Depends on: 862470
You need to log in before you can comment on or make changes to this bug.