Closed
Bug 934125
Opened 11 years ago
Closed 11 years ago
B2G RIL: remove redundant js function name from object method definition
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
Attachments
(5 files, 7 obsolete files)
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(...) {}
Assignee | ||
Comment 1•11 years ago
|
||
Remove white space between keyword 'function' and left parenthesis of a unnamed function.
Assignee: nobody → vyang
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Correct |RIL[FOO] = function FOO() {...}| as well.
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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
Attachment #826316 -
Flags: feedback?(allstars.chh)
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Comment 8•11 years ago
|
||
Sorry for the delay. I'll take a look at least by today. Thanks!
Comment 9•11 years ago
|
||
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 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #826314 -
Attachment is obsolete: true
Attachment #8347132 -
Flags: review?(gene.lian)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #826315 -
Attachment is obsolete: true
Attachment #8347133 -
Flags: review?(gene.lian)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8347134 -
Flags: review?(gene.lian)
Assignee | ||
Updated•11 years ago
|
Attachment #8347133 -
Attachment description: part 2/3: remove redundant method name : v2 → part 2.a/3: remove redundant method name : v2
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #826316 -
Attachment is obsolete: true
Attachment #8347135 -
Flags: review?(gene.lian)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8347136 -
Flags: review?(gene.lian)
Assignee | ||
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8347133 -
Flags: review?(gene.lian) → review+
Updated•11 years ago
|
Attachment #8347134 -
Flags: review?(gene.lian) → review+
Updated•11 years ago
|
Attachment #8347135 -
Flags: review?(gene.lian) → review+
Updated•11 years ago
|
Attachment #8347136 -
Flags: review?(gene.lian) → review+
Comment 18•11 years ago
|
||
Nice! Thank you!
Assignee | ||
Comment 19•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
Rebase.
Attachment #8347133 -
Attachment is obsolete: true
Attachment #8358189 -
Flags: review+
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8347134 -
Attachment is obsolete: true
Attachment #8358190 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #8347135 -
Attachment is obsolete: true
Attachment #8358191 -
Flags: review+
Assignee | ||
Comment 23•11 years ago
|
||
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.
Assignee | ||
Comment 24•11 years ago
|
||
Pass local xpcshell & marionette runs. Full try before push: https://tbpl.mozilla.org/?tree=Try&rev=3dee3a200b47
Assignee | ||
Comment 25•11 years ago
|
||
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
Comment 26•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 27•11 years ago
|
||
Note - either this bug or bug 958773 broke cell data. See bug 959602.
You need to log in
before you can comment on or make changes to this bug.
Description
•