795542 - Implement StringEncoding API in Workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Masatoshi Kimura [:emk]]

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]]

Attachment: Part 1: Avoid reinterpret_cast in Codegen

reinterpret_cast will not work for TextEncoder (see the next patch).

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Create Text(En|De)coderBase

Separating common logic from main-thread specific parts.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Implement StringEncoding API objects in Workers

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 666158 [details] [diff] [review]
Part 1: Avoid reinterpret_cast in Codegen

This patch caused test failures...

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Create Text(En|De)coderBase

Swapped the inherit order so that reinterpret_cast works correctly.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Implement StringEncoding API objects in Workers

Same as the above.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Implement StringEncoding API objects in Workers

Rebased to tip

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!)]

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]]

Attachment: Part 1: Create Text(En|De)coderBase. r=bent

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Implement StringEncoding API objects in Workers

Rebased to tip and resolved review comments.

Comment 12

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=563a7c05a328

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Create Text(En|De)coderBase. r=bent

Trimmed an extra comment header.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: 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!)]

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]]

(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 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/905b2cfe26ae
https://hg.mozilla.org/mozilla-central/rev/ac143af3f698

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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]]

You can use FileReaderSync in workers to work around.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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)]

(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]]

(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]

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]

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)]

(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] (he/him)]

(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]]

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] (he/him)]

: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]]

Looks good to me. (Except for the last hunk in the second patch. It doesn't look like it belongs to this patch.)

Comment 30

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/08e95442bc65
https://hg.mozilla.org/releases/mozilla-b2g18/rev/34a84093e26a

Comment 31

[Masatoshi Kimura [:emk]]

Backed out because of test failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/563c58c449c6

Comment 32

[Masatoshi Kimura [:emk]]

Relanded.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/44ccc86fa232
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e94004b134a3

Comment 33

[Mihai Morar, (:MihaiMorar)]

Is this related for Desktop Firefox? If it does, how can QA verify the fix?

Comment 34

[Masatoshi Kimura [:emk]]

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]

Can we land this in v1.0.1 as per bug flags?

Comment 36

[Masatoshi Kimura [:emk]]

Yes.

Comment 37

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e87652d46979
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ec1a3c58695f

Comment 38

[Jean-Yves Perrier [:teoli]]

I've added a line here:
https://developer.mozilla.org/en-US/docs/Web/Guide/Needs_categorization/Functions_available_to_workers
and in:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/20
and updated the compat table in
https://developer.mozilla.org/en-US/docs/Web/API/TextDecoder
https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder

Comment 39

[Andrew Sutherland [:asuth] (he/him)]

: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]]

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]]

I added a Firefox OS column in compat tables of all related articles.