See bug 938023 comment 70. Not sure why this wasn't filed when the backout of bug 938023 was undone, but we need this done sooner rather than later.

Isn't the download API in 1.3? If so, this needs to be 1.3+.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1) > Isn't the download API in 1.3? If so, this needs to be 1.3+. The download manager/API for b2g is a 1.4 feature and we backed everything out in 1.3 afaik.

Ah. Bug 938023 has a target milestone set indicating it's in 1.3, but it was really backed out in bug 948040.

Peter, how do you want to do this?

So, guys, where are we with this? This should be completed this week.

Where we are is that someone needs to write a patch to fix the issues pointed out in the comment referenced in comemnt 0 of this bug. Once that someone (you? Fabrice? Someone else involved in bug 938023?) does that, along with the corresponding changes to test_interfaces.html, Peter or I review the patch and it gets checked in and then backported to whatever branches bug 938023 is on.

And I'm happy to hand-hold as needed, but to do that I need to understand what part of bug 938023 comment 70 is not clear.

Adding comment 70 from bug 938023 (In reply to Peter Van der Beken [:peterv] from comment #70) > Comment on attachment 8339529 [details] [diff] [review] > Patch - Final - Download API (Test Interfaces Settings) - Apply Last > > Review of attachment 8339529 [details] [diff] [review]: > ----------------------------------------------------------------- > > So this is just the test change, but really it's about exposing those > interfaces. It seems odd that we're exposing these interfaces everywhere (on > B2G), they're only usable if the "downloads" permission is set? If that's > the case I think the interfaces should have a |Func="..."| extended > attribute calling a C++ function that checks for the "downloads" permission > (similar to what the code in Navigator::DoNewResolve does). > > I'll file a bug on making this nicer, we should be able to actually generate > these permission checks from an extended attribute on the interface. But > that shouldn't block this for now. The thing is that we have to expose the navigator property even if an app doesn't have the permission. The marketplace app has to be able to determine if an API is available or not even if it doesn't have the permission to access it. AFAIK the current deal with the marketplace app is that the navigator property is 'undefined' if an API is not available on the device (controlled by a pref). It is 'null' if it is available but the marketplace app doesn't have the permission to access it.

I thought we'd come up with a setup for the marketplace app that doesn't force use to pollute the web with this stuff. Certainly plenty of other permissions-based stuff on navigator is hidden based on those permissions!

But also, I don't see what Peter's comments have to do with the marketplace app. We're not saying to make navigator.mozDownloadManager invisible to the web, though that would be ideal. Presumably that's what marketplace uses to detect the API, right? What Peter's comment is talking about is window.DOMDownload, window.DOMDownloadManager, window.DownloadEvent. Those are being stuck on the window for all webpages. That's not OK, and certainly isn't needed by marketplace.

And if marketplace _does_ currently still need the namespace pollution on Navigator, who owns fixing that problem? We've been talking about it for like 18 months now, and our current setup really is not acceptable if it still requires exposing random broken stuff to the web just so marketplace can detect it. It's just not.

(In reply to Boris Zbarsky [:bz] from comment #10) > > What Peter's comment is talking about is window.DOMDownload, > window.DOMDownloadManager, window.DownloadEvent. Those are being stuck on > the window for all webpages. That's not OK, and certainly isn't needed by > marketplace. Ok I see what you mean now. Thanks for the clarification. A quick followup question: What is different with the download API and all the other APIs we are exposing for B2G that are behind a permission? I assume they got a proper r+ from a DOM peer. Are we at a point where we need a generic solution or is there anything else I am missing here. For the API detection for marketplace: I thought Jonas has a plan there. I will let him talk to this once he is back.

> What is different with the download API and all the other APIs we are exposing for B2G > that are behind a permission? There shouldn't be any. > I assume they got a proper r+ from a DOM peer. I wouldn't assume that. People have been checking in stuff without review all over; that's why the scary warnings got added to test_interfaces that bug 938023 then ignored. We may well need to go and fix other APIs too. > Are we at a point where we need a generic solution Maybe, see bug 938023 comment 70. But I wouldn't block on that generic solution for fixing this bug...

OK, sounds like I have enough information to wrap this up (in lieu of a generic solution). Fabrice had a preliminary patch to address some of the concerns. I will see if he still has it laying around and will revive it.

khuey, peterv or bz may make more sense as reviewers, as they are probably more familiar with this.

>+ // WebIDL standard specifies we should simply ignore setting attributes >+ // to invalid enum values. The bindings actually enforce that already. But in any case, "state" is readonly, so who's going to be calling that setter? And even more importantly, that patch seems to be completely ignoring the main issue this bug was filed about...

Comment on attachment 8360690 [details] [diff] [review] Patch - Final - Address review concerns from DOM Peers. r- due to comment 18. To be clear, it appears the missing piece here is that the interface names are always visible, even to code that doesn't have the "downloads" permission.

What would be a good place to stick a [Func] in this case, since it's a JS-implemented interface?

In Navigator.cpp in dom/base. I'm testing out a patch that addresses the visibility of mozDownloadManager to apps that don't have the correct permissions right now, it'll be up as soon as my mochitest run passes.

Please don't forget to revert the test_interfaces.html changes from bug 938023....

Comment on attachment 8361404 [details] [diff] [review] bug957592-domdownload-webidl.patch >+ nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(js::CheckedUnwrap(aGlobal)); Why do you need the CheckedUnwrap? That seems fishy... r=me on the rest if you revert attachment 8339529 [details] [diff] [review] and if the marketplace is ok with this setup. Thank you!

(In reply to Ghislain 'Aus' Lacroix from comment #21) > In Navigator.cpp in dom/base. I thought the plan was to leave navigator.mozDownloadManager as null, just not the stuff on window. That would mean the Navigator::HasDownloadsSupport method would be unrelated to Navigator. I guess if the null-means-no-permission thing is no longer necessary, I'll file a bug to remove the mozSettings code.

Boris, I believe I need to do this because part of the implementation accesses the content window from chrome. Which ends up being a security wrapper around a window. This means that when we get to getting the window from the global, it's possible that we will need to unwrap first. I believe if it's already the window unwrap does nothing. I just learned how all this really works today, so, if I got some parts in there a little off, I apologize :) I will revert attachment 8339529 [details] [diff] [review] as well as part of the final patch. Reuben, From my understanding, yes, we could update Settings the same way.

> I believe I need to do this because part of the implementation accesses the content > window from chrome. Hmm. So you have chrome code doing someContentWindow.DOMDownload, basically? But do you really want that to return undefined if someContentWindow doesn't have the permission? Seems like it would make more sense for chrome to always see the DOMDownload property on someContentWindow, no matter what the window's permissions are, I'd think.

Yes, that's correct. the chrome code will be using the constructor attached to the window. I agree that it would be ideal for the chrome window / js code to always have access to it. I'm not sure what the best way to do this would be, could you give me some pointers?

mozilla::dom::ThreadsafeCheckIsChrome should do the trick.

Though given this is window stuff, just xpc::AccessCheck::isChrome(aObj) would work too.

r+ carries. bz, if you don't mind having a quick look, it'd be much appreciated! :)

Comment on attachment 8363311 [details] [diff] [review] Patch - v3 - Minor updates based on review >+++ b/dom/base/Navigator.cpp >+ if(mozilla::dom::ThreadsafeCheckIsChrome(cx, aGlobal)) { Space after "if". And drop the "mozilla::dom::" bit: this file is already in that namespace. Also, please name the first argument to this function "aCx", since the second one is aGlobal. >+++ b/dom/base/Navigator.h + static bool HasDownloadsSupport(JSContext* /* unused */, JSObject* aGlobal); The first argument is used. >+++ b/dom/downloads/tests/serve_file.sjs Bunch of missing whitespace in here ("catch(e)", "if(" a few times, etc). The rest looks fine, I think.

r+ carries from bz. Thanks for all the help. :)

r+ carries from bz.

Inbound is green, marking fixed.

(In reply to Ghislain 'Aus' Lacroix from comment #37) > Inbound is green, marking fixed. Not so fast. The sheriffs are doing this once they merge to central :)

Whoops! Gotcha! I'm just excited that it's finally landed and it looks like I didn't break anything! :)

Comment on attachment 8363871 [details] [diff] [review] Patch - Final - Address review concerns from DOM peers. Review of attachment 8363871 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/downloads/tests/test_downloads_navigator_object.html @@ +47,5 @@ > function() { > SpecialPowers.pushPermissions([ > {type: "downloads", allow: 0, context: document} > ], function() { > + ise(frames[0].navigator.mozDownloadManager, undefined, "navigator.mozDownloadManager is undefined when the page doesn't have permissions"); Oh we shouldn't do that. The navigator.mozDownloadManager has to stay the same when we don't have permission :(

OK. It sounds like what I'd have to do is keep the previous code that existed in Navigator.cpp that exposed mozDownloadManager as null even when the app does not have permissions but also keep the new function style check for DOMDownload and DOMDownloadEvent?

This is the code in question that I believe I need to re-insert for Marketplace feature detection: - if (name.EqualsLiteral("mozDownloadManager")) { - if (!CheckPermission("downloads")) { - aValue.setNull(); - return true; - } - } Correct?

Hey Guys, sorry had to backout this change because of hazard analysis permafailure/bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=33451782&tree=B2g-Inbound

The analysis thinks (incorrectly, I suspect) that ThreadSafeCheckIsChrome can gc. Since aGlobal is used after that call, it needs to be locally rooted...

OK. It looks like we definitely still need to expose mozDownloadManager on navigator when the pref is turned on but the permission isn't present. I've gone ahead and done that to the best of my knowledge. I've also attempted to solve the hazard analysis failure by rooting the object before calling CheckIsChrome and for safety am using it during other calls as well.

Comment on attachment 8364659 [details] [diff] [review] Patch - Keep compatibility with Marketplace while hiding the rest. >+ // We'll need a rooted object so that no GC is possible while we're calling >+ // CheckIsChrome. No, you need it so it won't go away if ThreadsafeCheckIsChrome GCs. You can't _prevent_ GC with a Rooted. r=me with the comment fixed, assuming this passes test_interfaces.html.

r+ carries over from bz. Still waiting on review from Gregor.

FYI, I ran test-interfaces locally and it did not fail because of any of the downloads api interfaces. It did fail because it was lacking interfaces that are not available on desktop. I think we're in the green there.

Comment on attachment 8364732 [details] [diff] [review] Patch - Final - Keep compatibility with Marketplace while hiding the rest Review of attachment 8364732 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/Downloads.webidl @@ +19,5 @@ > + > +// > +// XXXTODO: When we have a generic way to do feature detection in marketplace > +// we will *STOP* using the pref and use the function like DOMDownload > +// and DownloadEvent. nit: Whitespace

Hm the interface tests are failing: https://tbpl.mozilla.org/php/getParsedLog.php?id=33507864&tree=Pine&full=1

Do you perhaps want DOMDownloadManager to be [NoInterfaceObject]?

Yes, I think you do.

r+ carries from bz and gwagner.

For certified APIs, there is no reason for marketplace (or other similar consumers) to be able to detect support for the API. Marketplace will never host apps that can use certified APIs anyway. For non-certified APIs, I was talked out of exposing a null-valued property. It was deemed "too non-javascripty" to ask people to do "if (navigator.foo)" to detect ability to use and "if ('foo' in navigator)" to detect implemented-but-you-might-not-have-access. Instead we were going to add some sort of hasFeature('api.navigator.foo')-like API which we'd use to allow specifically for implemented-but-you-might-not-have-access. Lots of unanswered questions here though. But this seems to me like the path forward. Details to be discussed in a separate bug obviously.

> I believe I need to do this because part of the implementation accesses the content > window from chrome. Hmm. I was just thinking about this some more. Blake says we should really only expose contentWindow.DOMDownload if the target window has the permission. So maybe the CheckedUnwrap thing is in fact the right thing to do...

Blake, see comment 27 to comment 29.

As soon as you guys let me know which is ideal I'll create a small follow-up bug and attach a patch (if need be :))

(In reply to Boris Zbarsky [:bz] from comment #57) > Hmm. I was just thinking about this some more. Blake says we should really > only expose contentWindow.DOMDownload if the target window has the > permission. So maybe the CheckedUnwrap thing is in fact the right thing to > do... My understanding is that the immediate need for chrome to access content's DownloadManager's interface only happens in cases where either solution works. Beyond that, I think that Xrays should reflect the actual state of the underlying DOM object, so that CheckedUnwrap makes more sense here.

OK. I'll do that as part of fixing bug 965094.