Last Comment Bug 795542 - Implement StringEncoding API in Workers
: Implement StringEncoding API in Workers
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Masatoshi Kimura [:emk]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 764234 814257 852226
  Show dependency treegraph
 
Reported: 2012-09-28 19:50 PDT by Masatoshi Kimura [:emk]
Modified: 2013-08-27 03:18 PDT (History)
10 users (show)
VYV03354: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
fixed
fixed
wontfix
fixed


Attachments
Part 1: Avoid reinterpret_cast in Codegen (8.59 KB, patch)
2012-09-28 20:10 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Create Text(En|De)coderBase (22.66 KB, patch)
2012-09-28 20:11 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 3: Implement StringEncoding API objects in Workers (14.63 KB, patch)
2012-09-28 20:11 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 1: Create Text(En|De)coderBase (22.66 KB, patch)
2012-09-29 07:25 PDT, Masatoshi Kimura [:emk]
bent.mozilla: review+
Details | Diff | Splinter Review
Part 2: Implement StringEncoding API objects in Workers (14.58 KB, patch)
2012-09-29 07:26 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Implement StringEncoding API objects in Workers (14.38 KB, patch)
2012-10-04 06:34 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 1: Create Text(En|De)coderBase. r=bent (23.01 KB, patch)
2012-12-14 21:08 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 2: Implement StringEncoding API objects in Workers (14.55 KB, patch)
2012-12-14 21:10 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 1: Create Text(En|De)coderBase. r=bent (22.85 KB, patch)
2012-12-14 21:23 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 2: Implement StringEncoding API objects in Workers (14.56 KB, patch)
2012-12-19 06:24 PST, Masatoshi Kimura [:emk]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2012-09-28 19:50:14 PDT
This API would be useful for Workers. It should be easy because nsIUnicode(En|De)coder and nsICharsetConverterManager is thread-safe.
Comment 1 Masatoshi Kimura [:emk] 2012-09-28 20:10:19 PDT
Created attachment 666158 [details] [diff] [review]
Part 1: Avoid reinterpret_cast in Codegen

reinterpret_cast will not work for TextEncoder (see the next patch).
Comment 2 Masatoshi Kimura [:emk] 2012-09-28 20:11:22 PDT
Created attachment 666159 [details] [diff] [review]
Part 2: Create Text(En|De)coderBase

Separating common logic from main-thread specific parts.
Comment 3 Masatoshi Kimura [:emk] 2012-09-28 20:11:55 PDT
Created attachment 666160 [details] [diff] [review]
Part 3: Implement StringEncoding API objects in Workers
Comment 4 Masatoshi Kimura [:emk] 2012-09-29 07:23:41 PDT
Comment on attachment 666158 [details] [diff] [review]
Part 1: Avoid reinterpret_cast in Codegen

This patch caused test failures...
Comment 5 Masatoshi Kimura [:emk] 2012-09-29 07:25:02 PDT
Created attachment 666205 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase

Swapped the inherit order so that reinterpret_cast works correctly.
Comment 6 Masatoshi Kimura [:emk] 2012-09-29 07:26:01 PDT
Created attachment 666206 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Same as the above.
Comment 7 Masatoshi Kimura [:emk] 2012-10-04 06:34:27 PDT
Created attachment 667943 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Rebased to tip
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-12-12 15:49:23 PST
Comment on attachment 666205 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase

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

r=me with these things fixed up.

::: dom/encoding/TextDecoder.h
@@ +35,5 @@
>      return txtDecoder.forget();
>    }
>  
>    TextDecoder(nsISupports* aGlobal)
> +    : TextDecoderBase(), mGlobal(aGlobal)

Nit: No need to explicitly call base constructor.

::: dom/encoding/TextDecoderBase.h
@@ +37,5 @@
> +  void Init(const nsAString& aEncoding, const bool aFatal, ErrorResult& aRv);
> +
> +public:
> +  virtual
> +  ~TextDecoderBase()

Shouldn't be public.

::: dom/encoding/TextEncoder.h
@@ +34,5 @@
>      return txtEncoder.forget();
>    }
>  
>    TextEncoder(nsISupports* aGlobal)
> +    : TextEncoderBase(), mGlobal(aGlobal)

Nit: Unless you're passing an arg to the base constructor I'd just leave it out.

