Last Comment Bug 801487 - Update StringEncoding API per the latest spec and fix some bugs
: Update StringEncoding API per the latest spec and fix some bugs
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
: 802979 (view as bug list)
Depends on: 1125698
Blocks: 764234
  Show dependency treegraph
 
Reported: 2012-10-14 19:24 PDT by Masatoshi Kimura [:emk]
Modified: 2015-01-25 19:15 PST (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.20 KB, patch)
2012-10-15 12:27 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch v2 (12.09 KB, patch)
2012-10-15 16:29 PDT, Masatoshi Kimura [:emk]
jonas: review+
ryanvm: checkin+
Details | Diff | Splinter Review
Update WebIDL definition (5.17 KB, patch)
2012-10-23 15:26 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Add a JSAPI to make a report with unicode arguments (3.23 KB, patch)
2012-10-23 15:29 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Change some exceptions from EncodingError to TypeError (18.06 KB, patch)
2012-10-23 15:30 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Change some exceptions from EncodingError to TypeError (18.88 KB, patch)
2012-10-23 16:07 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Update WebIDL definition (5.18 KB, patch)
2012-10-24 14:18 PDT, Masatoshi Kimura [:emk]
jonas: review+
ryanvm: checkin+
Details | Diff | Splinter Review
Add a JSAPI to make a report with unicode arguments (3.29 KB, patch)
2012-10-24 14:19 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Change some exceptions from EncodingError to TypeError (18.93 KB, patch)
2012-10-24 14:20 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Add a JSAPI to make a report with unicode arguments (9.37 KB, patch)
2012-10-26 16:07 PDT, Masatoshi Kimura [:emk]
jwalden+bmo: review+
Details | Diff | Splinter Review
Implement ErrorResult::ThrowTypeError (7.12 KB, patch)
2012-10-26 16:10 PDT, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review
Part 5: Change some exceptions from EncodingError to TypeError (10.15 KB, patch)
2012-10-26 16:11 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Implement ErrorResult::ThrowTypeError. r=bz (7.75 KB, patch)
2012-10-27 14:43 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Implement ErrorResult::ThrowTypeError. r=bz (7.80 KB, patch)
2012-10-30 11:51 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Add a JSAPI to make a report with unicode arguments. r=waldo (9.12 KB, patch)
2012-11-03 05:46 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 6: Remove encoding detection using BOM (14.59 KB, patch)
2012-11-04 06:32 PST, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Part 7: Import the latest tests from https://code.google.com/p/stringencoding/ (21.85 KB, patch)
2012-11-04 12:57 PST, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Part 8: Import the latest tests from https://code.google.com/p/stringencoding/ , part 2 (4.79 KB, patch)
2012-11-04 12:58 PST, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Part 9: Fixup imported tests (7.02 KB, patch)
2012-11-04 13:01 PST, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Part 4: Implement ErrorResult::ThrowTypeError (7.82 KB, patch)
2012-11-04 13:06 PST, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review
Part 3: Add a JSAPI to make a report with unicode arguments (9.88 KB, patch)
2012-11-04 13:08 PST, Masatoshi Kimura [:emk]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 4: Implement ErrorResult::ThrowTypeError. r=bz (7.82 KB, patch)
2012-11-06 11:53 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2012-10-14 19:24:20 PDT
I should have noticed that sooner :(
Comment 1 Masatoshi Kimura [:emk] 2012-10-15 12:04:35 PDT
I found some other bugs and spec updates. Morphing the bug.
Comment 2 Masatoshi Kimura [:emk] 2012-10-15 12:27:33 PDT
Created attachment 671544 [details] [diff] [review]
patch

- Cahnged ToLowerCase to ASCIIToLower.
- Added 2 encodings ("iso-8859-8-i", "x-user-defined") to catch up the spec.
- Fixed "shift_jis" encoding name.
- Removed "x-windows-949" hack because EUC-KR decoder was just an alias of x-windows-949 decoder and TextEncoder does not support non-UTF encodings.
- Added testcases verifying above changes.
Comment 3 Masatoshi Kimura [:emk] 2012-10-15 16:29:23 PDT
Created attachment 671648 [details] [diff] [review]
patch v2

Improved the test code a bit.
Comment 4 Masatoshi Kimura [:emk] 2012-10-18 04:21:50 PDT
*** Bug 802979 has been marked as a duplicate of this bug. ***
Comment 5 Masatoshi Kimura [:emk] 2012-10-23 15:26:30 PDT
Created attachment 674406 [details] [diff] [review]
Update WebIDL definition

The first argument of encode/decode has a default value until https://www.w3.org/Bugs/Public/show_bug.cgi?id=19646 is fixed.
Comment 6 Masatoshi Kimura [:emk] 2012-10-23 15:29:15 PDT
Created attachment 674407 [details] [diff] [review]
Add a JSAPI to make a report with unicode arguments

This patch also fix a bug of js_ErrorToException. js_ErrorToException couldn't create an Exception with a non-ASCII message.
Comment 7 Masatoshi Kimura [:emk] 2012-10-23 15:30:03 PDT
Created attachment 674408 [details] [diff] [review]
Change some exceptions from EncodingError to TypeError

According to the latest spec.
Comment 8 Masatoshi Kimura [:emk] 2012-10-23 16:07:23 PDT
Created attachment 674426 [details] [diff] [review]
Change some exceptions from EncodingError to TypeError
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-24 12:28:59 PDT
Please flag any other patches that you need reviewed.
Comment 10 Masatoshi Kimura [:emk] 2012-10-24 14:18:08 PDT
Created attachment 674828 [details] [diff] [review]
Update WebIDL definition
Comment 11 Masatoshi Kimura [:emk] 2012-10-24 14:19:13 PDT
Created attachment 674829 [details] [diff] [review]
Add a JSAPI to make a report with unicode arguments
Comment 12 Masatoshi Kimura [:emk] 2012-10-24 14:20:00 PDT
Created attachment 674832 [details] [diff] [review]
Change some exceptions from EncodingError to TypeError
Comment 13 Masatoshi Kimura [:emk] 2012-10-24 14:20:36 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fb87802de76d
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-24 16:12:31 PDT
Comment on attachment 674832 [details] [diff] [review]
Change some exceptions from EncodingError to TypeError

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

I think bz needs to review this. I can do the TextEncoder/Decoder parts once he has ok'ed the other changes. It seems very unfortunate to me to require a context for throwing type errors. Can't the generated bindings code do that?
Comment 15 Masatoshi Kimura [:emk] 2012-10-24 16:36:06 PDT
TypeError is spec'ed by the Encoding Standard itself, not the WebIDL spec. So I don't think it belongs to the bindings code.
Anne, why did you change the error to TypeError?
Comment 16 Boris Zbarsky [:bz] 2012-10-24 18:00:17 PDT
I think the right way to do this is to have ThrowTypeError on ErrorResult, as this patch does, but have that just set state on the object.  Then the generated code should do the actual exception-throwing.  That's sort of been the exceptions plan all along.
Comment 17 Boris Zbarsky [:bz] 2012-10-24 18:01:49 PDT
And in particular, what the codegen does right now looks like this:

          if (rv.Failed()) {
            return ThrowMethodFailedWithDetails<true>(cx, rv, "XMLHttpRequest", "send");
          }

and ThrowMethodFailedWithDetails could do special things in case rv actually had a TypeError thrown on it.
Comment 18 Masatoshi Kimura [:emk] 2012-10-24 18:17:44 PDT
How can I pass around varargs across the function?
Comment 19 Boris Zbarsky [:bz] 2012-10-24 18:26:25 PDT
Hmm.  You want varargs because you want to be able to report the label that was passed in?

In this case, I would say just throw NOT_UTF for either failure.  But in general we'd need to figure out some way of doing that, yes...

One option, I guess, might be for the ErrorResult to have a JSContext*.  But I'd really like to keep it as cheap as possible in the non-exceptional cases.  :(
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-24 19:12:20 PDT
Comment on attachment 674828 [details] [diff] [review]
Update WebIDL definition

https://hg.mozilla.org/integration/mozilla-inbound/rev/05636b372638
Comment 22 Anne (:annevk) 2012-10-25 00:26:24 PDT
To answer comment 15, the idea is to emulate the semantics of a string enum, without writing out the actual enum in Web IDL as it would be very verbose (at least for TextDecoder). If I add a comment to that effect to the spec, would that be sufficient?
Comment 24 Boris Zbarsky [:bz] 2012-10-25 06:06:57 PDT
> without writing out the actual enum in Web IDL as it would be very verbose 

Why is that a problem, exactly?

But note that in this case the callee only accepts the UTF-8/16 encodings anyway, not all of them....
Comment 25 Masatoshi Kimura [:emk] 2012-10-25 06:16:22 PDT
TextDecoder supports non-UTF encodings while TextEncoder doesn't.
Comment 26 Boris Zbarsky [:bz] 2012-10-25 06:24:05 PDT
In which case it might actually make sense to have them take different types in the IDL to make this clear, fwiw.
Comment 27 Masatoshi Kimura [:emk] 2012-10-25 06:27:10 PDT
Other problem is, Web IDL enum doesn't support case-insensitive matching and trimming space characters.
Comment 28 Boris Zbarsky [:bz] 2012-10-25 06:34:47 PDT
Ah.  Yes, ok.  That last makes it infeasible to use here.
Comment 29 Anne (:annevk) 2012-10-25 13:07:35 PDT
bz, shall I name the argument utfLabel for TextEncoder to indicate it is somewhat different? FWIW, I'm happy to use a different exception here, maybe NotSupportedError? EncodingError just did not seem right as it's not related to any encoding or decoding error, it's just an incorrect argument.
Comment 30 Boris Zbarsky [:bz] 2012-10-25 13:09:05 PDT
> utfLabel for TextEncoder to indicate it is somewhat different?

Might not be a bad idea.

I don't think there's a problem with having a TypeError here per se, for what it's worth.  But then again, I also think that it doesn't matter that much what sorts of exceptions are thrown.  ;)
Comment 31 Masatoshi Kimura [:emk] 2012-10-26 16:07:30 PDT
Created attachment 675729 [details] [diff] [review]
Add a JSAPI to make a report with unicode arguments

