1265775 - replace uses of Cu.reportError

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace Cu.reportError uses in devtools for de-chrome-ification project.

Comment 1

[Tom Tromey :tromey]

Note also bug 1179970 (wontfix, but for the other direction); and bug 877389 (for the rest
of the browser).

Comment 2

[Tom Tromey :tromey]

Maybe .catch(Cu.reportError) can use .catch(console.error), though many places
in the tree use .catch(e => console.error(e)), leading us to wonder about "this" binding.

Comment 3

[Tom Tromey :tromey]

:ochameau points out:

<ochameau> tromey: hey! about you comment on console.log. try
	   console.log.apply(null, ["plop"]) in a webpage, it will throw
<ochameau> tromey: the console in modules may be bound, but not the one in
	   documents
<ochameau> tromey: so it is better to assume it is not bound, as we may use
	   console api from tools document scope


Given this I think there are some existing latent bugs where something like
.then(null, console.error) is used.

Comment 4

[Tom Tromey :tromey]

One option here is to use ThreadSafeDevToolsUtils.reportException

Comment 5

[James Long (:jlongster)]

Attachment: 1265775.patch

Initial pass

Comment 6

[James Long (:jlongster)]

Attachment: 1265775.patch

Rebased on top of fx-team

Comment 7

[James Long (:jlongster)]

Let's see what this does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5be54cf49ca

Comment 8

[James Long (:jlongster)]

New run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f4b8c205498

Comment 9

[James Long (:jlongster)]

Attachment: 1265775.patch

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8749244 [details] [diff] [review]
1265775.patch

Review of attachment 8749244 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thank you for doing this.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4566e5d0d25c

Comment 12

[James Long (:jlongster)]

I still need to replace any instances in the devtools/shared directory.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4566e5d0d25c

Comment 14

[James Long (:jlongster)]

Attachment: 1265775-shared.patch

Fairly small patch. I ignored a few places that clearly won't be loaded in the client, like Loaders.jsm

Comment 15

[James Long (:jlongster)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91ea2931c386

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8750449 [details] [diff] [review]
1265775-shared.patch

Review of attachment 8750449 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you.

This patch looks good to me.

I am perhaps mildly worried about testing it.  For the most part I think it's not worth worrying about; but for example the previous transport.js code made me wonder if something unusual is going on there.

Comment 17

[James Long (:jlongster)]

Will push this once the tree opens

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/95e7b6e69427

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95e7b6e69427