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

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.

And the try result is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b76df3bc687

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

Try again.

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

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

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

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.

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

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

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

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!

Conflcts resolved. I'll trigger Try again later.

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

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

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

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

Already landed in m-c, so close it. https://hg.mozilla.org/mozilla-central/rev/7fb0e0d67e9b