Last Comment Bug 711721 - merge nsIScriptError and nsIScriptError2 interfaces
: merge nsIScriptError and nsIScriptError2 interfaces
: addon-compat, dev-doc-complete
Product: Toolkit Graveyard
Classification: Graveyard
Component: Error Console (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :aceman
Depends on: 122213
Blocks: 611032
  Show dependency treegraph
Reported: 2011-12-17 07:32 PST by :aceman
Modified: 2016-06-29 11:02 PDT (History)
8 users (show)
bzbarsky: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

patch, merges interfaces and updates callers (26.87 KB, patch)
2011-12-19 12:25 PST, :aceman
neil: review+
Details | Diff | Splinter Review
v2 patch, merges interfaces and updates callers (28.16 KB, patch)
2011-12-20 11:16 PST, :aceman
no flags Details | Diff | Splinter Review
v3, error checking fixes (30.98 KB, patch)
2011-12-20 15:33 PST, :aceman
neil: review+
Details | Diff | Splinter Review
v4, fixes nsnull check (30.97 KB, patch)
2011-12-21 09:59 PST, :aceman
acelists: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
v5, style fixes (30.96 KB, patch)
2011-12-21 12:38 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
v6, fixed (30.96 KB, patch)
2011-12-21 12:54 PST, :aceman
bzbarsky: superreview+
Details | Diff | Splinter Review

Description :aceman 2011-12-17 07:32:59 PST
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?).
Comment 1 :aceman 2011-12-17 09:06:40 PST
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 ?
Comment 2 2011-12-17 10:22:27 PST
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.
Comment 3 :aceman 2011-12-17 11:30:36 PST
I see a pattern like this:

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


  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);


  return sConsoleService->LogMessage(errorObject);

Is this the reuse you mean?
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-12-17 11:54:54 PST
Comment 5 :aceman 2011-12-19 01:58:57 PST
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).
Comment 6 2011-12-19 02:03:03 PST
Yes, it is technically both a performance and size win (the nsScriptError object has one fewer vtable pointer).
Comment 7 :aceman 2011-12-19 12:25:04 PST
Created attachment 582917 [details] [diff] [review]
patch, merges interfaces and updates callers

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).
Comment 8 2011-12-19 17:01:00 PST
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 9 2011-12-20 08:27:04 PST
Comment on attachment 582917 [details] [diff] [review]
patch, merges interfaces and updates callers

r=me on the rest of the patch.
Comment 10 :aceman 2011-12-20 11:16:22 PST
Created attachment 583226 [details] [diff] [review]
v2 patch, merges interfaces and updates callers

Thanks for your hint, I updated that. Fixed some typos while there.
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-20 12:01:24 PST
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 :)
Comment 12 2011-12-20 13:56:28 PST
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.)
Comment 13 :aceman 2011-12-20 14:07:59 PST
In that same spot? Or should I check all files again?
Comment 14 :aceman 2011-12-20 14:27:29 PST
In that one spot, can I just set scriptError to nsnull so that the following tests ( if (scriptError) {} ) catch it?
Comment 15 :aceman 2011-12-20 14:34:52 PST
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.
Comment 16 :aceman 2011-12-20 15:33:00 PST
Created attachment 583320 [details] [diff] [review]
v3, error checking fixes

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)).
Comment 17 2011-12-20 16:59:51 PST
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.
Comment 18 :aceman 2011-12-21 09:59:07 PST
Created attachment 583539 [details] [diff] [review]
v4, fixes nsnull check

Carrying over r=neil.
Comment 19 Boris Zbarsky [:bz] 2011-12-21 11:31:46 PST
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.
Comment 20 :aceman 2011-12-21 11:45:08 PST
(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?
Comment 21 Boris Zbarsky [:bz] 2011-12-21 11:50:39 PST
Starting line 427 in the attached diff.
Comment 22 :aceman 2011-12-21 12:03:54 PST
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())
+                      )
+         ) {
Comment 23 Boris Zbarsky [:bz] 2011-12-21 12:13:46 PST
How about this:

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

Comment 24 :aceman 2011-12-21 12:38:06 PST
Created attachment 583588 [details] [diff] [review]
v5, style fixes

As you wish.
Comment 25 Boris Zbarsky [:bz] 2011-12-21 12:48:29 PST
That looks like comment 22 not comment 23.
Comment 26 :aceman 2011-12-21 12:54:35 PST
Created attachment 583596 [details] [diff] [review]
v6, fixed

Yes, sorry, forgot to update.
Comment 27 Boris Zbarsky [:bz] 2011-12-21 13:31:57 PST
Comment on attachment 583596 [details] [diff] [review]
v6, fixed

Comment 29 Ed Morley [:emorley] 2011-12-22 03:50:23 PST
Comment 30 Eric Shepherd [:sheppy] 2012-04-19 11:19:40 PDT
Documentation updated:

And mentioned on Firefox 12 for developers.

Note You need to log in before you can comment on or make changes to this bug.