::: dom/encoding/TextEncoderBase.h
@@ +7,5 @@
> +#define mozilla_dom_textencoderbase_h_
> +
> +#include "jsapi.h"
> +#include "mozilla/dom/BindingUtils.h"
> +#include "mozilla/dom/TextEncoderBinding.h"

Nit: You've got a lot of #include here that you probably should move to a cpp. What do you actually need here to make your header compile?

@@ +35,5 @@
> +  void Init(const Optional<nsAString>& aEncoding, ErrorResult& aRv);
> +
> +public:
> +  virtual
> +  ~TextEncoderBase()

Shouldn't be public.

@@ +61,5 @@
> +                   const bool aStream, ErrorResult& aRv);
> +
> +protected:
> +  virtual JSObject*
> +  CreateUint8Array(JSContext* aCx, char* buf, uint32_t len) = 0;

Nit: The rest of your args are 'a' prefixed. Let's be consistent.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-12-12 16:32:33 PST
Comment on attachment 667943 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

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

This looks good! I'd like to see the changes though.

::: dom/workers/TextDecoder.cpp
@@ +9,5 @@
> +
> +void
> +TextDecoder::_trace(JSTracer* aTrc)
> +{
> +  workers::DOMBindingBase::_trace(aTrc);

Nit: no need for the 'workers::' here, or below.

::: dom/workers/TextDecoder.h
@@ +7,5 @@
> +#define mozilla_dom_workers_textdecoder_h_
> +
> +#include "mozilla/dom/TextDecoderBase.h"
> +#include "mozilla/dom/workers/bindings/DOMBindingBase.h"
> +#include "DOMBindingInlines.h"

These inlines should only be included in cpp files. Please move this and the Create method into the cpp.

@@ +15,5 @@
> +class TextDecoder : public DOMBindingBase, public TextDecoderBase
> +{
> +protected:
> +  TextDecoder(JSContext* aCx)
> +    : DOMBindingBase(aCx)

Nit: : aligned with class name.

@@ +49,5 @@
> +
> +    return txtDecoder;
> +  }
> +
> +  void Decode(const ArrayBufferView* aView,

Nit: return type on its own line

::: dom/workers/TextEncoder.cpp
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "TextEncoder.h"
> +
> +BEGIN_WORKERS_NAMESPACE

Nit: Just USING_WORKERS_NAMESPACE here.

@@ +9,5 @@
> +
> +void
> +TextEncoder::_trace(JSTracer* aTrc)
> +{
> +  workers::DOMBindingBase::_trace(aTrc);

Nit: no need for the 'workers::' here, or below.

::: dom/workers/TextEncoder.h
@@ +7,5 @@
> +#define mozilla_dom_workers_textencoder_h_
> +
> +#include "mozilla/dom/TextEncoderBase.h"
> +#include "mozilla/dom/workers/bindings/DOMBindingBase.h"
> +#include "DOMBindingInlines.h"

These inlines should only be included in cpp files. Please move this and the Create method into the cpp.

@@ +11,5 @@
> +#include "DOMBindingInlines.h"
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +class TextEncoder : public DOMBindingBase, public TextEncoderBase

Nit: one base class per line.

@@ +15,5 @@
> +class TextEncoder : public DOMBindingBase, public TextEncoderBase
> +{
> +protected:
> +  TextEncoder(JSContext* aCx)
> +    : DOMBindingBase(aCx)

Nit: : aligned with class name.

@@ +48,5 @@
> +
> +    return txtEncoder;
> +  }
> +
> +  JSObject* Encode(JSContext* aCx,

Nit: return type on its own line.

@@ +57,5 @@
> +  }
> +
> +protected:
> +  virtual JSObject*
> +  CreateUint8Array(JSContext* aCx, char* buf, uint32_t len) MOZ_OVERRIDE

Nit: aBuf and aLen
Comment 10 Masatoshi Kimura [:emk] 2012-12-14 21:08:52 PST
Created attachment 692547 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase. r=bent
Comment 11 Masatoshi Kimura [:emk] 2012-12-14 21:10:21 PST
Created attachment 692548 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Rebased to tip and resolved review comments.
Comment 12 Masatoshi Kimura [:emk] 2012-12-14 21:11:42 PST
https://tbpl.mozilla.org/?tree=Try&rev=563a7c05a328
Comment 13 Masatoshi Kimura [:emk] 2012-12-14 21:23:24 PST
Created attachment 692549 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase. r=bent

Trimmed an extra comment header.
Comment 14 Masatoshi Kimura [:emk] 2012-12-19 06:24:07 PST
Created attachment 693855 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Fixed a bitrot by bug 820544.
Comment 15 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-12-21 09:10:01 PST
Comment on attachment 693855 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

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

Looks great! Thanks!

::: dom/bindings/Bindings.conf
@@ +436,5 @@
>  
> +'TextDecoder': [
> +{
> +    'workers': True,
> +}],

Nit: You don't need the array here I think.

::: dom/workers/TextDecoder.cpp
@@ +5,5 @@
> +
> +#include "TextDecoder.h"
> +#include "DOMBindingInlines.h"
> +
> +using namespace mozilla;

Hm, is this needed?

@@ +21,5 @@
> +{
> +  DOMBindingBase::_finalize(aFop);
> +}
> +
> +/*static */TextDecoder*

Nit: Let's do this for clarity:

  // static
  TextDecoder*
  TextDecoder::Constructor(...)

::: dom/workers/TextEncoder.cpp
@@ +5,5 @@
> +
> +#include "TextEncoder.h"
> +#include "DOMBindingInlines.h"
> +
> +using namespace mozilla;

Is this needed?

@@ +20,5 @@
> +{
> +  DOMBindingBase::_finalize(aFop);
> +}
> +
> +/*static */TextEncoder*

Same nit here.

::: dom/workers/TextEncoder.h
@@ +45,5 @@
> +  }
> +
> +protected:
> +  virtual JSObject*
> +  CreateUint8Array(JSContext* aCx, char* aBuf, uint32_t aLen) MOZ_OVERRIDE

Nit: Just put this in the other protected block above?
Comment 16 Masatoshi Kimura [:emk] 2012-12-21 16:23:41 PST
(In reply to ben turner [:bent] from comment #15)
> ::: dom/bindings/Bindings.conf
> @@ +436,5 @@
> >  
> > +'TextDecoder': [
> > +{
> > +    'workers': True,
> > +}],
> 
> Nit: You don't need the array here I think.

If the array is absent, CodeGen doesn't generate non-workers binding (like FileReaderSync).
We need both of non-workers binding and workers binding here, so the array is needed.

> ::: dom/workers/TextDecoder.cpp
> @@ +5,5 @@
> > +
> > +#include "TextDecoder.h"
> > +#include "DOMBindingInlines.h"
> > +
> > +using namespace mozilla;
> 
> Hm, is this needed?

Changed it to |using mozilla::ErrorResult;|. (Same for TextEncoder.)

> @@ +21,5 @@
> > +{
> > +  DOMBindingBase::_finalize(aFop);
> > +}
> > +
> > +/*static */TextDecoder*
> 
> Nit: Let's do this for clarity:
> 
>   // static
>   TextDecoder*
>   TextDecoder::Constructor(...)

Done. (Same for TextEncoder.)

> ::: dom/workers/TextEncoder.h
> @@ +45,5 @@
> > +  }
> > +
> > +protected:
> > +  virtual JSObject*
> > +  CreateUint8Array(JSContext* aCx, char* aBuf, uint32_t aLen) MOZ_OVERRIDE
> 
> Nit: Just put this in the other protected block above?

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/905b2cfe26ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac143af3f698
Comment 18 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-02-18 18:45:09 PST
I'm currently moving the Email code to a worker and this will be extremely painful without this patch on b2g-18. I feel like this is not risky since this touch only this particular API that is really used yet.
Comment 19 Masatoshi Kimura [:emk] 2013-02-18 19:37:04 PST
You can use FileReaderSync in workers to work around.
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-19 08:05:03 PST
So with comment 19 proposing a workaround with an existing, known API, we will not block here on such a new implementation so late in v1.0.1
Comment 21 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-02-22 05:39:45 PST
(In reply to Lukas Blakk [:lsblakk] from comment #20)
> So with comment 19 proposing a workaround with an existing, known API, we
> will not block here on such a new implementation so late in v1.0.1

It seems much nore risky for me to change the behavior of the email protocols to switch to this API than to expose an API to worker. The interface of this API won't change on worker so there is not logic error risk and the risk could be limited to weird crash . This API has baked for months in m-c so can you elaborate why it is less risky than changing how works the internal of the email app? FileReaderSync except File or Blob while a lot of calls use raw strings or array buffers directly.

Renoming since 'such a new implementation' for an API that expose months ago is probably way less new than many APIs that has landed for v1.0.0. :)
Comment 22 Alex Keybl [:akeybl] 2013-02-22 08:31:19 PST
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #21)
> Renoming since 'such a new implementation' for an API that expose months ago
> is probably way less new than many APIs that has landed for v1.0.0. :)

Can this land alongside bug 814257, or is there value to tef+'ing already?
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-02-22 08:33:14 PST
Also, can we make sure to run all the main-thread-tests for the StringEncoding API also running in workers? And land those tests on m-c/aurora/beta branches. That way we'll have more confidence that this code is working.
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-28 08:04:11 PST
Given that bug 814257 isn't nominated and doesn't appear to be ready for uplift in time, we'll minus this for v1.0.1
Comment 25 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-03-01 03:48:03 PST
(In reply to Alex Keybl [:akeybl] from comment #22)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #21)
> > Renoming since 'such a new implementation' for an API that expose months ago
> > is probably way less new than many APIs that has landed for v1.0.0. :)
> 
> Can this land alongside bug 814257, or is there value to tef+'ing already?

No value to tef+ it before to be honest.
Comment 26 Andrew Sutherland [:asuth] 2013-03-17 18:29:19 PDT
(In reply to Vivien Nicolas (:vingtetun) (:21) (away until march 25th) from comment #25)
> (In reply to Alex Keybl [:akeybl] from comment #22)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #21)
> > > Renoming since 'such a new implementation' for an API that expose months ago
> > > is probably way less new than many APIs that has landed for v1.0.0. :)
> > 
> > Can this land alongside bug 814257, or is there value to tef+'ing already?
> 
> No value to tef+ it before to be honest.

I think we want this uplifted now to v1.0.1 and v1-train so that people will be able to test the worker-thread branch of the e-mail app without needing a custom build.

I have requested bug 814257 be formally marked tef+ since that's its de facto priority, and I think accordingly this should get tef+ too:
https://bugzilla.mozilla.org/show_bug.cgi?id=814257#c31
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-03-21 08:17:58 PDT
This doesn't apply cleanly to the b2g18 branch. Please post branch-specific patches (and set checkin-needed again when ready).
Comment 28 Andrew Sutherland [:asuth] 2013-03-21 09:00:13 PDT
:emk, were Vivien's patches on bug 814257 okay apart from the removal of the optional default for encoding you pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=814257#c22, or should the patches be regenerated? from hg here.  It's not clear to me how he arrived at those patches given that regression or how deep an inspection of those patches you did?  Thanks!

For reference, the patches are:
https://bugzilla.mozilla.org/attachment.cgi?id=722933&action=edit
https://bugzilla.mozilla.org/attachment.cgi?id=722934&action=edit
Comment 29 Masatoshi Kimura [:emk] 2013-03-21 09:14:31 PDT
Looks good to me. (Except for the last hunk in the second patch. It doesn't look like it belongs to this patch.)
Comment 31 Masatoshi Kimura [:emk] 2013-03-23 01:30:41 PDT
Backed out because of test failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/563c58c449c6
Comment 33 Mihai Morar, (:MihaiMorar) 2013-03-26 02:35:30 PDT
Is this related for Desktop Firefox? If it does, how can QA verify the fix?
Comment 34 Masatoshi Kimura [:emk] 2013-03-26 05:10:17 PDT
No, b2g only. Desktop Firefox will have TextDecoder and TextEncoder in workers since the next version (20), so I'm not planning to backport to desktop branches.
Comment 35 Daniel Coloma:dcoloma 2013-04-06 16:48:40 PDT
Can we land this in v1.0.1 as per bug flags?
Comment 36 Masatoshi Kimura [:emk] 2013-04-06 21:31:35 PDT
Yes.
Comment 39 Andrew Sutherland [:asuth] 2013-08-23 14:46:48 PDT
:teoli, this was landed for Firefox OS's v1.0.1 and v1.1, which are Gecko 18-ish (Firefox 18 never had/won't have it, but Firefox OS does).  Is there a special MDN flag we can set to convey that?
Comment 40 Jean-Yves Perrier [:teoli] 2013-08-23 15:52:00 PDT
We should add a column for FirefoxOS. We have discussed about it last week at the writer meeting. 

We already have a special "template" to indicate this info in it. I will do it as soon as I land.

I switch the flag so that I cannot forget it :-)
Comment 41 Jean-Yves Perrier [:teoli] 2013-08-27 03:18:20 PDT
I added a Firefox OS column in compat tables of all related articles.

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