I changed the function not to depend on varargs.
Comment 32 Masatoshi Kimura [:emk] 2012-10-26 16:10:31 PDT
Created attachment 675735 [details] [diff] [review]
Implement ErrorResult::ThrowTypeError

The only overhead is one pointer-size storage. It will not be even touched unless TypeError is thrown. It should be negligible because ErrorResult is noncopyable.
Comment 33 Masatoshi Kimura [:emk] 2012-10-26 16:11:16 PDT
Created attachment 675737 [details] [diff] [review]
Part 5: Change some exceptions from EncodingError to TypeError
Comment 34 Boris Zbarsky [:bz] 2012-10-26 23:18:04 PDT
Comment on attachment 675735 [details] [diff] [review]
Implement ErrorResult::ThrowTypeError

Why not make mErrorNumber be a dom::ErrNum?

>+  MOZ_ASSERT(argCount <= 10);

How about doing that in ThrowTypeError?  So if argCount there is too large, simply delete message, set mMessage to null, and move on (after asserting).

That will make the failure mode "wrong exception thrown" not "out of bounds stack write.

Alternately, you could just set argCount = NS_MIN(argCount, 10) or something and ignore extra args (after the assert you have).

But also, you could just do:

  const jschar* args[argCount+1];

and not worry about all this, no?

