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

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

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.

(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!

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

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.

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.

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 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.

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

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.

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?

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

(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 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?

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

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

(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!

(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?

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 on attachment 8460159 [details] [diff] [review] Added dom.push for Push 2 implementation Review of attachment 8460159 [details] [diff] [review]: ----------------------------------------------------------------- Lovely, thank you!

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?

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!

(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?

Just add the dom.push prefs.

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

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.

Superseded by bug 1038811