717975 - only expose m-c implementation of navigator.mozApps on b2g

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

This is needed o not conflict with the add-on implementation.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch

This patch makes sure that we only build and package Webapps.* for the b2g target.

Comment 2

[[:fabrice] Fabrice Desré]

pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08f701c3b6e

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a08f701c3b6e

Don't forget the Target Milestone

Comment 4

[Ed Morley [:emorley]]

Doesn't this need a removed files entry as well?

Comment 5

[Justin Wood (:Callek)]

(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)]

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é]

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é]

Attachment: followup

followup to :
- add entries to removed-files.sh
- only build the .xpt for b2g

Comment 9

[Justin Wood (:Callek)]

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é]

Attachment: followup

Comment 11

[Justin Wood (:Callek)]

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

[[e-mail] [:onecyrenus]]

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é]

followup pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de34e4fefcad

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de34e4fefcad

Comment 15

[Serge Gautherie (:sgautherie)]

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é]

(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)]

(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é]

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)]

(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)]

(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)]

PS: Isn't this bug wanted on mozilla11 too?

Comment 22

[Myk Melez [:myk] [@mykmelez]]

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

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

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)]

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

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

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 28

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d17b10ac1ac
https://hg.mozilla.org/releases/mozilla-beta/rev/aacaf3be8ba8

Comment 31

[[e-mail] [:onecyrenus]]

Verified for FF 11.0 b3.