Masatoshi Kimura [:emk] Assignee
	Description • 12 years ago

This API would be useful for Workers. It should be easy because nsIUnicode(En|De)coder and nsICharsetConverterManager is thread-safe.

	Masatoshi Kimura [:emk] Assignee
	Comment 1 • 12 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 2 • 12 years ago

Separating common logic from main-thread specific parts.

	Masatoshi Kimura [:emk] Assignee
	Comment 4 • 12 years ago

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

This patch caused test failures...

	Masatoshi Kimura [:emk] Assignee
	Comment 5 • 12 years ago

Swapped the inherit order so that reinterpret_cast works correctly.

	Masatoshi Kimura [:emk] Assignee
	Comment 6 • 12 years ago

Same as the above.

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 12 years ago

Rebased to tip

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 8 • 12 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 9 • 12 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 11 • 12 years ago

Rebased to tip and resolved review comments.

	Masatoshi Kimura [:emk] Assignee
	Comment 12 • 12 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 13 • 12 years ago

Trimmed an extra comment header.

	Masatoshi Kimura [:emk] Assignee
	Comment 14 • 12 years ago

Fixed a bitrot by bug 820544.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 15 • 12 years ago

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?

	Masatoshi Kimura [:emk] Assignee
	Comment 16 • 12 years ago

(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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 17 • 12 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 18 • 11 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 19 • 11 years ago

You can use FileReaderSync in workers to work around.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 20 • 11 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 21 • 11 years ago

(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. :)

	Alex Keybl [:akeybl]
	Comment 22 • 11 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 11 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 24 • 11 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 25 • 11 years ago

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

	Andrew Sutherland [:asuth] (he/him)
	Comment 26 • 11 years ago

(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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

This doesn't apply cleanly to the b2g18 branch. Please post branch-specific patches (and set checkin-needed again when ready).

	Andrew Sutherland [:asuth] (he/him)
	Comment 28 • 11 years ago

: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

	Masatoshi Kimura [:emk] Assignee
	Comment 29 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 30 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 31 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 32 • 11 years ago

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

	Mihai Morar, (:MihaiMorar)
	Comment 33 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 34 • 11 years ago

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.

	Daniel Coloma:dcoloma
	Comment 35 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 36 • 11 years ago

Yes.

	Andrew Sutherland [:asuth] (he/him)
	Comment 37 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 38 • 11 years ago

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

	Andrew Sutherland [:asuth] (he/him)
	Comment 39 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 40 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 41 • 11 years ago

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