Last Comment Bug 717975 - only expose m-c implementation of navigator.mozApps on b2g
: only expose m-c implementation of navigator.mozApps on b2g
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
: 725783 (view as bug list)
Depends on: 697383
Blocks: 718912
  Show dependency treegraph
 
Reported: 2012-01-13 10:06 PST by [:fabrice] Fabrice Desré
Modified: 2012-07-28 09:26 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (1.35 KB, patch)
2012-01-13 10:20 PST, [:fabrice] Fabrice Desré
jst: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
followup (1.42 KB, patch)
2012-01-18 09:57 PST, [:fabrice] Fabrice Desré
bugspam.Callek: feedback-
Details | Diff | Review
followup (1.39 KB, patch)
2012-01-18 10:50 PST, [:fabrice] Fabrice Desré
jst: review+
bugspam.Callek: feedback+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description [:fabrice] Fabrice Desré 2012-01-13 10:06:16 PST
This is needed o not conflict with the add-on implementation.
Comment 1 [:fabrice] Fabrice Desré 2012-01-13 10:20:39 PST
Created attachment 588452 [details] [diff] [review]
patch

This patch makes sure that we only build and package Webapps.* for the b2g target.
Comment 2 [:fabrice] Fabrice Desré 2012-01-13 21:10:14 PST
pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08f701c3b6e
Comment 3 :Ms2ger 2012-01-14 01:49:53 PST
https://hg.mozilla.org/mozilla-central/rev/a08f701c3b6e

Don't forget the Target Milestone
Comment 4 Ed Morley [:emorley] 2012-01-17 14:07:43 PST
Doesn't this need a removed files entry as well?
Comment 5 Justin Wood (:Callek) 2012-01-17 22:05:43 PST
(In reply to Ed Morley [:edmorley] from comment #4)
> Doesn't this need a removed files entry as well?

Yes it does, I imagine jst didn't catch that fact.
Comment 6 Justin Wood (:Callek) 2012-01-17 22:11:15 PST
Also you just created an issue with packaging trying to package "dom_apps.xpt" which is created at "dom/interfaces/apps" but is traversed from "dom/Makefile" without a guard on B2G or gonk or anything.

I don't think your done here.
Comment 7 [:fabrice] Fabrice Desré 2012-01-18 09:27:50 PST
Justin,

I'll post a follow-up for the removed files entry - I was not aware of this file.

I don't understand you comment about packaging dom_apps.xpt : the patch that landed removes this entry from browser/installer/package-manifest.in
Comment 8 [:fabrice] Fabrice Desré 2012-01-18 09:57:50 PST
Created attachment 589552 [details] [diff] [review]
followup

followup to :
- add entries to removed-files.sh
- only build the .xpt for b2g
Comment 9 Justin Wood (:Callek) 2012-01-18 10:41:05 PST
Comment on attachment 589552 [details] [diff] [review]
followup

imo looks good except the Makefile.in part, where you _need_ to remove the already-existing interfaces/apps from above (the interfaces section) and my minor nit here would be to have that block added just under the other interfaces traversal there.

I'd move forward with a real review after you fix that. (Thanks)
Comment 10 [:fabrice] Fabrice Desré 2012-01-18 10:50:15 PST
Created attachment 589573 [details] [diff] [review]
followup
Comment 11 Justin Wood (:Callek) 2012-01-18 11:04:21 PST
Comment on attachment 589573 [details] [diff] [review]
followup

Yea that --looks-- good to me, I did not dive deeply into a real review peer at it though :-)
Comment 12 dclarke@mozilla.com [:onecyrenus] 2012-01-18 16:37:25 PST
How should we go about verifying this bug ? The fact that nightly continues to work is not a great test.
Comment 13 [:fabrice] Fabrice Desré 2012-01-19 09:28:39 PST
followup pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de34e4fefcad
Comment 14 Ed Morley [:emorley] 2012-01-19 17:47:07 PST
https://hg.mozilla.org/mozilla-central/rev/de34e4fefcad
Comment 15 Serge Gautherie (:sgautherie) 2012-01-19 23:41:21 PST
Comment on attachment 588452 [details] [diff] [review]
patch

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

Your 2nd patch made things much clearer.

::: dom/base/Makefile.in
@@ +60,5 @@
>  	        Webapps.js \
>  	        Webapps.manifest \
>  		$(NULL)
>  
> +EXTRA_JS_MODULES += Webapps.jsm \

What about Cu.import("resource://gre/modules/Webapps.jsm");
in services/sync/modules/engines/apps.js
which was created by (part 2 of) bug 706545 ?

