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

VERIFIED FIXED in Firefox 11

Status

()

Core
DOM: Apps
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
This is needed o not conflict with the add-on implementation.
(Assignee)

Comment 1

5 years ago
Created attachment 588452 [details] [diff] [review]
patch

This patch makes sure that we only build and package Webapps.* for the b2g target.
Attachment #588452 - Flags: review?(jst)

Updated

5 years ago
Attachment #588452 - Flags: review?(jst) → review+
(Assignee)

Comment 2

5 years ago
pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08f701c3b6e
https://hg.mozilla.org/mozilla-central/rev/a08f701c3b6e

Don't forget the Target Milestone
Assignee: nobody → fabrice
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: only expose m-c implementation of mavigator.mozApps on b2g → only expose m-c implementation of navigator.mozApps on b2g
Status: RESOLVED → VERIFIED

Comment 4

5 years ago
Doesn't this need a removed files entry as well?
Blocks: 718912
(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.
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
Created attachment 589552 [details] [diff] [review]
followup

followup to :
- add entries to removed-files.sh
- only build the .xpt for b2g
Attachment #589552 - Flags: feedback?(bugspam.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)
Attachment #589552 - Flags: feedback?(bugspam.Callek) → feedback-
(Assignee)

Comment 10

5 years ago
Created attachment 589573 [details] [diff] [review]
followup
Attachment #589552 - Attachment is obsolete: true
Attachment #589573 - Flags: feedback?(bugspam.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 :-)
Attachment #589573 - Flags: feedback?(bugspam.Callek) → feedback+
(Assignee)

Updated

5 years ago
Attachment #589573 - Flags: review?(jst)

Updated

5 years ago
Attachment #589573 - Flags: review?(jst) → review+
How should we go about verifying this bug ? The fact that nightly continues to work is not a great test.
(Assignee)

Comment 13

5 years ago
followup pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de34e4fefcad
https://hg.mozilla.org/mozilla-central/rev/de34e4fefcad
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 697383
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?
(Assignee)

Comment 16

5 years ago
(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.
(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....
(Assignee)

Comment 18

5 years ago
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.
(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?
(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.
PS: Isn't this bug wanted on mozilla11 too?
(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.
Assignee: fabrice → nobody
Component: General → DOM: Mozilla Extensions
Product: Web Apps → Core
QA Contact: general → general
Target Milestone: --- → mozilla11
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.
Attachment #588452 - Flags: approval-mozilla-beta?
Attachment #588452 - Flags: approval-mozilla-aurora?
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:
Attachment #589573 - Flags: approval-mozilla-beta?
Attachment #589573 - Flags: approval-mozilla-aurora?
(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
Assignee: nobody → fabrice
status-firefox11: --- → affected
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: mozilla11 → mozilla12
Version: unspecified → Trunk
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.
Attachment #588452 - Flags: approval-mozilla-aurora?
Attachment #589573 - Flags: approval-mozilla-aurora?
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.
Attachment #588452 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
Attachment #589573 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
tracking-firefox11: --- → +
(Assignee)

Comment 28

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/1d17b10ac1ac
https://hg.mozilla.org/releases/mozilla-beta/rev/aacaf3be8ba8
status-firefox11: affected → fixed
Duplicate of this bug: 725783
Duplicate of this bug: 725783
QA Contact: general → dclarke
Verified for FF 11.0 b3.
Status: RESOLVED → VERIFIED

Updated

5 years ago
QA Contact: dclarke → general

Updated

5 years ago
Component: DOM: Mozilla Extensions → DOM: Apps
You need to log in before you can comment on or make changes to this bug.