995844 - test_dsds_mobile_data_connection.js retrieve mozMobileConnection from a wrong frame

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ This bug was initially created as a clone of Bug #995107 +++

|let connections = window.navigator.mozMobileConnections;|
http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/test_dsds_mobile_data_connection.js#9

However, when "head.js" is included, we open an iframe and run mobileconnection tests in it instead of current window.  Bug 995107 introduced a new global variable "workingFrame" for this.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

Comment 2

[Jessica Jong [:jessica] (inactive)]

Thank you Vicamo.

Do we need to indicate the working frame to use for cases like [1][2][3], where we get mozMobileConnection for a specific service id?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#285
[2] http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#410
[3] http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#470

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> Thank you Vicamo.
> 
> Do we need to indicate the working frame to use for cases like [1][2][3],
> where we get mozMobileConnection for a specific service id?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/
> marionette/head.js#285
> [2]
> http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/
> marionette/head.js#410
> [3]
> http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/
> marionette/head.js#470

Thank you for catching these.  Sure, I should correct them all at once. :)

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v2

1) use 'workingFrame' whenever necessary,
2) use getNumOfRadioInterfaces instead.

Mnw only: https://tbpl.mozilla.org/?tree=Try&rev=b7fac81d99d6

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8405998 [details] [diff] [review]
patch : v2

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

This patch looks good to me, but I have one question.
Do we need to retrieve mozSettings from 'workingFrame' [1][2] as well?

Thank you.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#96
[2] http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#123

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> Review of attachment 8405998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks good to me, but I have one question.
> Do we need to retrieve mozSettings from 'workingFrame' [1][2] as well?
> 
> Thank you.

The first time when Jessica raised this question, I thought mozSettings is unrelated to mozMobileConnection.  However, now I suddenly realize that mozSettings related permissions are also only installed in that workingFrame.  So, yes, I should have corrected accesses to mozSettings as well.  Thank you :)

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v3

Address previous comment.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8407527 [details] [diff] [review]
patch : v3

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

Thank you :)

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/36e61fd7ed89

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36e61fd7ed89