Closed Bug 1332245 Opened 3 years ago Closed 3 years ago

Move nsScriptError from js/xpconnect to dom/bindings.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

Derived from bug 1283712.

Currently nsScriptError is stored in js/xpconnect, especially declared in js/xpconnect/src/xpcprivate.h.
bug 1283712 is about to add some more method and class (addNotes, and nsScriptErrorNote) there and don't want to make it mutable via XPIDL for simplicity.
dom/worker needs to mutate, so nsScriptError and nsIScriptError should be moved under dom/
See Also: → 1283712
hmm, things moved to the new module isn't recognized on linux on try run, but works correctly on OSX locally...
Please leave this stuff in nsLayoutModule.
Moved the following files from js/xpconnect/{src,idl} to dom/bindings
  * nsIScriptError.idl
  * nsScriptError.cpp
  * nsScriptErrorWithStack.cpp
and also changed coding style from SpiderMonkey to Gecko.

Also added the following file, separated from js/xpconnect/src/xpcprivate.h
  * dom/bindings/nsScriptError.h
it's published to mozilla/dom/nsScriptError.h and is included from several files
that were using nsScriptError via xpcprivate.h

CID/CONTRACTID things are embedded directly to layout/build/nsLayoutModule.cpp,
moved from XPCONNECT_{CIDENTRIES,CONTRACTS}

One question, what should the value of XPIDL_MODULE for nsIScriptError.idl be?
Attachment #8828748 - Flags: review?(bzbarsky)
Comment on attachment 8828748 [details] [diff] [review]
Move nsScriptError from js/xpconnect to dom/bindings.

Please don't combine moves and coding style changes in a single changeset.  In fact, I'm not sure it's worth the noise of changing coding style at all.

If we _do_ change coding style, I'd like to see a diff -w for whatever changeset does that.

As for XPIDL_MODULE... I guess "dom_bindings", but I expect it really doesn't matter much.
Attachment #8828748 - Flags: review?(bzbarsky) → review-
Okay, reverted the coding style change.
(nsScriptError.h is in Gecko style, since it's new file)

also changed XPIDL_MODULE from dom to dom_bindings, and added dom_bindings.xpt to package-manifest.in files.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4e066498b49e841054ee9ac7358d81c9817ed94&selectedJob=70899136
Attachment #8828748 - Attachment is obsolete: true
Attachment #8829064 - Flags: review?(bzbarsky)
Comment on attachment 8829064 [details] [diff] [review]
Move nsScriptError from js/xpconnect to dom/bindings.

Sorry for the horrible lag here.  :(

> +++ b/b2g/installer/package-manifest.in

Please file a bug on comm-central to add this new xpt to im/installer/package-manifest.in, mail/installer/package-manifest.in, and suite/installer/package-manifest.in.  Not doing that second one will certainly break Thunderbird...

>+++ b/dom/bindings/moz.build
>@@ -29,16 +35,17 @@ EXPORTS.mozilla.dom += [
>+    'nsScriptError.h',

I don't think we should export this to mozilla/dom (for one thing, it's not in the mozilla::dom namespace).

Probably better to not export it at all, and add '/dom/bindings' to LOCAL_INCLUDES in js/xpconnect/src/moz.build and layout/build/moz.build.

>+++ b/dom/bindings/nsScriptError.cpp

>  * nsIScriptError implementation.  Defined here, lacking a JS-specific
>  * place to put XPCOM things.

The second sentence doesn't seem relevant anymore.

>+++ b/dom/bindings/nsScriptError.h

>+// Definition of nsScriptError, defined here because we lack a place to put
>+// XPCOM objects associated with the JavaScript engine.

Again, the second half of this comment doesn't look right anymore.

>+  // TODO - do something reasonable on getting null from these babies.

This comment should probably go away, here or in a followup.

r=me with the above nits fixed.
Attachment #8829064 - Flags: review?(bzbarsky) → review+
See Also: → 1334214
https://hg.mozilla.org/mozilla-central/rev/fa90a8ad3133
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.