887880 - SimplePush: Rename prefs to dom.push.*

Status: RESOLVED INVALID

(Core :: DOM: Push Subscriptions, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

As Push is a DOM API, the preferences should be renamed so once it lands on desktop.

Files:
modules/libpref/src/init/all.js
b2g/app/b2g.js
browser/app/profile/firefox.js
mobile/android/app/mobile.js

Comment 1

[Masroor Hasan]

Hi there, I would like to take this on as my first bug :)

Comment 2

[Masroor Hasan]

So, the renaming should basically be:
The pref calls in the .js files should be replaced with calls to dom.push?

I'm a little unsure of what you mean by "..so once it lands on desktop." 

Thanks.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Masroor Hasan from comment #2)
> So, the renaming should basically be:
> The pref calls in the .js files should be replaced with calls to dom.push?
Yes, and the files mentioned in the bug description too.

> 
> I'm a little unsure of what you mean by "..so once it lands on desktop."
Bug 857464. But if you can do it before that and rebase the patches of Bug 857464 to use the new prefs, that would be great!

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Masroor, are you still working on this and do you have any patches?

Comment 5

[Vedad Hadžić [:vedad][:vedadux]]

If Masroor Hasan isn't working on it anymore, could I have a try. I am a newbie when it comes to contribution, so could you please guid me through my first bug patch. Which project should I build for this? B2G maybe?
Thanks in advance.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Vedad, reassigning to you.
You will want to build B2G, but if you see the prefs file and the Push.js/PushService.jsm/PushServiceLauncher.js code, it's mainly about changing a few string literals. So you aren't required to build anything.

Comment 7

[Vedad Hadžić [:vedad][:vedadux]]

Attachment: Patch to bug887880. Changet occurances of services.push.* with dom.push.*

Files that were changed:
modules/libpref/src/init/all.js
b2g/app/b2g.js
mobile/android/app/mobile.js
dom/push/src/Push.js
dom/push/src/PushService.jsm
dom/push/src/PushServiceLauncher.js

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 826900 [details] [diff] [review]
Patch to bug887880. Changet occurances of services.push.* with dom.push.*

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

The patch is good but there is one more spot to be changed in dom/base/Navigator.cpp.

Something to also think about is how this will play with updates. When a device goes from B2G v1.1 to B2G v1.x when this patch ships, the user preferences changed should still be respected and so on. I don't know if this has any precedent. If there is no simple way to deal with it, it may not be worth making this change.

Comment 9

[Vedad Hadžić [:vedad][:vedadux]]

Attachment: added the dom/base/Navidator.cpp change

This new patch includes the change on file dom/base/Navigator.cpp

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 827503 [details] [diff] [review]
added the dom/base/Navidator.cpp change

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

r=me with the one change.

