905109 - B2G RIL: Move Buf in ril_worker.js to another WorkerModule

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In Bug 872421, the module loader for worker is landed,
we should try to move the Buf object inside dom/system/gonk/ril_worker to another Worker module called Buf.jsm.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Hi, Yoric
Can you help to provide some feedback for the usage of module loader here?

Thanks

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #0)
> In Bug 872421, the module loader for worker is landed,
> we should try to move the Buf object inside dom/system/gonk/ril_worker to
> another Worker module called Buf.jsm.

Would ril_worker_buf.js be a better name?

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #0)
> > In Bug 872421, the module loader for worker is landed,
> > we should try to move the Buf object inside dom/system/gonk/ril_worker to
> > another Worker module called Buf.jsm.
> 
> Would ril_worker_buf.js be a better name?

I plan to reuse it in NFC, so ril prefix might not be appropriate here.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 790147 [details] [diff] [review]
Patch

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

Generally, that looks good. I have a few suggestions, though.

I would suggest giving a name in .js rather than .jsm.
Also, I would suggest to either:
- put the file in resource://gre/modules/ril_worker/, if it is not meant to be shared with other workers; or
- put the file in resource://gre/modules/workers otherwise, and give it a more descriptive name.

::: dom/system/gonk/Buf.jsm
@@ +20,5 @@
> + * The outgoing buffer is to prepare outgoing parcels. The index is reset
> + * every time a parcel is sent.
> + */
> +
> +self.incomingBufferLength = 1024;

I am not sure why you publish this in |self|, but I suspect this is a practice we want to avoid, as |self| is the global |self| object of workers.
Rather, I would publish it as part of |module.exports| or, if you want it world-readable/writable, as part of an object |module.exports.Config|.
Similar remarks for the other values you put in |self|.

@@ +22,5 @@
> + */
> +
> +self.incomingBufferLength = 1024;
> +self.outgoingBufferLength = 1024;
> +

Nit: since Buf.init() is published largely for testing purposes, you probably want to make this a comment.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Add code quality test for Buf.jsm

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 790629 [details] [diff] [review]
Add code quality test for Buf.jsm

Oh great,
thanks.

I am going to rename it to worker_buf.js, so I'll update your patch here.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Move Buf to worker_buf.js

Hi, Ben and Vicamo

This is a patch for moving Buf object inside ril_worker to another file called worker_buf.js.

I have got a feedback+ from Yoric before, and have addressed his comments from Comment 4.
I plan to ask r? for him, for he's taking PTO now, so now I change r? to Ben.

r? to bent for he's the owner of Worker.
r? to vicamo for he's the owner of RIL.

Thanks

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Update Makefile

Hi Gregory
Can you help to review this Makefile change for adding worker_buf.js to modules/workers?

Thanks

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: update test_ril_code_quality

This is the patch provided by Aknow in the first place, I update the filename to worker_buf.js

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 790677 [details] [diff] [review]
Part 3: update test_ril_code_quality

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

Thanks!

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 790676 [details] [diff] [review]
Part 2: Update Makefile

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

The proper way to define JavaScript modules is in moz.build files. e.g.

