Closed Bug 905109 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(4 files, 6 obsolete files)

1.75 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1017 bytes, patch
allstars.chh
: review+
Details | Diff | Splinter Review
43.96 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
4.69 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
Hi, Yoric
Can you help to provide some feedback for the usage of module loader here?

Thanks
Attachment #790147 - Flags: feedback?(dteller)
(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?
(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 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.
Attachment #790147 - Flags: feedback?(dteller) → feedback+
Attachment #790629 - Flags: review?(allstars.chh)
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.
Attachment #790629 - Flags: review?(allstars.chh)
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
Attachment #790147 - Attachment is obsolete: true
Attachment #790673 - Flags: review?(vyang)
Attachment #790673 - Flags: review?(bent.mozilla)
Attached patch Part 2: Update Makefile (obsolete) — Splinter Review
Hi Gregory
Can you help to review this Makefile change for adding worker_buf.js to modules/workers?

Thanks
Attachment #790676 - Flags: review?(gps)
This is the patch provided by Aknow in the first place, I update the filename to worker_buf.js
Attachment #790629 - Attachment is obsolete: true
Attachment #790677 - Flags: review?(htsai)
Comment on attachment 790677 [details] [diff] [review]
Part 3: update test_ril_code_quality

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

Thanks!
Attachment #790677 - Flags: review?(htsai) → review+
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'
Attachment #790676 - Flags: review?(gps) → review-
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
Attachment #790673 - Flags: review?(vyang) → review+
(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
(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
(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.
Attachment #790676 - Flags: review- → review+
(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.
(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.
Address Vicamo's comments.
Attachment #790673 - Attachment is obsolete: true
Attachment #790673 - Flags: review?(bent.mozilla)
Add a TODO in the patch.

This patch has been r+ by gps.
Attachment #790676 - Attachment is obsolete: true
Attachment #793835 - Flags: review+
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.
Attachment #793834 - Flags: review?(dteller)
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!
Attachment #793834 - Flags: review?(dteller) → review+
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
If we rewrite worker in c++, do we need to change this buf function to be a service?
rebase, and add r=bent, vicamo
Attachment #793834 - Attachment is obsolete: true
Attachment #794405 - Flags: review+
(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
I found now in worker_buf.js, calling this.foo() will become undefined now,
so replace this.foo() to foo();
Attachment #794405 - Attachment is obsolete: true
Attachment #794961 - Flags: review+
expose currentParcelSize and readAvaialable
Attachment #794962 - Flags: review?(vyang)
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
Attachment #794962 - Flags: review?(htsai)
Attachment #794962 - Flags: review?(htsai) → review+
Blocks: 912442
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: