Last Comment Bug 707659 - Hook up WebSMS to new WebAPI permission manager
: Hook up WebSMS to new WebAPI permission manager
Status: RESOLVED FIXED
[LOE:S]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla17
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on: 707625 760199
Blocks: 774716 784617 websms 785867
  Show dependency treegraph
 
Reported: 2011-12-05 06:53 PST by Justin Lebar (not reading bugmail)
Modified: 2012-08-27 07:13 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Part 1: DOM implementation (5.69 KB, patch)
2012-08-15 02:34 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 1: DOM implementation : V2 (8.11 KB, patch)
2012-08-15 03:27 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: fix test scripts : WIP (4.91 KB, patch)
2012-08-15 03:31 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: fix test scripts : V2 (9.36 KB, patch)
2012-08-15 05:14 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 1: DOM implementation : V3 (7.83 KB, patch)
2012-08-15 23:47 PDT, Vicamo Yang [:vicamo][:vyang]
justin.lebar+bug: review+
Details | Diff | Review
Part 2: fix test scripts : V3 (8.62 KB, patch)
2012-08-15 23:54 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Review
Part 1: DOM implementation : V4 (8.37 KB, patch)
2012-08-17 04:10 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: fix test scripts : V4 (9.20 KB, patch)
2012-08-17 04:11 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
[DO NOT SUBMIT] debug (8.03 KB, patch)
2012-08-17 04:13 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Emulator Mochitest adb logcat (23.58 KB, text/plain)
2012-08-17 04:14 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details
Part 1: DOM implementation : V5 (8.34 KB, patch)
2012-08-21 21:14 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: fix test scripts : V5 (9.16 KB, patch)
2012-08-21 21:16 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: fix test scripts : V6 (9.29 KB, patch)
2012-08-22 01:24 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2011-12-05 06:53:06 PST
WebSMS's permissions are currently a hack.  But once we have the proper permission manager, we can fix this.
Comment 1 Vicamo Yang [:vicamo][:vyang] 2012-08-15 02:34:00 PDT
Created attachment 652045 [details] [diff] [review]
Part 1: DOM implementation
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-08-15 03:27:31 PDT
Created attachment 652057 [details] [diff] [review]
Part 1: DOM implementation : V2

remove perf "dom.sms.whitelist" from modules/libpref/src/init/all.js as well.
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-08-15 03:31:38 PDT
Created attachment 652059 [details] [diff] [review]
Part 2: fix test scripts : WIP

test_sms_basics.html restores original whitelist preference setting, but there doesn't seem to be a corresponding API in permission manager. There is no 'isGranted' or 'tryAskPermission' or a similar one. And, is it important at all?
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-08-15 05:14:28 PDT
Created attachment 652071 [details] [diff] [review]
Part 2: fix test scripts : V2

* Fix marionette test cases as well
* mochitest is in https://tbpl.mozilla.org/?tree=Try&rev=d7efa8ccec8b
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-08-15 20:26:09 PDT
Comment on attachment 652071 [details] [diff] [review]
Part 2: fix test scripts : V2

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

::: dom/sms/tests/marionette/test_incoming.js
@@ +31,5 @@
>    ok(message.timestamp instanceof Date);
> +  // Comparing timestamp of two emulator instances might be meaningless.
> +  // Just try to ensure the timestamp attribute was assigned with an
> +  // appropriate value.
> +  ok(Math.abs(message.timestamp.getTime() - now) < 5 * 60 * 1000);

This part was previously open as bug 760199. This test is not running with two emulator instances. I'll remove it. See also bug 760199 comment #5.
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-08-15 23:47:30 PDT
Created attachment 652336 [details] [diff] [review]
Part 1: DOM implementation : V3

Update patch for hg
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-08-15 23:54:44 PDT
Created attachment 652338 [details] [diff] [review]
Part 2: fix test scripts : V3

* update patch for hg
* remove changes in test_incoming.js that belongs to bug 760199
* ignore restoring sms permission in test_sms_basics.html and explicitly remove it instead.
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-08-15 23:55:43 PDT
trying https://tbpl.mozilla.org/?tree=Try&rev=b87daa7d2820
Comment 9 Justin Lebar (not reading bugmail) 2012-08-16 10:02:33 PDT
Comment on attachment 652336 [details] [diff] [review]
Part 1: DOM implementation : V3

> +/* static */already_AddRefed<SmsManager>
> +SmsManager::CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow)

Hey, I've seen this code before...

We may want to abstract this a bit sometime, but this is fine for now.  :)

Please add the check for the dom.sms.enabled pref into CheckPermissionAndCreateInstance, though.
Comment 10 Philipp von Weitershausen [:philikon] 2012-08-16 15:15:31 PDT
Comment on attachment 652338 [details] [diff] [review]
Part 2: fix test scripts : V3

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

Thanks, Vicamo!

::: testing/marionette/client/marionette/marionette_test.py
@@ +67,3 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
> +let pm = Components.classes["@mozilla.org/permissionmanager;1"]
> +                   .getService(Components.interfaces.nsIPermissionManager);