>+#ifdef DEBUG
>+  ~ErrorResult() {
>+    MOZ_ASSERT_IF(IsTypeError(), !mMessage);
>+  }
>+#endif

There are ErrorResult consumers who don't ever use ThrowMethodFailedWithDetails.  In fact, they just swallow the error...

It might be safer to just make mMessage an nsAutoPtr....

r=me with that, I guess.
Comment 35 Masatoshi Kimura [:emk] 2012-10-26 23:52:25 PDT
(In reply to Boris Zbarsky (:bz) from comment #34)
> Why not make mErrorNumber be a dom::ErrNum?
Will do.

> >+  MOZ_ASSERT(argCount <= 10);
> 
> How about doing that in ThrowTypeError?  So if argCount there is too large,
> simply delete message, set mMessage to null, and move on (after asserting).
> 
> That will make the failure mode "wrong exception thrown" not "out of bounds
> stack write.
It will crash with buffer overrun in js anyway.
https://mxr.mozilla.org/mozilla-central/source/js/src/jscntxt.cpp?rev=d1984b76a0b2#886

> Alternately, you could just set argCount = NS_MIN(argCount, 10) or something
> and ignore extra args (after the assert you have).
We can't change the argCount here (see the js code above). I don't think assertion can actually fail for a reason other than silly typo because argCount is statically embedded in Errors.msg.

> But also, you could just do:
> 
>   const jschar* args[argCount+1];
> 
> and not worry about all this, no?
MSVC doesn't support C99 variable length arrays :( 

> >+#ifdef DEBUG
> >+  ~ErrorResult() {
> >+    MOZ_ASSERT_IF(IsTypeError(), !mMessage);
> >+  }
> >+#endif
> 
> There are ErrorResult consumers who don't ever use
> ThrowMethodFailedWithDetails.  In fact, they just swallow the error...
Even worse, it will cause memory leak.
But no such consumers use NS_ERROR_TYPE_ERR because ThrowTypeError is a newly added method. Also I added assertions to catch such a usage.

> It might be safer to just make mMessage an nsAutoPtr....
nsAutoPtr will always initialize mMessage even if no errors are thrown. Is it OK?
Comment 36 Masatoshi Kimura [:emk] 2012-10-27 01:57:22 PDT
> nsAutoPtr will always initialize mMessage even if no errors are thrown. Is it OK?
And it will always check mMessage on destruction. If it doesn't matter, I'll gladly replace it with nsAutoPtr.
Comment 37 Boris Zbarsky [:bz] 2012-10-27 08:09:53 PDT
> We can't change the argCount here (see the js code above).

My point is that you can just ignore args after the 10th one.  Is that not the case?

> MSVC doesn't support C99 variable length arrays :( 

Even in C++?  I was pretty sure we use them elsewhere in the tree....

> Even worse, it will cause memory leak.

Well, right.   That's the problem.  Swallowing the error is intended behavior for those consumers, but leaking is not.

> Also I added assertions to catch such a usage.

Hmm.  OK, fair.  Given that, I guess let's go without nsAutoPtr for now.  I agree that having the extra init/check is undesirable.
Comment 38 Masatoshi Kimura [:emk] 2012-10-27 14:43:11 PDT
Created attachment 675896 [details] [diff] [review]
Implement ErrorResult::ThrowTypeError. r=bz

(In reply to Boris Zbarsky (:bz) from comment #37)
> > We can't change the argCount here (see the js code above).
> 
> My point is that you can just ignore args after the 10th one.  Is that not
> the case?

OK, I've moved assertion into ThrowTypeError and added |argCount = std::min<uint16_t>(argCount, 10);|. (NS_MIN is going to be obsoleted per bug 786533.)

> > MSVC doesn't support C99 variable length arrays :( 
> 
> Even in C++?  I was pretty sure we use them elsewhere in the tree....

When I changed the code to |const jschar* args[argCount + 1];|, compile failed with the following error:
h:/m/mozilla-central/dom/bindings/BindingUtils.cpp(82) : error C2057: expected constant expression
h:/m/mozilla-central/dom/bindings/BindingUtils.cpp(82) : error C2466: cannot allocate an array of constant size 0
h:/m/mozilla-central/dom/bindings/BindingUtils.cpp(82) : error C2133: 'args' : unknown size
Supporting variable length arrays in C++ is a gcc extension. Even C++11 didn't spec that. I'd be surprised if it is actually used in our XP code.

> > Also I added assertions to catch such a usage.
> 
> Hmm.  OK, fair.  Given that, I guess let's go without nsAutoPtr for now.  I
> agree that having the extra init/check is undesirable.

I've added a comment explaining why a raw pointer is used.
Comment 39 Masatoshi Kimura [:emk] 2012-10-30 11:51:53 PDT
Created attachment 676692 [details] [diff] [review]
Implement ErrorResult::ThrowTypeError. r=bz

Rebased to tip
Comment 40 Jeff Walden [:Waldo] (remove +bmo to email) 2012-11-02 21:23:47 PDT
Comment on attachment 675729 [details] [diff] [review]
Add a JSAPI to make a report with unicode arguments

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

::: js/src/jsapi.cpp
@@ +6732,5 @@
> +                         va_list ap)
> +{
> +    AssertHeapIsIdle(cx);
> +    js_ReportErrorNumberVA(cx, JSREPORT_ERROR, errorCallback, userRef,
> +                           errorNumber, JS_FALSE, ap);

The JS_FALSE here (which I would ordinarily say should be false, just so you're aware for other patches) is totally unintelligible when read here.  :-)

These days we would make this an enum ArgumentType { AsciiString, UnicodeString } so that places like this could pass something readable, and more obviously correct or not-correct.  Could you file a followup bug to make this change, please?  Could be good first bug material, if I don't get irritated enough by this to just go do it myself.  :-)

::: js/src/jscntxt.cpp
@@ -914,5 @@
>                  argLengths[i] = js_strlen(reportp->messageArgs[i]);
>                  totalArgsLength += argLengths[i];
>              }
> -            /* NULL-terminate for easy copying. */
> -            reportp->messageArgs[i] = NULL;

SpiderMonkey's error reporting API, mechanism, and code is a disaster, as I'm sure you've discovered.  :-)  Which makes me think that here, if you're not going to null-terminate in the case when messageArgs were passed in from the start, this entry is NULL.  You should assert this fact as well, so if any user screws it up somehow at some point, we'll quickly discover it.

@@ +1072,5 @@
> +                            void *userRef, const unsigned errorNumber,
> +                            const jschar **args)
> +{
> +    if (checkReportFlags(cx, &flags))
> +        return JS_TRUE;

SpiderMonkey doesn't use JS_TRUE or JS_FALSE in new code -- we use true and false directly.  We also use bool instead of JSBool, everywhere except in APIs (at least for the moment; SpiderMonkey's transition to being C++ means we might remove JSBool entirely sometime).  (There might be one or two cases where we make exceptions, that don't immediately spring to mind; nothing in this function is such a case.)

@@ +1073,5 @@
> +                            const jschar **args)
> +{
> +    if (checkReportFlags(cx, &flags))
> +        return JS_TRUE;
> +    JSBool warning = JSREPORT_IS_WARNING(flags);

bool
Comment 41 Masatoshi Kimura [:emk] 2012-11-03 05:46:11 PDT
Created attachment 677999 [details] [diff] [review]
Add a JSAPI to make a report with unicode arguments. r=waldo

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #40)
> Comment on attachment 675729 [details] [diff] [review]
> Add a JSAPI to make a report with unicode arguments
> 
> Review of attachment 675729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.cpp
> @@ +6732,5 @@
> > +                         va_list ap)
> > +{
> > +    AssertHeapIsIdle(cx);
> > +    js_ReportErrorNumberVA(cx, JSREPORT_ERROR, errorCallback, userRef,
> > +                           errorNumber, JS_FALSE, ap);
> 
> The JS_FALSE here (which I would ordinarily say should be false, just so
> you're aware for other patches) is totally unintelligible when read here. 
> :-)
> 
> These days we would make this an enum ArgumentType { AsciiString,
> UnicodeString } so that places like this could pass something readable, and
> more obviously correct or not-correct.  Could you file a followup bug to
> make this change, please?  Could be good first bug material, if I don't get
> irritated enough by this to just go do it myself.  :-)

Filed bug 808286.

> ::: js/src/jscntxt.cpp
> @@ -914,5 @@
> >                  argLengths[i] = js_strlen(reportp->messageArgs[i]);
> >                  totalArgsLength += argLengths[i];
> >              }
> > -            /* NULL-terminate for easy copying. */
> > -            reportp->messageArgs[i] = NULL;
> 
> SpiderMonkey's error reporting API, mechanism, and code is a disaster, as
> I'm sure you've discovered.  :-)  Which makes me think that here, if you're
> not going to null-terminate in the case when messageArgs were passed in from
> the start, this entry is NULL.  You should assert this fact as well, so if
> any user screws it up somehow at some point, we'll quickly discover it.

