1237357 - missing Cr used by AddonLocalizationConverter in simpleServices

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

Cr is not currently defined in "toolkit/components/utils/simpleServices.js" and this prevents AddonLocalizationConverter to correctly report errors.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8704747 [details] [diff] [review]
0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch

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

I thought I'd already fixed this. Thanks.

Can you also update the test (toolkit/components/extensions/test/xpcshell/test_locale_converter.js) to check that `e.result == Cr.NS_ERROR_INVALID_ARG`? The second to `Assert.throws` can be a function, so something like `e => e.result === Cr.NS_ERROR_INVALID_ARG` should do.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch

Patch updated with the suggested change applied to the related test case.

(Thanks Kris! it was exactly what I was searching for: 'where this code is covered by tests and why the issue did not come out before')

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8704883 [details] [diff] [review]
0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch

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

(In reply to Luca Greco [:rpl] from comment #3)
> (Thanks Kris! it was exactly what I was searching for: 'where this code is
> covered by tests and why the issue did not come out before')

Yeah, the test layout is a bit weird. I wrote some quick docs about it
yesterday: https://wiki.mozilla.org/WebExtensions/Hacking#Unit_tests

::: toolkit/components/extensions/test/xpcshell/test_locale_converter.js
@@ +114,1 @@
>    });

Arrow function, please. `e => e.result === Cr.NS_ERROR_INVALID_ARG` or `e => { ...`

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Kris Maglione [:kmag] from comment #4)
> Comment on attachment 8704883 [details] [diff] [review]
> 0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch
> 
> Review of attachment 8704883 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/components/extensions/test/xpcshell/test_locale_converter.js
> @@ +114,1 @@
> >    });
> 
> Arrow function, please. `e => e.result === Cr.NS_ERROR_INVALID_ARG` or `e =>
> { ...`

Initially I tried to use an arrow function but Assert.throws complained about the
prototype of the expected (which is the arrow function itself):

ERROR Unexpected exception TypeError: 'prototype' property of expected is not an object at resource://testing-common/Assert.jsm:317
expectedException@resource://testing-common/Assert.jsm:317:1
proto.throws@resource://testing-common/Assert.jsm:358:31
testInvalidUUID@/zfs-tardis/mozlab/desktop/fx-team/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/components/extensions/test/xpcshell/test_locale_converter.js:116:3
_run_next_test@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:1483:9
do_execute_soon/<.run@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:675:9
_do_main@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:208:5
_execute_test@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:535:5
@-e:1:1

it is the first time I'm using the xpcshell tests, so I'm not sure if the issue on retrieving the arrow function prototype is related to its sightly different environment, and unfortunately Assert.jsm check if the actual exception is an instance of expected just before using it as a function:

- https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/testing/modules/Assert.jsm#317

Comment 6

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #5)
> (In reply to Kris Maglione [:kmag] from comment #4)
> > Comment on attachment 8704883 [details] [diff] [review]
> > 0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch
> > 
> > Review of attachment 8704883 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: toolkit/components/extensions/test/xpcshell/test_locale_converter.js
> > @@ +114,1 @@
> > >    });
> > 
> > Arrow function, please. `e => e.result === Cr.NS_ERROR_INVALID_ARG` or `e =>
> > { ...`
> 
> Initially I tried to use an arrow function but Assert.throws complained
> about the prototype of the expected (which is the arrow function itself):

OK. Please just add a short comment about why arrow functions don't work,
then.

It might also be worth filing a bug in the testing component to fix
Assert.throws to work with arrow functions.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1237357-Fix-missing-Cr-used-by-AddonLocalization.patch

Patch updated (added a comment about the Assert.throws and arrow function issue, with a reference to the opened Bug 1237961)

Comment 8

[Pulsebot]

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

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/af1d9d9f34ec