816900 - [Permission] askPermission() from PermissionPromptHelpter.jsm calls expandPermissions() but no meaning.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

In askPermission(), it use |access="readwrite"| to do expandPermissions(), this returns an arry as [contacts-read, contacts-write and contacts-create].

But askPermission() just test a permission combined by |msg.type + "-" + msg.access| (ex:contacts-write). So it will take this single permission be checked three times (line 71).

Comment 1

[Benjamin Chen [:bechen]]

Attachment: quick fix

Comment 2

[Gregor Wagner [:gwagner]]

Comment on attachment 697808 [details] [diff] [review]
quick fix

I think you can also hoist the first lines out of the loop.
Thanks!

Comment 3

[Benjamin Chen [:bechen]]

Attachment: quick fix

Comment 4

[Benjamin Chen [:bechen]]

Attachment: 1. Use |expandedPermNames[idx]| instead |msg.type + "-" + msg.access|. 2. Add |write, create| permissions in test_sim_contacts.js

version 2:
fix marionette test fail "ERROR - TEST-UNEXPECTED-FAIL | test_sim_contacts.js | ScriptTimeoutException: timed out"

Comment 5

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 699064 [details] [diff] [review]
1. Use |expandedPermNames[idx]| instead |msg.type + "-" + msg.access|. 2. Add |write, create| permissions in test_sim_contacts.js

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

::: dom/contacts/tests/marionette/test_sim_contacts.js
@@ +4,5 @@
>  MARIONETTE_TIMEOUT = 30000;
>  
>  SpecialPowers.addPermission("contacts-read", true, document);
> +// TODO: see bug 804623, We are preventing "read" operations
> +// even if just "write" has been set to DENY_ACTION

nit: place a period at the end of a line.

@@ +5,5 @@
>  
>  SpecialPowers.addPermission("contacts-read", true, document);
> +// TODO: see bug 804623, We are preventing "read" operations
> +// even if just "write" has been set to DENY_ACTION
> +// bug 816900: Because PermissionPromptHelper.jsm will expand the contacts-read

nit: s/bug/Bug

@@ +7,5 @@
> +// TODO: see bug 804623, We are preventing "read" operations
> +// even if just "write" has been set to DENY_ACTION
> +// bug 816900: Because PermissionPromptHelper.jsm will expand the contacts-read
> +// to contacts-read, contacts-write, contacts-create, 
> +// we add the other two permissions here 

nit: white spaces & don't forget a period. Please wrap the sentences at the 80th character.

@@ +14,1 @@
>  

Remember to remove those permissions in cleanUp() function.

Comment 6

[Benjamin Chen [:bechen]]

Attachment: 1. Use |expandedPermNames[idx]| instead |msg.type + "-" + msg.access|. 2. Add |write, create| permissions in test_sim_contacts.js

Comment 7

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 699544 [details] [diff] [review]
1. Use |expandedPermNames[idx]| instead |msg.type + "-" + msg.access|. 2. Add |write, create| permissions in test_sim_contacts.js

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

Nice! Thanks, but please take care of the nits below.
r=me for the part of test_sim_contacts.js.

::: dom/contacts/tests/marionette/test_sim_contacts.js
@@ +6,5 @@
>  SpecialPowers.addPermission("contacts-read", true, document);
> +// TODO: see bug 804623, We are preventing "read" operations
> +// even if just "write" has been set to DENY_ACTION.
> +// Bug 816900: Because PermissionPromptHelper.jsm will expand the contacts-read
> +// to contacts-read, contacts-write, contacts-create, 

nit: remove the white space at the end of the line.

@@ +7,5 @@
> +// TODO: see bug 804623, We are preventing "read" operations
> +// even if just "write" has been set to DENY_ACTION.
> +// Bug 816900: Because PermissionPromptHelper.jsm will expand the contacts-read
> +// to contacts-read, contacts-write, contacts-create, 
> +// we add the other two permissions here 

nit: remove the white space at the end of the line & place a period.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: 1. Use |expandedPermNames[idx]| instead |msg.type + "-" + msg.access|. 2. Add |write, create| permissions in test_sim_contacts.js

r+ after addressing comment 7.

Comment 9

[Benjamin Chen [:bechen]]

try server
https://tbpl.mozilla.org/?tree=Try&rev=aaf23490f063

Comment 10

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d08a08f79c3

Please remember to type the bug number and the r=foo in the commit message next time. ;)

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d08a08f79c3

Comment 12

[Gene Lian (I already quit Mozilla)]

Should we further ask this to be checked in into the b2g18 branch?

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Gene Lian [:gene] from comment #12)
> Should we further ask this to be checked in into the b2g18 branch?

It might be to late now but you can ask for approval.

Comment 14

[Gene Lian (I already quit Mozilla)]

Hi Benjamin, it's up to you to decide if we need to check in this into b2g18. If you want, please ask for approval. Thanks!

Comment 15

[Jason Smith [:jsmith]]

(In reply to Gene Lian [:gene] from comment #14)
> Hi Benjamin, it's up to you to decide if we need to check in this into
> b2g18. If you want, please ask for approval. Thanks!

FYI - Please read https://groups.google.com/forum/#!topic/mozilla.dev.b2g/RhE99AtyTPE. Only tef+ bugs now can be checked into b2g18.