Added an assertion.

> @@ +1072,5 @@
> > +                            void *userRef, const unsigned errorNumber,
> > +                            const jschar **args)
> > +{
> > +    if (checkReportFlags(cx, &flags))
> > +        return JS_TRUE;
> 
> SpiderMonkey doesn't use JS_TRUE or JS_FALSE in new code -- we use true and
> false directly.  We also use bool instead of JSBool, everywhere except in
> APIs (at least for the moment; SpiderMonkey's transition to being C++ means
> we might remove JSBool entirely sometime).  (There might be one or two cases
> where we make exceptions, that don't immediately spring to mind; nothing in
> this function is such a case.)

Done.

> @@ +1073,5 @@
> > +                            const jschar **args)
> > +{
> > +    if (checkReportFlags(cx, &flags))
> > +        return JS_TRUE;
> > +    JSBool warning = JSREPORT_IS_WARNING(flags);
> 
> bool

Done.
Comment 42 Masatoshi Kimura [:emk] 2012-11-04 06:32:30 PST
Created attachment 678112 [details] [diff] [review]
Part 6: Remove encoding detection using BOM

The latest spec doesn't mention useBOM internal flag anymore.
Comment 43 Masatoshi Kimura [:emk] 2012-11-04 12:57:30 PST
Created attachment 678161 [details] [diff] [review]
Part 7: Import the latest tests from https://code.google.com/p/stringencoding/
Comment 44 Masatoshi Kimura [:emk] 2012-11-04 12:58:57 PST
Created attachment 678162 [details] [diff] [review]
Part 8: Import the latest tests from https://code.google.com/p/stringencoding/ , part 2

Using -u0 to reduce the patch size.
Comment 45 Masatoshi Kimura [:emk] 2012-11-04 13:01:50 PST
Created attachment 678163 [details] [diff] [review]
Part 9: Fixup imported tests

- Disabled tests for unpaired surrogates because Gecko doesn't comply the spec yet.
- Removed "ibm864" from and added "iso-8859-8-i" and "x-user-defined" to the encoding list. The test doesn't catch up the latest spec yet.
Comment 46 Masatoshi Kimura [:emk] 2012-11-04 13:06:22 PST
Created attachment 678164 [details] [diff] [review]
Part 4: Implement ErrorResult::ThrowTypeError

The new imported test caused an assertion failure here.
https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp?rev=265427a0694d#150
Comment 47 Masatoshi Kimura [:emk] 2012-11-04 13:07:21 PST
Interdiff for the reference.
https://bugzilla.mozilla.org/attachment.cgi?oldid=676692&action=interdiff&newid=678164&headers=1
Comment 48 Masatoshi Kimura [:emk] 2012-11-04 13:08:19 PST
Created attachment 678165 [details] [diff] [review]
Part 3: Add a JSAPI to make a report with unicode arguments

Added an assertion to catch the error earlier.
Comment 50 Boris Zbarsky [:bz] 2012-11-04 19:13:10 PST
Comment on attachment 678164 [details] [diff] [review]
Part 4: Implement ErrorResult::ThrowTypeError

r=me
Comment 51 Masatoshi Kimura [:emk] 2012-11-05 12:03:11 PST
https://tbpl.mozilla.org/?tree=Try&rev=ab08d2661719
Comment 52 Jonas Sicking (:sicking) PTO Until July 5th 2012-11-05 15:11:54 PST
Comment on attachment 678112 [details] [diff] [review]
Part 6: Remove encoding detection using BOM

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

r=me assuming that this is what the spec says.

That said, I'm a bit surprised that the API doesn't support any type of BOM detection. But I guess this is fine to do for now and if people want BOM detection they can request it added to the spec.
Comment 53 Masatoshi Kimura [:emk] 2012-11-06 11:53:55 PST
Created attachment 678829 [details] [diff] [review]
Part 4: Implement ErrorResult::ThrowTypeError. r=bz

Unbitrotted

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