Last Comment Bug 761732 - B2G RIL: rename {handle|send}DOMMessage in ril_worker.js
: B2G RIL: rename {handle|send}DOMMessage in ril_worker.js
Status: RESOLVED FIXED
[good first bug][lang=js][mentor=vica...
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla25
Assigned To: Jessica Jong [:jessica]
:
: Andrew Overholt [:overholt]
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 899484
  Show dependency treegraph
 
Reported: 2012-06-05 11:53 PDT by Philipp von Weitershausen [:philikon]
Modified: 2013-07-30 02:02 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch(v1) (27.92 KB, patch)
2012-06-16 11:15 PDT, Jignesh Kakadiya [:jhk]
philipp: feedback+
Details | Diff | Splinter Review
rename {handle|send}DOMMessage in ril_worker.js patch (47.25 KB, patch)
2013-07-19 01:25 PDT, Jessica Jong [:jessica]
vicamo: review+
Details | Diff | Splinter Review
rename {handle|send}DOMMessage in ril_worker.js (47.27 KB, patch)
2013-07-19 02:04 PDT, Jessica Jong [:jessica]
jjong: review+
Details | Diff | Splinter Review
rename {handle|send}DOMMessage in ril_worker.js patch (47.83 KB, patch)
2013-07-23 00:40 PDT, Jessica Jong [:jessica]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-06-05 11:53:06 PDT
We don't really notify the DOM directly from ril_worker.js. With e10s we don't even notify the *same process* that the DOM lives in. Suggested names:

* {handle|send}ChromeMessage
* {handle|send}MainthreadMessage
* ...
Comment 1 Jignesh Kakadiya [:jhk] 2012-06-08 01:41:02 PDT
> * {handle|send}ChromeMessage
> * {handle|send}MainthreadMessage
> * ...

Can you please which name should be preferred here.
Comment 2 Philipp von Weitershausen [:philikon] 2012-06-16 10:52:20 PDT
(In reply to Jignesh Kakadiya [:jhk] from comment #1)
> > * {handle|send}ChromeMessage
> > * {handle|send}MainthreadMessage
> > * ...
> 
> Can you please which name should be preferred here.

Sure, I'll toss a coin. ;) sendChromeMessage is shorter, so let's go with that?
Comment 3 Jignesh Kakadiya [:jhk] 2012-06-16 11:15:12 PDT
Created attachment 633832 [details] [diff] [review]
Patch(v1)
Comment 4 Philipp von Weitershausen [:philikon] 2012-07-05 16:54:21 PDT
Comment on attachment 633832 [details] [diff] [review]
Patch(v1)

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

Great, thanks! We need to find a good moment to land this without breaking any of the other patches that are about to land. Would you mind keeping this patch up to date until we can find such a moment (note: unfortunately since this isn't a blocker, it may very well be that this moment will not happen very soon.)
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2013-04-04 11:37:10 PDT
Philikin's no longer involved with b2g. Who can provide some forward direction here?
Comment 6 Jignesh Kakadiya [:jhk] 2013-05-19 21:58:33 PDT
Keeping it open for all. If some one would like to work on it :)
Comment 7 Jessica Jong [:jessica] 2013-07-19 01:25:41 PDT
Created attachment 778347 [details] [diff] [review]
rename {handle|send}DOMMessage in ril_worker.js patch
Comment 8 Vicamo Yang [:vicamo][:vyang] 2013-07-19 01:38:10 PDT
Comment on attachment 778347 [details] [diff] [review]
rename {handle|send}DOMMessage in ril_worker.js patch

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

Nice patch! r=me with nits addressed. :)

::: dom/system/gonk/ril_worker.js
@@ +2446,5 @@
>          if (this.IMEI == null) {
>            this.getIMEI({mmi: true});
>            return;
>          }
> +        // If we already had the device's IMEI, we just send it to chrome

nit: period at line end.

@@ +4452,5 @@
>     *
>     * @param message
>     *        Object containing the message. Messages are supposed
>     */
> +  handleChromeMessage: function handleMessage(message) {

nit: s/handleMessage/handleChromeMessage/ as well.
Comment 9 Jessica Jong [:jessica] 2013-07-19 02:04:51 PDT
Created attachment 778360 [details] [diff] [review]
rename {handle|send}DOMMessage in ril_worker.js

Two changes made to address comments in Comment 8:

> +        // If we already had the device's IMEI, we just send it to chrome
added period at end of line

> +  handleChromeMessage: function handleMessage(message) {
replaced handleMessage with handleChromeMessage
Comment 10 Jessica Jong [:jessica] 2013-07-23 00:40:24 PDT
Created attachment 779646 [details] [diff] [review]
rename {handle|send}DOMMessage in ril_worker.js patch

rebased.

try results:
https://tbpl.mozilla.org/?tree=Try&rev=4f200a1e3a3c
Comment 11 Vicamo Yang [:vicamo][:vyang] 2013-07-23 01:30:31 PDT
https://hg.mozilla.org/projects/birch/rev/7b9a998bde77
Comment 12 Ed Morley [:emorley] 2013-07-23 05:58:51 PDT
https://hg.mozilla.org/mozilla-central/rev/7b9a998bde77

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