1123201 - B2G RIL: Pull the BufObject out from ril_worker.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Greg Weng [:snowmantw][:gweng][:λ]]

+++ This bug was initially created as a clone of Bug #1123066 +++

In this bug I want to pull the BufObject out, and discuss if the coding style is adoptable.

Another issue is that I've found that in Gecko people don't use linter tool for JavaScript. So in another experimental patch I try to make ril_worker.js pass JSHint, as Gaia code does. It's helpful since I even fixed other minor bugs and clarify the dependencies by satisfy the requests the linter ask in the patching progress. I would fire another bug with WIP patch to see if we could adopt that for JS files I pull out from ril_worker.js.

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch

Hi, since the Try now is green for the patch, I think it's OK to discuss about the changes I made in this patch:

1. Wrap the definition in an anonymous function to prevent leaking. This is a common pattern to split files from the monolithic JavaScript, since we could include it without worrying about name conflicts, and could hide some internal definitions that would be used only in the scope. For example, the next constructor 'TelephonyRequestEntry' is only for the 'TelephonyRequestQueue', which is unnecessary to expose it.

2. Use 'global' comments clarify the dependencies of the file & component. If we want to include the file rather to use module system to indicate the dependencies semantically, it's helpful to add such comments. It's also good for linting tools like JSHint, which help us to catch many possible errors in Gaia.

3. Since I don't know whether to define it as a module is OK or not, I export the constructor to the 'self', namely the 'global' of the worker. It's still apt to turn to the constructor as a module, if the overhead is acceptable.

Please let me know your concerns, thanks.

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

And the try result is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b76df3bc687

Comment 3

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

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #1)
> Created attachment 8553441 [details] [diff] [review]
> Patch
> 
> Hi, since the Try now is green for the patch, I think it's OK to discuss
> about the changes I made in this patch:
> 
> 1. Wrap the definition in an anonymous function to prevent leaking. This is
> a common pattern to split files from the monolithic JavaScript, since we
> could include it without worrying about name conflicts, and could hide some
> internal definitions that would be used only in the scope. For example, the
> next constructor 'TelephonyRequestEntry' is only for the
> 'TelephonyRequestQueue', which is unnecessary to expose it.
> 
> 2. Use 'global' comments clarify the dependencies of the file & component.
> If we want to include the file rather to use module system to indicate the
> dependencies semantically, it's helpful to add such comments. It's also good
> for linting tools like JSHint, which help us to catch many possible errors
> in Gaia.
> 
> 3. Since I don't know whether to define it as a module is OK or not, I
> export the constructor to the 'self', namely the 'global' of the worker.
> It's still apt to turn to the constructor as a module, if the overhead is
> acceptable.
> 
> Please let me know your concerns, thanks.

Hi Greg,
Were you attaching a patch of patch?  It doesn't show well by bugzilla's diff function.

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Try again

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch

Try again.

Comment 6

[Edgar Chen [:edgar]]

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

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

I'd like to invite Hsinyi and Aknow to have their opinion. :)

Comment 7

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

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

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

Looks pretty good.  Thank you.

::: dom/system/gonk/ril_worker_buf_object.js
@@ +20,5 @@
> +   * In the old version it has 'init' method while we have the constructor.
> +   * This is unnecessary, and since the use cases this files shows that
> +   * people would call 'init' after new the object, it could be merge into
> +   * the constructor.
> +   */

I don't think we need the comment.  The code itself is reasonable and doesn't require any further explanation.

@@ +124,5 @@
> +  // Before we make sure to form it as a module would not add extra
> +  // overhead of module loading, define it in this way rather than
> +  // 'module.exports it as a module component.
> +  exports.BufObject = BufObject;
> +})(self); /* in worker self is the global */

I will use single line comment style with two leading spaces.

})(self);  // in worker self is the global

Comment 8

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

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

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

It looks good to me, thank you.

