merge nsIScriptError and nsIScriptError2 interfaces

VERIFIED FIXED in mozilla12

Status

defect
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({addon-compat, dev-doc-complete})

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Spinoff from bug 122213 comment 64.

The two interfaces nsIScriptError and nsIScriptError2 could probably be merged into one in js/xpconnect/idl/nsIScriptError.idl . Currently, nsIScriptError2 is an extension of nsIScriptError (inherits from it?).
I have found these occurrences of nsIScriptError2 in /mozilla:

user:mozilla$ grep -r -n nsIScriptError2 *
browser/devtools/scratchpad/scratchpad.js:339:                        createInstance(Ci.nsIScriptError2);
browser/devtools/webconsole/test/browser_webconsole_bug_603750_websocket.js:25:        !(aSubject instanceof Ci.nsIScriptError2)) {
browser/devtools/webconsole/HUDService.jsm:6526:        !(aSubject instanceof Ci.nsIScriptError2) ||
content/base/src/nsEventSource.cpp:1094:  nsCOMPtr<nsIScriptError2> errObj(
content/base/src/nsWebSocket.cpp:130:  nsCOMPtr<nsIScriptError2> errorObject(
content/base/src/nsContentUtils.cpp:2833:  nsCOMPtr<nsIScriptError2> errorObject =
content/xml/document/src/nsXMLDocument.cpp:404:      nsCOMPtr<nsIScriptError2> errorObject =
dom/base/nsJSEnvironment.cpp:383:        nsCOMPtr<nsIScriptError2> error2(do_QueryInterface(errorObject));
dom/base/nsDOMClassInfo.cpp:1730:  nsCOMPtr<nsIScriptError2> scriptError =
dom/workers/WorkerPrivate.cpp:1112:    nsCOMPtr<nsIScriptError2> scriptError =
image/src/Decoder.cpp:133:    nsCOMPtr<nsIScriptError2> errorObject =
js/xpconnect/src/xpcprivate.h:3945:                      public nsIScriptError2 {
js/xpconnect/src/nsScriptError.cpp:50:                              nsIScriptError2)
js/xpconnect/src/XPCComponents.cpp:2749:    nsCOMPtr<nsIScriptError2> scripterr(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
js/xpconnect/src/XPCWrappedJSClass.cpp:1136:                                nsCOMPtr<nsIScriptError2> scriptError2 =
js/xpconnect/idl/nsIScriptError.idl:100:interface nsIScriptError2 : nsISupports {
layout/style/nsCSSScanner.cpp:427:    nsCOMPtr<nsIScriptError2> errorObject =
layout/style/nsFontFaceLoader.cpp:800:  nsCOMPtr<nsIScriptError2> scriptError =
parser/htmlparser/src/nsExpatDriver.cpp:950:    nsCOMPtr<nsIScriptError2> serr2(do_QueryInterface(serr));

There were none in /mailnews, /mail, /suite .

Is it enough to merge the interfaces into nsIScriptError (containing all the properties) and then just change all these places to use nsIScriptError ?
Most of these locations need both nsIScriptError and nsIScriptError2 so in that case you would simply reuse the existing object, but otherwise yes.

I did wonder whether it's worth putting timeStamp back after innerWindowID, but it's probably not.
I see a pattern like this:

  nsCOMPtr<nsIScriptError2> errorObject =
      do_CreateInstance(NS_SCRIPTERROR_CONTRACTID, &rv);

  errorObject->InitWithWindowID(...);

  nsCOMPtr<nsIScriptError> logError = do_QueryInterface(errorObject);
  return sConsoleService->LogMessage(logError);

Can I change it to this?

  nsCOMPtr<nsIScriptError> errorObject =
      do_CreateInstance(NS_SCRIPTERROR_CONTRACTID, &rv);

  errorObject->InitWithWindowID(...);

  return sConsoleService->LogMessage(errorObject);

Is this the reuse you mean?
Thanks, so I can try it.

Does this have any performance considerations?
In the pattern below, it seems it could save one pointer (or a full object?) and do_QueryInterface call.
On the other hand, places that were sufficient with nsIScriptError interface will now get the full merged interface (with the 2 variables added). But from my understanding this is no real change, as in the background they were already getting the full nsIScriptError object, just didn't have the interface to access those 2 variables (3 before bug 122213).

So should this be a theoretical performance win? Did I miss anything? Or do we not care as there are not many of these objects created (not many messages).
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Yes, it is technically both a performance and size win (the nsScriptError object has one fewer vtable pointer).
This is my attempt at it. It does compile and work in Thunderbird fine. The error console is populated with some messages (that appear at each start).
Attachment #582917 - Flags: review?(neil)
Comment on attachment 582917 [details] [diff] [review]
patch, merges interfaces and updates callers

>     // Otherwise log an error to the error console.
>-    nsCOMPtr<nsIScriptError2> scriptError =
>+    nsCOMPtr<nsIScriptError> scriptError =
>       do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
>     NS_WARN_IF_FALSE(scriptError, "Faild to create script error!");
> 
>     nsCOMPtr<nsIConsoleMessage> consoleMessage;
This one confused you by using nsIConsoleMessage instead of nsIScriptError, but they can in fact be the same variable too.
Comment on attachment 582917 [details] [diff] [review]
patch, merges interfaces and updates callers

r=me on the rest of the patch.
Attachment #582917 - Flags: review?(neil) → review+
Thanks for your hint, I updated that. Fixed some typos while there.
Attachment #582917 - Attachment is obsolete: true
Attachment #583226 - Flags: superreview?(bzbarsky)
Attachment #583226 - Flags: review?(neil)
Comment on attachment 583226 [details] [diff] [review]
v2 patch, merges interfaces and updates callers

>diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
> ...
>     if (scriptError) {
>       if (NS_SUCCEEDED(scriptError->InitWithWindowID(aMessage.get(),
>                                                      aFilename.get(),
>                                                      aLine.get(), aLineNumber,
>                                                      aColumnNumber, aFlags,
>                                                      "Web Worker",
>                                                      aInnerWindowId))) {
>-        consoleMessage = do_QueryInterface(scriptError);
>-        NS_ASSERTION(consoleMessage, "This should never fail!");
>       }
>       else {
>         NS_WARNING("Failed to init script error!");
>       }
>     }

Drive-by, please don't leave an empty if clause. Switch the condition to NS_FAILED :)
One other thing I spotted was that the new code tries to log the script error even if it failed to init it; the old one didn't because of the extra call to QueryInterface that only happened if the init succeeded. (Most users of LogMessage just bail out early if init fails, so it's not normally a problem.)
In that same spot? Or should I check all files again?
In that one spot, can I just set scriptError to nsnull so that the following tests ( if (scriptError) {} ) catch it?
It seems I have found about 4 other places in the patch where InitWithWindowID was not checked even before my changes. I'll fix that too.
Posted patch v3, error checking fixes (obsolete) — Splinter Review
So I added the error checking of InitWithWindowID in the places I thought were missing it. I tried to guess from the surrounding code whether each individual function can return early with the error, or it should continue just without sending the error to the console.

I found another spot in js/xpconnect/src/XPCConvert.cpp (where I didn't touch anything previously) what is not checking InitWithWindowID. Maybe that should be fixed up, but I don't know if it can just return in case of error (NS_ENSURE_SUCCESS (rv,rv)).
Attachment #583226 - Attachment is obsolete: true
Attachment #583226 - Flags: superreview?(bzbarsky)
Attachment #583226 - Flags: review?(neil)
Attachment #583320 - Flags: review?(neil)
Comment on attachment 583320 [details] [diff] [review]
v3, error checking fixes

>+      if (scriptError != nsnull) {
Doesn't need to be explictly checked against nsnull: if (scriptError) is fine.
Attachment #583320 - Flags: review?(neil) → review+
Posted patch v4, fixes nsnull check (obsolete) — Splinter Review
Carrying over r=neil.
Attachment #583320 - Attachment is obsolete: true
Attachment #583539 - Flags: superreview?(bzbarsky)
Attachment #583539 - Flags: review+
Comment on attachment 583539 [details] [diff] [review]
v4, fixes nsnull check

Please leave the curlies in the style system code and fix the odd "line up the '(' with the first letter of the function name" indent in the one place you use it, and sr=me.
Attachment #583539 - Flags: superreview?(bzbarsky) → superreview+
(In reply to Boris Zbarsky (:bz) from comment #19)
> fix the odd "line up
> the '(' with the first letter of the function name" indent in the one place
> you use it, and sr=me.

Not sure what you mean with this. What line number in the patch?
Starting line 427 in the attached diff.
There is not much space left if we want to wrap at 80 columns. What about this:
+      if (NS_SUCCEEDED(errorObject->InitWithWindowID(msg.get(),
+                       NS_ConvertUTF8toUTF16(mImage.GetURIString()).get(),
+                       nsnull, 0, 0, nsIScriptError::errorFlag,
+                       "Image", mImage.InnerWindowID())
+                      )
+         ) {
How about this:

+      if (NS_SUCCEEDED(errorObject->InitWithWindowID(
+                         msg.get(),
+                         NS_ConvertUTF8toUTF16(mImage.GetURIString()).get(),
+                         nsnull, 0, 0, nsIScriptError::errorFlag,
+                         "Image", mImage.InnerWindowID()
+                       ))) {

?
Posted patch v5, style fixes (obsolete) — Splinter Review
As you wish.
Attachment #583539 - Attachment is obsolete: true
Attachment #583588 - Flags: superreview?(bzbarsky)
Attachment #583588 - Flags: review+
Posted patch v6, fixedSplinter Review
Yes, sorry, forgot to update.
Attachment #583588 - Attachment is obsolete: true
Attachment #583588 - Flags: superreview?(bzbarsky)
Attachment #583596 - Flags: superreview?(bzbarsky)
Comment on attachment 583596 [details] [diff] [review]
v6, fixed

sr=me
Attachment #583596 - Flags: superreview?(bzbarsky) → superreview+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/916876a3db97
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Keywords: addon-compat
Status: RESOLVED → VERIFIED
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.