Currently RIL JS object methods all look like: methodName: function methodName(...) {} It doesn't really mean much and causes trouble when we have a long method name. So, I suggest not to write the method name again and have following form instead: methodName: function(...) {}

Remove white space between keyword 'function' and left parenthesis of a unnamed function.

Correct |RIL[FOO] = function FOO() {...}| as well.

Comment on attachment 826316 [details] [diff] [review] part 3/3 What do you think?

See also https://bugzilla.mozilla.org/show_bug.cgi?id=876397#c89

Sorry for the long wait here. The function name property is needed as defined in Mozilla Coding Style. [1] "In JavaScript, functions should use camelCase but should not capitalize the first letter. *Methods should use the named function expression syntax so that stack traces read better*: doSomething: function MyObject_doSomething(aFoo, aBar) { ... } " However I do agree that Firefox has provided more detail information for the stack now, even for anonymous function. This is introduced in Firefox 17, or Bug 433529, for the Components.stack, it's landed in Bug 876397. So in your original proposal, you say that function name will make the lines longer, maybe we could wrap that into two lines. Like longFuncName : function longFuncName(aFoo) { } Althogh I do agree it looks uglier. Or if the reason is from Comment 5, for better stack trace, maybe we need to ask someone to update the Mozilla Coding Style for Javascript. [1]: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #6) > Or if the reason is from Comment 5, for better stack trace, > maybe we need to ask someone to update the Mozilla Coding Style for > Javascript. http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js Contacts API has adopted this style for a long time. Don't think that's a blocking step for our own refactoring.

Sorry for the delay. I'll take a look at least by today. Thanks!

Comment on attachment 826316 [details] [diff] [review] part 3/3 Review of attachment 826316 [details] [diff] [review]: ----------------------------------------------------------------- It should be OK to go.

Comment on attachment 826316 [details] [diff] [review] part 3/3 Review of attachment 826316 [details] [diff] [review]: ----------------------------------------------------------------- It should be fine to remove function name property for now as the time proves we don't really need it in ril code, and we have better logging now.

Part 1, 2.a, 3.a can be verified by the commands listed in commit summary. You'll also find I exclude files belong to NFC, NetworkStats, etc. Part 2.b and 3.b has to be reviewed by naked eyes and fortunately they're not too long and complicated. Just fix some alignments and indentations.

Comment on attachment 8347132 [details] [diff] [review] part 1/3: s/function \+(/function(/ : v2 Review of attachment 8347132 [details] [diff] [review]: ----------------------------------------------------------------- I didn't go through each single line but I believe this patch exactly does what it does.

Nice! Thank you!

Rebase only. Actually, I used the command in commit summary to re-generate this patch.

Rebase.

Part 3.b/3 has no change. To check the changes, I have a git branch bug-934125 based on b2g-inbound. And: $ git checkout b2g-inbound $ cat `git diff --name-only b2g-inbound bug-934125` | sed -e 's/\s\+/ /g' > orig.txt $ git checkout bug-934125 $ cat `git diff --name-only b2g-inbound bug-934125` | sed -e 's/\s\+/ /g' | diff -Nu - orig.txt | grep -e '^[\+-]' > diff.txt This generates a txt file of slightly more than 3000 lines and contains only all the changes one may want to review.

Pass local xpcshell & marionette runs. Full try before push: https://tbpl.mozilla.org/?tree=Try&rev=3dee3a200b47

https://hg.mozilla.org/integration/b2g-inbound/rev/558444d16f2e https://hg.mozilla.org/integration/b2g-inbound/rev/e6a1ea2dec91 https://hg.mozilla.org/integration/b2g-inbound/rev/2790b437e403 https://hg.mozilla.org/integration/b2g-inbound/rev/e24b5206e319 https://hg.mozilla.org/integration/b2g-inbound/rev/50a48b715419

https://hg.mozilla.org/mozilla-central/rev/558444d16f2e https://hg.mozilla.org/mozilla-central/rev/e6a1ea2dec91 https://hg.mozilla.org/mozilla-central/rev/2790b437e403 https://hg.mozilla.org/mozilla-central/rev/e24b5206e319 https://hg.mozilla.org/mozilla-central/rev/50a48b715419

Note - either this bug or bug 958773 broke cell data. See bug 959602.