EXTRA_JS_MODULES += [worker_buf.js']
JS_MODULES_PATH = ''workers'

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 790673 [details] [diff] [review]
Part 1: Move Buf to worker_buf.js

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

::: dom/system/gonk/worker_buf.js
@@ +643,5 @@
> +  startCalOutgoingSize: startCalOutgoingSize,
> +  stopCalOutgoingSize: stopCalOutgoingSize,
> +  growIncomingBuffer: growIncomingBuffer,
> +  growOutgoingBuffer: growOutgoingBuffer,
> +  ensureIncomingAvailable: ensureIncomingAvailable,

growIncomingBuffer, growOutgoingBuffer, ensureIncomingAvailable are currently only referenced in Buf.  Let's keep them hidden from other components until some foreign objects need them.

@@ +645,5 @@
> +  growIncomingBuffer: growIncomingBuffer,
> +  growOutgoingBuffer: growOutgoingBuffer,
> +  ensureIncomingAvailable: ensureIncomingAvailable,
> +  seekIncoming: seekIncoming,
> +  readUint8Unchecked: readUint8Unchecked,

ditto: readUint8Unchecked

@@ +655,5 @@
> +  readString: readString,
> +  readStringList: readStringList,
> +  readStringDelimiter: readStringDelimiter,
> +  readParcelSize: readParcelSize,
> +  ensureOutgoingAvailable: ensureOutgoingAvailable,

ditto: readParcelSize, ensureOutgoingAvailable

@@ +664,5 @@
> +  writeStringList: writeStringList,
> +  writeStringDelimiter: writeStringDelimiter,
> +  writeParcelSize: writeParcelSize,
> +  copyIncomingToOutgoing: copyIncomingToOutgoing,
> +  writeToIncoming: writeToIncoming,

ditto: writeToIncoming

@@ +666,5 @@
> +  writeParcelSize: writeParcelSize,
> +  copyIncomingToOutgoing: copyIncomingToOutgoing,
> +  writeToIncoming: writeToIncoming,
> +  processIncoming: processIncoming,
> +  processParcel: processParcel,

ditto: processParcel

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Gregory Szorc [:gps] from comment #11)
> 
> The proper way to define JavaScript modules is in moz.build files. e.g.
> 
> EXTRA_JS_MODULES += [worker_buf.js']
> JS_MODULES_PATH = ''workers'

Hi, Gregory
In dom/system/gonk/moz.build

There is already EXTRA_JS_MODULES there.
If I add JS_MODULES_PATH, will this apply for all the EXTRA_JS_MODULES? 

Or just for worker_buf.js?


Thanks

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #13)
> (In reply to Gregory Szorc [:gps] from comment #11)
> > 
> > The proper way to define JavaScript modules is in moz.build files. e.g.
> > 
> > EXTRA_JS_MODULES += [worker_buf.js']
> > JS_MODULES_PATH = ''workers'
> 
> Hi, Gregory
> In dom/system/gonk/moz.build
> 
> There is already EXTRA_JS_MODULES there.
> If I add JS_MODULES_PATH, will this apply for all the EXTRA_JS_MODULES? 
> 
Hi, Gregory.

It did move all EXTRA_JS_MODULES to 'workers', can I specify two JS_MODULES_PATH in the same moz.build?
So only worker_buf.js will be 'modules/workers', and other JS_MODULES will be the original path, 'modules'.

Thanks

Comment 15

[Gregory Szorc [:gps]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #14)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #13)
> > (In reply to Gregory Szorc [:gps] from comment #11)
> > > 
> > > The proper way to define JavaScript modules is in moz.build files. e.g.
> > > 
> > > EXTRA_JS_MODULES += [worker_buf.js']
> > > JS_MODULES_PATH = ''workers'
> > 
> > Hi, Gregory
> > In dom/system/gonk/moz.build
> > 
> > There is already EXTRA_JS_MODULES there.
> > If I add JS_MODULES_PATH, will this apply for all the EXTRA_JS_MODULES? 
> > 
> Hi, Gregory.
> 
> It did move all EXTRA_JS_MODULES to 'workers', can I specify two
> JS_MODULES_PATH in the same moz.build?
> So only worker_buf.js will be 'modules/workers', and other JS_MODULES will
> be the original path, 'modules'.

Not currently, sadly. We have bug 887958 open to make this better. Until then, you'll need to use INSTALL_TARGETS in Makefile.in.

I'll re-review it.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Gregory Szorc [:gps] from comment #15)
> Not currently, sadly. We have bug 887958 open to make this better. Until
> then, you'll need to use INSTALL_TARGETS in Makefile.in.
> 
Hi, Gregory
Thanks for the info,
I'll add a TODO in my this patch.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #16)
> Hi, Gregory
> Thanks for the info,
> I'll add a TODO in my this patch.

Filed a bug Bug 908038 for this and will add this TODO in the part 2 patch.

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Move Buf to worker_buf.js v2

Address Vicamo's comments.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Update Makefile. v2

Add a TODO in the patch.

This patch has been r+ by gps.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 793834 [details] [diff] [review]
Part 1: Move Buf to worker_buf.js v2

This patch is already r+ by Vicamo.

I have asked for r? to Ben for a week, but seems he's busy right now and Yoric is back, so switch the r? back to Yoric.

Comment 21

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

Comment on attachment 793834 [details] [diff] [review]
Part 1: Move Buf to worker_buf.js v2

rs=me for simply moving code into a different file.

If you actually changed any of the code please separate that out into a different patch and request review on it.

Thanks!

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks Ben.

I didn't change the code, just move it to a different file.

Thanks for the review even when you're so busy. :D

Comment 23

[Ken Chang[:kenkai]]

If we rewrite worker in c++, do we need to change this buf function to be a service?

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Move Buf to worker_buf.js v3

rebase, and add r=bent, vicamo

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Ken Chang from comment #23)
> If we rewrite worker in c++, do we need to change this buf function to be a
> service?

yes

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Move Buf to worker_buf.js v4

I found now in worker_buf.js, calling this.foo() will become undefined now,
so replace this.foo() to foo();

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: Add getter for members

expose currentParcelSize and readAvaialable

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

try result 
https://tbpl.mozilla.org/?tree=Try&rev=1ac58692d204

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 794962 [details] [diff] [review]
Part 4: Add getter for members

Ask for r? for Vicamo and Hsinyi.
I hope I chould check-in this today, so who see this first could review it.

If one of you can review this I think is enough.
Thanks

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0282b8289368
https://hg.mozilla.org/integration/b2g-inbound/rev/c262a2751eb1
https://hg.mozilla.org/integration/b2g-inbound/rev/a7d2957f10c0
https://hg.mozilla.org/integration/b2g-inbound/rev/90e65238ec61

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0282b8289368
https://hg.mozilla.org/mozilla-central/rev/c262a2751eb1
https://hg.mozilla.org/mozilla-central/rev/a7d2957f10c0
https://hg.mozilla.org/mozilla-central/rev/90e65238ec61