Services.perms
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-08-17 03:58:37 PDT
(In reply to Justin Lebar [:jlebar] from comment #9)
> Please add the check for the dom.sms.enabled pref into
> CheckPermissionAndCreateInstance, though.

Wow, that's funny. The test script doesn't work at all :(

In dom/sms/tests/test_sms_basics.html, function checkSmsEnabled():

> // WebSms is disabled on all platforms except Android for the moment.
> if (navigator.appVersion.indexOf("Android") == -1) {
>   checkSmsDisabled();
>   return;
> }

The appVersion string got in B2G emulator is "5.0 ()", so it checks whether SMS is disabled throughout the test script.

With above lines removed, there are still errors in the test result. This time, `Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)` always returns true and breaks several test cases. Then I found:

> nsresult
> Preferences::GetBool(const char* aPref, bool* aResult);

Well, the function prototype is totally different at all.
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-08-17 04:10:10 PDT
Created attachment 652728 [details] [diff] [review]
Part 1: DOM implementation : V4

address Justin's review in comment #9
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-08-17 04:11:18 PDT
Created attachment 652729 [details] [diff] [review]
Part 2: fix test scripts : V4

Address Philipp's review opinions in comment #10
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-08-17 04:13:19 PDT
Created attachment 652730 [details] [diff] [review]
[DO NOT SUBMIT] debug
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-08-17 04:14:21 PDT
Created attachment 652731 [details]
Emulator Mochitest adb logcat
Comment 16 Vicamo Yang [:vicamo][:vyang] 2012-08-17 04:25:05 PDT
Comment on attachment 652731 [details]
Emulator Mochitest adb logcat

> ...
>I/GeckoDump(  424): TEST-PASS | unknown test url | SmsCursor should be prefixed
>I/GeckoDump(  424): TEST-PASS | unknown test url | SmsCursor should be prefixed
>I/Navigator(  393): PREF_SetBoolPref(dom.sms.enabled, false, false)
>I/Navigator(  437): PREF_SetBoolPref(dom.sms.enabled, false, true)

Why are there two `PREF_SetBoolPref` called?

>I/Navigator(  393): PREF_GetBoolPref(dom.sms.enabled, 0xbedb3898, false)
>I/GeckoDump(  424): TEST-PASS | unknown test url | dom.sms.enabled - false should equal false

TEST-PASS because SpecialPowers gets correct modified value.

>I/GeckoDump(  424): TEST-PASS | unknown test url | Checking [false, false, disabled]
>I/GeckoDump(  424): TEST-PASS | unknown test url | navigator.mozSms should exist
>I/Navigator(  424): Navigator::GetMozSms - mSmsManager is null
>I/Navigator(  424): Navigator::GetMozSms - window got
>I/Navigator(  424): PREF_GetBoolPref(dom.sms.enabled, 0xbeac631f, false)
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - enabled

In native code, we get an unmodified dom.sms.enabled value and may fail following tests. Why?

>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - document got
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - principal got
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - permMgr got
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - permission removed
>I/GeckoDump(  424): TEST-PASS | unknown test url | navigator.mozSms should return null - null should equal null

TEST-PASS here, but actually we get null SmsManager for no access, not SMS is disabled as expected.
Comment 17 Justin Lebar (not reading bugmail) 2012-08-17 09:38:09 PDT
> Wow, that's funny. The test script doesn't work at all :(

Awesome.

> `Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)` always returns true and breaks 
> several test cases. Then I found:
>
> nsresult
> Preferences::GetBool(const char* aPref, bool* aResult);
>
> Well, the function prototype is totally different at all.

That function has an override, though:

>  static bool GetBool(const char* aPref, bool aDefault = false)

See modules/libpref/public/Preferences.h.

> Why are there two `PREF_SetBoolPref` called?

I don't know.  You could stick a breakpoint there and see, if you're curious.  (gdb) call DumpJSStack() will show you the JS caller, if there is one on the stack.

> In native code, we get an unmodified dom.sms.enabled value and may fail following tests. Why?

I don't know what you mean here.  I'm happy to try to answer if you can clarify.
Comment 18 Vicamo Yang [:vicamo][:vyang] 2012-08-21 21:01:15 PDT
After a weekend, I got another problem that Mochitest runs test cases in OOP and it always hangs in:

> PContentChild::SendPSmsConstructor(PSmsChild* actor) {
>   ...
>   bool __sendok = (mChannel).Send(__msg);
>   ...
> }

I think I'd better give up for now to get this issue solved. I'll upload another revision for part 2 to keep "unittest.expectedFailure" mark. Any suggestions?
Comment 19 Vicamo Yang [:vicamo][:vyang] 2012-08-21 21:14:57 PDT
Created attachment 654070 [details] [diff] [review]
Part 1: DOM implementation : V5

rebase
Comment 20 Vicamo Yang [:vicamo][:vyang] 2012-08-21 21:16:34 PDT
Created attachment 654072 [details] [diff] [review]
Part 2: fix test scripts : V5

add "unittest.expectedFailure" annotation back
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-08-21 21:18:04 PDT
try again: https://tbpl.mozilla.org/?tree=Try&rev=2c8ac9a7cc45
Comment 22 Vicamo Yang [:vicamo][:vyang] 2012-08-22 01:13:08 PDT
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> try again: https://tbpl.mozilla.org/?tree=Try&rev=2c8ac9a7cc45

Still breaks mochitests of platforms other than Android :(
I opened a new bug 784617 for this and will update patches later.
Comment 23 Vicamo Yang [:vicamo][:vyang] 2012-08-22 01:24:08 PDT
Created attachment 654131 [details] [diff] [review]
Part 2: fix test scripts : V6

only checkSmsEnabled in Android build
Comment 24 Vicamo Yang [:vicamo][:vyang] 2012-08-22 01:24:37 PDT
try yet again: https://tbpl.mozilla.org/?tree=Try&rev=021e574fe29d
Comment 27 :Ehsan Akhgari (out sick) 2012-08-23 08:56:26 PDT
This bug added new NSPR types.  Please avoid doing that!  (See bug 579517 for more info).  Thanks!

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