B2G RIL: remove redundant js function name from object method definition

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

4.96 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
180.45 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
571.60 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
30.31 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
42.82 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → vyang
Posted patch part 3/3 (obsolete) — Splinter Review
Correct |RIL[FOO] = function FOO() {...}| as well.
Comment on attachment 826316 [details] [diff] [review]
part 3/3

What do you think?
Attachment #826316 - Flags: feedback?(htsai)
Attachment #826316 - Flags: feedback?(gene.lian)
Attachment #826316 - Flags: feedback?(allstars.chh)
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.
Attachment #826316 - Flags: feedback?(gene.lian) → feedback+
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.
Attachment #826316 - Flags: feedback?(htsai) → feedback+
Attachment #826314 - Attachment is obsolete: true
Attachment #8347132 - Flags: review?(gene.lian)
Attachment #826315 - Attachment is obsolete: true
Attachment #8347133 - Flags: review?(gene.lian)
Attachment #8347134 - Flags: review?(gene.lian)
Attachment #8347133 - Attachment description: part 2/3: remove redundant method name : v2 → part 2.a/3: remove redundant method name : v2
Attachment #826316 - Attachment is obsolete: true
Attachment #8347135 - Flags: review?(gene.lian)
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.
Attachment #8347132 - Flags: review?(gene.lian) → review+
Attachment #8347133 - Flags: review?(gene.lian) → review+
Attachment #8347134 - Flags: review?(gene.lian) → review+
Attachment #8347135 - Flags: review?(gene.lian) → review+
Attachment #8347136 - Flags: review?(gene.lian) → review+
Nice! Thank you!
Rebase only.  Actually, I used the command in commit summary to re-generate this patch.
Attachment #8347132 - Attachment is obsolete: true
Attachment #8358188 - Flags: review+
Rebase.
Attachment #8347133 - Attachment is obsolete: true
Attachment #8358189 - Flags: review+
Attachment #8347134 - Attachment is obsolete: true
Attachment #8358190 - Flags: review+
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
Note - either this bug or bug 958773 broke cell data. See bug 959602.
Blocks: 960961
You need to log in before you can comment on or make changes to this bug.