Implement StringEncoding API in Workers

RESOLVED FIXED in Firefox 20

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-complete})

unspecified
mozilla20
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, firefox20 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

22.85 KB, patch
emk
: review+
Details | Diff | Splinter Review
14.56 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 666158 [details] [diff] [review]
Part 1: Avoid reinterpret_cast in Codegen

reinterpret_cast will not work for TextEncoder (see the next patch).
Attachment #666158 - Flags: review?(peterv)
(Assignee)

Comment 2

5 years ago
Created attachment 666159 [details] [diff] [review]
Part 2: Create Text(En|De)coderBase

Separating common logic from main-thread specific parts.
Attachment #666159 - Flags: review?(bent.mozilla)
(Assignee)

Comment 3

5 years ago
Created attachment 666160 [details] [diff] [review]
Part 3: Implement StringEncoding API objects in Workers
Attachment #666160 - Flags: review?(bent.mozilla)
(Assignee)

Comment 4

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

This patch caused test failures...
Attachment #666158 - Attachment is obsolete: true
Attachment #666158 - Flags: review?(peterv)
(Assignee)

Comment 5

5 years ago
Created attachment 666205 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase

Swapped the inherit order so that reinterpret_cast works correctly.
Attachment #666159 - Attachment is obsolete: true
Attachment #666159 - Flags: review?(bent.mozilla)
Attachment #666205 - Flags: review?(bent.mozilla)
(Assignee)

Comment 6

5 years ago
Created attachment 666206 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Same as the above.
Attachment #666160 - Attachment is obsolete: true
Attachment #666160 - Flags: review?(bent.mozilla)
Attachment #666206 - Flags: review?(bent.mozilla)
(Assignee)

Comment 7

5 years ago
Created attachment 667943 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Rebased to tip
Attachment #666206 - Attachment is obsolete: true
Attachment #666206 - Flags: review?(bent.mozilla)
Attachment #667943 - Flags: review?(bent.mozilla)
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.
Attachment #666205 - Flags: review?(bent.mozilla) → review+
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
Attachment #667943 - Flags: review?(bent.mozilla)
(Assignee)

Comment 10

5 years ago
Created attachment 692547 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase. r=bent
Attachment #666205 - Attachment is obsolete: true
Attachment #692547 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #692547 - Attachment description: Create Text(En|De)coderBase. r=bent → Part 1: Create Text(En|De)coderBase. r=bent
(Assignee)

Comment 11

5 years ago
Created attachment 692548 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Rebased to tip and resolved review comments.
Attachment #667943 - Attachment is obsolete: true
Attachment #692548 - Flags: review?(bent.mozilla)
(Assignee)

Comment 12

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=563a7c05a328
(Assignee)

Comment 13

5 years ago
Created attachment 692549 [details] [diff] [review]
Part 1: Create Text(En|De)coderBase. r=bent

Trimmed an extra comment header.
Attachment #692547 - Attachment is obsolete: true
Attachment #692549 - Flags: review+
(Assignee)

Comment 14

5 years ago
Created attachment 693855 [details] [diff] [review]
Part 2: Implement StringEncoding API objects in Workers

Fixed a bitrot by bug 820544.
Attachment #692548 - Attachment is obsolete: true
Attachment #692548 - Flags: review?(bent.mozilla)
Attachment #693855 - Flags: review?(bent.mozilla)
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?
Attachment #693855 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 16

5 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
Status: NEW → ASSIGNED
Flags: in-testsuite+
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/905b2cfe26ae
https://hg.mozilla.org/mozilla-central/rev/ac143af3f698
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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.
blocking-b2g: --- → tef?
(Assignee)

Comment 19

5 years ago
You can use FileReaderSync in workers to work around.
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
blocking-b2g: tef? → -
(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. :)
blocking-b2g: - → tef?
Blocks: 814257
(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?
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.
Flags: needinfo?(21)
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
blocking-b2g: tef? → -
(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.
Flags: needinfo?(21)
(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
blocking-b2g: - → tef?
blocking-b2g: tef? → tef+

Updated

5 years ago
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
Blocks: 852226
Keywords: checkin-needed
This doesn't apply cleanly to the b2g18 branch. Please post branch-specific patches (and set checkin-needed again when ready).
status-firefox20: --- → fixed
Keywords: checkin-needed
: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
Flags: needinfo?(VYV03354)
(Assignee)

Comment 29

5 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.)
Flags: needinfo?(VYV03354)
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/08e95442bc65
https://hg.mozilla.org/releases/mozilla-b2g18/rev/34a84093e26a
status-b2g18: affected → fixed
(Assignee)

Comment 31

5 years ago
Backed out because of test failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/563c58c449c6
status-b2g18: fixed → affected
(Assignee)

Comment 32

5 years ago
Relanded.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/44ccc86fa232
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e94004b134a3
status-b2g18: affected → fixed
Is this related for Desktop Firefox? If it does, how can QA verify the fix?
Flags: needinfo?(VYV03354)
(Assignee)

Comment 34

5 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.
Flags: needinfo?(VYV03354)
Can we land this in v1.0.1 as per bug flags?
Flags: needinfo?(VYV03354)
(Assignee)

Comment 36

4 years ago
Yes.
Flags: needinfo?(VYV03354)
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e87652d46979
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ec1a3c58695f
status-b2g18-v1.0.1: affected → fixed
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
Keywords: dev-doc-needed → dev-doc-complete
: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?
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 :-)
Keywords: dev-doc-complete → dev-doc-needed
I added a Firefox OS column in compat tables of all related articles.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.