Last Comment Bug 754777 - B2G RIL: Add a Helper to read String delimiters
: B2G RIL: Add a Helper to read String delimiters
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Yoshi Huang[:allstars.chh]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-13 23:38 PDT by Yoshi Huang[:allstars.chh]
Modified: 2012-05-15 06:30 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to readStringDelimiter helper function (7.72 KB, patch)
2012-05-14 00:22 PDT, Yoshi Huang[:allstars.chh]
philipp: feedback+
Details | Diff | Splinter Review
Patch to readStringDelimiter helper function , V2 (7.94 KB, patch)
2012-05-14 19:36 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Splinter Review

Description Yoshi Huang[:allstars.chh] 2012-05-13 23:38:25 PDT
In ril_worker.js,
there are many code snippets like

let delimiter = Buf.readUint16();
if (!(length & 1)) {
  delimiter |= Buf.readUint16();
}
if (DEBUG) {
  if (delimiter != 0) {
    debug("Something's wrong, found string delimiter: " + delimiter);
  }
}


Since it can be simplified as a function,
we should add a helper for this.
Comment 1 Yoshi Huang[:allstars.chh] 2012-05-14 00:22:51 PDT
Created attachment 623601 [details] [diff] [review]
Patch to readStringDelimiter helper function
Comment 2 Philipp von Weitershausen [:philikon] 2012-05-14 12:34:24 PDT
Comment on attachment 623601 [details] [diff] [review]
Patch to readStringDelimiter helper function

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

Yay! But having this be a method on `GsmPDUHelper` doesn't make much sense. Let's put those methods on `Buf`.
Comment 3 Yoshi Huang[:allstars.chh] 2012-05-14 19:36:12 PDT
Created attachment 623914 [details] [diff] [review]
Patch to readStringDelimiter helper function , V2

Hi, Philikon
Thanks for that comment, yes, it makes much sense to add this helper in Buf.

Do I need to add r=philikon in this patch?

thanks
Comment 4 Philipp von Weitershausen [:philikon] 2012-05-14 20:35:05 PDT
(In reply to Yoshi Huang[:yoshi] from comment #3)
> Do I need to add r=philikon in this patch?

In the future this would be helpful, yes. It saves me having to add it manually.

Thanks for the updated patch!
Comment 5 Philipp von Weitershausen [:philikon] 2012-05-14 21:15:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03005c65982
Comment 6 Ed Morley [:emorley] 2012-05-15 06:30:00 PDT
https://hg.mozilla.org/mozilla-central/rev/f03005c65982

Note You need to log in before you can comment on or make changes to this bug.