::: dom/system/gonk/ril_worker_buf_object.js
@@ +21,5 @@
> +   * This is unnecessary, and since the use cases this files shows that
> +   * people would call 'init' after new the object, it could be merge into
> +   * the constructor.
> +   */
> +  var BufObject = function(aContext) {

s/var/let/

@@ +122,5 @@
> +  };
> +
> +  // Before we make sure to form it as a module would not add extra
> +  // overhead of module loading, define it in this way rather than
> +  // 'module.exports it as a module component.

nit:  a redundant "single-quot" or another quote is missing ?

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch rev2

Hi, I've fixed the issues and update the patch.
I don't know who should review this so if it's wrong please tell me.
Also I don't know whether we need a new test file at this stage, since the old tests are still valid and works well. At least at this stage, I don't want to get into too much troubles, and prefer do changes as small as possible. My plan is first to pull out components and important them in the ril_worker.js to keep things work well, and than we could consider to modify the tests to include only the target file, rather than the ril_worker.js.

(Also I need to ask how to land Gecko patch after this get r+...)

Comment 10

[Edgar Chen [:edgar]]

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

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

Looks good to me, just one small question. Please see comment below. Thank you.

::: dom/system/gonk/ril_worker_buf_object.js
@@ +11,5 @@
> + */
> +(function(exports) {
> +
> +  // Set to true in ril_consts.js to see debug messages
> +  let DEBUG = DEBUG_WORKER;

Could we just use the global |DEBUG| in ril_worker.js?

Comment 11

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

Comment on attachment 8555708 [details] [diff] [review]
Patch rev2

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

Thank you for your work.
r=me with comments addressed.

To land the patches
1. You need to push the patches to try server [1] and make sure that all the tests (only selecting b2g related platforms) are passed.
2. Make sure the commit message is properly formatted [2].  You have to specify the reviewer by something like => COMMIT-SUMMARY. r=[reviewer]
3. Paste the link to the result of try in the comment and set "checkin-needed" in the keyword field.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
[2] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

::: dom/system/gonk/ril_worker_buf_object.js
@@ +5,5 @@
> +
> +"use strict";
> +
> +/**
> + * This is a specialized worker buffer for the Parcel protocal.

s/protocal/protocol

@@ +7,5 @@
> +
> +/**
> + * This is a specialized worker buffer for the Parcel protocal.
> + *
> + * NOTE: To prevent including/importing twice, this file would be included

s/would/should

@@ +8,5 @@
> +/**
> + * This is a specialized worker buffer for the Parcel protocal.
> + *
> + * NOTE: To prevent including/importing twice, this file would be included
> + * in a file already included 'ril_consts.js' and 'require.js'.

in a file which already includes 'ril_consts.js' and 'require.js'.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch rev3

I Fixed the issues above and I'm waiting the try server.

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

The result should be here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b967776df4

For Edgar in Comment 10: I don't really understand the relations among these debug flags, so I have no idea how they should be at this time.

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

The Try is green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b967776df4

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch rev3

The same patch, added the review information at the comment.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

And since the try is green (Comment 14), I now set the checking-needed.

Comment 17

[Carsten Book [:Tomcat]]

Hi Greg, this patch failed to apply:

applying bug1123201.patch
patching file dom/system/gonk/ril_worker.js
Hunk #2 FAILED at 76
1 out of 3 hunks FAILED -- saving rejects to file dom/system/gonk/ril_worker.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug1123201.patch

could you take a look? Thanks!

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch rev4

Conflcts resolved. I'll trigger Try again later.

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

Try is green again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f1b77c2e33d

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7fb0e0d67e9b

Comment 21

[Greg Weng [:snowmantw][:gweng][:λ]]

Well, since it's landed, I close the bug.

Comment 22

[[:fabrice] Fabrice Desré]

Gecko bugs are only closed when the code lands on mozilla-central.

Comment 23

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, sorry I've confused it with the process of Gaia. Thanks.

Comment 24

[Edgar Chen [:edgar]]

Already landed in m-c, so close it.

https://hg.mozilla.org/mozilla-central/rev/7fb0e0d67e9b