Is there an issue now, or is it safe somehow?
Comment 16 [:fabrice] Fabrice Desré 2012-01-20 09:36:02 PST
(In reply to Serge Gautherie (:sgautherie) from comment #15) 
> What about Cu.import("resource://gre/modules/Webapps.jsm");
> in services/sync/modules/engines/apps.js
> which was created by (part 2 of) bug 706545 ?
> 
> Is there an issue now, or is it safe somehow?

It is safe because this sync engine is disabled, and has no UI to enable it.
Comment 17 Justin Wood (:Callek) 2012-01-20 09:47:53 PST
(In reply to Fabrice Desré [:fabrice] from comment #16)
> (In reply to Serge Gautherie (:sgautherie) from comment #15) 
> > What about Cu.import("resource://gre/modules/Webapps.jsm");
> > in services/sync/modules/engines/apps.js
> > which was created by (part 2 of) bug 706545 ?
> > 
> > Is there an issue now, or is it safe somehow?
> 
> It is safe because this sync engine is disabled, and has no UI to enable it.

Shouldn't we disable packaging this engine entirely then in the cases we can't use it?

And re-looking we're still missing interfaces/apps removal in this case, but I think we're getting into the "spin-off" bug department anyway....
Comment 18 [:fabrice] Fabrice Desré 2012-01-20 10:11:40 PST
Well, the goal here is not to remove the mozApps implementation foreer. It's a temporary action while we sort out a way to get the m-c implementation and the add-on to coexist peacefully.

This will happen soon.
Comment 19 Serge Gautherie (:sgautherie) 2012-01-20 21:21:19 PST
(In reply to Justin Wood (:Callek) from comment #17)
> And re-looking we're still missing interfaces/apps removal in this case

http://mxr.mozilla.org/comm-central/search?string=interfaces%2Fapps&case=on
{
/mozilla/dom/Makefile.in
    * line 73 -- interfaces/apps \
/mozilla/toolkit/toolkit-makefiles.sh
    * line 49 -- dom/interfaces/apps/Makefile
}

The former is fixed. I assume you're talking about the latter?
Comment 20 Justin Wood (:Callek) 2012-01-21 02:08:07 PST
(In reply to Serge Gautherie (:sgautherie) from comment #19)
> (In reply to Justin Wood (:Callek) from comment #17)
> > And re-looking we're still missing interfaces/apps removal in this case
....
> The former is fixed. I assume you're talking about the latter?

It was the former I was talking about, but either I mis-read or comm-central's mxr wasn't updated when I looked.
Comment 21 Serge Gautherie (:sgautherie) 2012-01-26 12:37:10 PST
PS: Isn't this bug wanted on mozilla11 too?
Comment 22 Myk Melez [:myk] [@mykmelez] 2012-02-09 15:19:13 PST
(In reply to Serge Gautherie (:sgautherie) from comment #21)
> PS: Isn't this bug wanted on mozilla11 too?

Yes.  Moving to Core : DOM: Mozilla Extensions so I can request approval for aurora and beta.
Comment 23 Myk Melez [:myk] [@mykmelez] 2012-02-09 15:28:11 PST
Comment on attachment 588452 [details] [diff] [review]
patch

[Approval Request Comment]

We thought this landed in time to make Firefox 11, but we just discovered today that it didn't (nor to make Firefox 12, currently in aurora), so I'm requesting this land on the two branches tracking those releases.

This low-risk fix disables the unsupported and undocumented transitional navigator.mozApps API that landed late last year.  It does not land the new API, which is being worked on in a separate bug, nor make any other changes.

Although the API is undocumented, we still don't want to ship it, because we don't want folks to discover it and start using it, only to have such usage break after we ship the documented and supported API in a later release (currently targeted at Firefox 13 on desktop and Fennec 14 on mobile).

Note: we will need to land both patches in this bug, this one and the followup.
Comment 24 Myk Melez [:myk] [@mykmelez] 2012-02-09 15:28:31 PST
Comment on attachment 589573 [details] [diff] [review]
followup

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Comment 25 Serge Gautherie (:sgautherie) 2012-02-09 15:37:33 PST
(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> nor to make Firefox 12, currently in aurora

Aren't these already in aurora?
https://hg.mozilla.org/releases/mozilla-aurora/rev/a08f701c3b6e
https://hg.mozilla.org/releases/mozilla-aurora/rev/de34e4fefcad
Comment 26 Myk Melez [:myk] [@mykmelez] 2012-02-09 15:54:24 PST
Comment on attachment 588452 [details] [diff] [review]
patch

(In reply to Serge Gautherie (:sgautherie) from comment #25)
> Aren't these already in aurora?
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a08f701c3b6e
> https://hg.mozilla.org/releases/mozilla-aurora/rev/de34e4fefcad

Erm, indeed, my bad; these are in aurora, but they aren't in beta.  Cancelling the aurora requests, but leaving the beta requests.
Comment 27 Alex Keybl [:akeybl] 2012-02-09 15:55:13 PST
Comment on attachment 588452 [details] [diff] [review]
patch

[Triage Comment]
There should be no external clients of this API, so there's near-zero risk in disabling it. Approved for Beta 11.
Comment 29 dclarke@mozilla.com [:onecyrenus] 2012-02-13 16:08:34 PST
*** Bug 725783 has been marked as a duplicate of this bug. ***
Comment 30 Anant Narayanan [:anant] 2012-02-14 17:01:21 PST
*** Bug 725783 has been marked as a duplicate of this bug. ***
Comment 31 dclarke@mozilla.com [:onecyrenus] 2012-02-16 02:24:20 PST
Verified for FF 11.0 b3.

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