::: dom/base/Navigator.cpp
@@ +1823,4 @@
>                                              JSObject* aGlobal)
>  {
>    nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> +  return win && Preferences::GetBool("dom.push.enabled", false) && CheckPermission(win, "push");

Nit: Wrap to 80 characters.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrew, is there a way to let partners know that if they customized preferences on their builds, the pref name has changed? Who do I even contact about this?

Comment 12

[Guillermo López :willyaranda (probably SLOW response)]

(In case of TEF, http://www.globalresearch.ca/wp-content/uploads/2009/07/114249.jpg, so no worries)

Comment 13

[Andrew Overholt [:overholt]]

(In reply to Nikhil Marathe [:nsm] from comment #11)
> Andrew, is there a way to let partners know that if they customized
> preferences on their builds, the pref name has changed? Who do I even
> contact about this?

You could email the TAMs for each of the partners.  I'll send you the details in an email.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 827503 [details] [diff] [review]
added the dom/base/Navidator.cpp change

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

::: dom/base/Navigator.cpp
@@ +1823,4 @@
>                                              JSObject* aGlobal)
>  {
>    nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> +  return win && Preferences::GetBool("dom.push.enabled", false) && CheckPermission(win, "push");

Can you move the last check to a new line?

Comment 15

[:ProgramFOX]

Attachment: Bug 887880: Renamed services.push to dom.push

In this patch, I renamed services.push to dom.push in the following files:
 b2g/app/b2g.js
 browser/app/profile/firefox.js
 browser/components/loop/MozLoopPushHandler.jsm
 browser/components/loop/test/xpcshell/head.js
 browser/components/loop/test/xpcshell/test_looppush_initialize.js
 dom/push/src/Push.js
 dom/push/src/PushService.jsm
 dom/push/src/PushServiceLauncher.js
 dom/webidl/PushManager.webidl
 mobile/android/app/mobile.js
 modules/libpref/src/init/all.js

I found this file list at [1]

  [1]: http://dxr.mozilla.org/mozilla-central/search?q=services.push&case=false

Comment 16

[:ProgramFOX]

Thanks for reviewing my patch! As I don't have the ability to push it in the tree myself, could you please do that?

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to ProgramFOX from comment #16)
> Thanks for reviewing my patch! As I don't have the ability to push it in the
> tree myself, could you please do that?

The intention is to subsume this patch for the new Push 2 implementation, while SimplePush will continue to use the old services.push.* prefs. So this will land, just in a while. Thanks!

Comment 18

[Tyler Smith [:tysmith]]

(In reply to ProgramFOX from comment #16)
> Thanks for reviewing my patch! As I don't have the ability to push it in the
> tree myself, could you please do that?

First of all, thanks for helping out! We recently had some internal discussions and decided that we were going to release the new push service alongside the old one, with the intention of phasing out the old push eventually.  With that in mind, do you think it would be possible to create a new patch, but this time with all the dom.push stuff added without removing the services.push?

Comment 19

[:ProgramFOX]

Attachment: Added dom.push for Push 2 implementation

Added dom.push without removing services.push, as requested by Tyler. I found the files that I changed on DXR, but I did not change Push.js, PushService.jsm, PushServiceLauncher.js and PushManager.webidl because those files looked like SimplePush-specific files. I also didn't change MozLoopPushHandler.jsm because the services.push line was here:
>let MozLoopPushHandler = {
>  // This is the uri of the push server.
>  pushServerUri: Services.prefs.getCharPref("services.push.serverURL"),
And I don't think I can add the dom.push.serverURL here.

Let me know if I should change something else.

Comment 20

[Tyler Smith [:tysmith]]

Comment on attachment 8460159 [details] [diff] [review]
Added dom.push for Push 2 implementation

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

Lovely, thank you!

Comment 21

[Tyler Smith [:tysmith]]

Comment on attachment 8460159 [details] [diff] [review]
Added dom.push for Push 2 implementation

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

Actually, wait. https://bugzilla.mozilla.org/show_bug.cgi?id=857464 has a lot of prefs defined in all.js for push, which I based my work on top of.  Should ProgramFOX have based his work on top of those changes, or was that patch more of a temporary thing?

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Right.

ProgramFOX, could you add the new prefs to modules/libpref/src/init/all.js?
Sorry about the constant changes.

Also add comments saying the prefs are for the Push specification, and modify b2g.js to say those prefs are for the non-standard Mozilla SimplePush implementation.

Thanks!

Comment 23

[:ProgramFOX]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #22)
> Right.
> 
> ProgramFOX, could you add the new prefs to modules/libpref/src/init/all.js?
> Sorry about the constant changes.
> 
> Also add comments saying the prefs are for the Push specification, and
> modify b2g.js to say those prefs are for the non-standard Mozilla SimplePush
> implementation.
> 
> Thanks!

When adding the new prefs to all.js, do I have to add only the dom.push prefs, or also the service.push prefs?

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Just add the dom.push prefs.

Comment 25

[:ProgramFOX]

Attachment: Added dom.push for Push 2 implementation

New patch, with new dom.push values in all.js.

Comment 26

[Tyler Smith [:tysmith]]

Comment on attachment 8460997 [details] [diff] [review]
Added dom.push for Push 2 implementation

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

Thanks ProgramFOX.  I had to make a few tiny tweaks that we didn't tell you about and I didn't want to bother you with, but this looks good.

Comment 27

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Superseded by bug 1038811