(In reply to Magnus Melin [:mkmelin] from comment #5) > Ah, null browsingContext isn't allowed - https://hg.mozilla.org/mozilla-central/diff/d6c5410fc2859ce35774009947841ce7a68f1711/uriloader/exthandler/nsExternalHelperAppService.cpp#l1.15 > > Can we allow that when we just want it to open externally? I'm not convinced we want to. The consumer doesn't normally know whether the URI will be opened externally, that's why it's calling the external helper app service. It's explicitly delegating how to open the URI to the app service. If it specifically wanted to open things externally, it could invoke the relevant helper app with the URI manually (via `launchWithURI`), right? I guess if TB knows that there are never any web protocol handlers, that's sort of an edgecase that wasn't considered, because Firefox can never know that for sure... On the flip side, I had to leave the argument optional for `launchWithURI` on the mime info objects because the helper app case doesn't always have access to a browsing context - but that's more to do with the fact that they run downloads that aren't supposed to be associated to a particular content window. The point of having the browser context is that if we want to load in the same context where the URL came from, or need to pop up a dialog a la `unknownContentType`, we have something to work with in terms of parenting that dialog. It's also likely we'll add permission support for external protocols soon (so webpages can't just force-open, uh, say, the Zoom video calling app...) and there too, we're likely to want to use browsing contexts to work out where to ask for permission and/or if we already have permission (I'm not sure off-hand if we'll need to do that behind this entrypoint or will do it elsewhere). So I'm hesitant about relaxing the requirement. If we did, we should continue to enforce that the browsing context param is passed when invoked in the child process. Do all the consumers here have a content area from where the links they want to open originate? It's not clear to me from looking at the patch, given the lack of context... Matt, what do you think?
Bug 1637826 Comment 6 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Magnus Melin [:mkmelin] from comment #5) > Ah, null browsingContext isn't allowed - https://hg.mozilla.org/mozilla-central/diff/d6c5410fc2859ce35774009947841ce7a68f1711/uriloader/exthandler/nsExternalHelperAppService.cpp#l1.15 > > Can we allow that when we just want it to open externally? I'm not convinced we want to. The consumer doesn't normally know whether the URI will be opened externally, that's why it's calling the external helper app service. It's explicitly delegating how to open the URI to the app service. If it specifically wanted to open things externally, it could invoke the relevant helper app with the URI manually (via `launchWithURI`), right? I guess if TB knows that there are never any web protocol handlers, that's sort of an edgecase that wasn't considered, because Firefox can never know that for sure... On the flip side, I had to leave the argument optional for `launchWithURI` on the mime info objects because the mimetype (not URI) case doesn't always have access to a browsing context - but that's more to do with the fact that they run downloads that aren't supposed to be associated to a particular content window. The point of having the browser context is that if we want to load in the same context where the URL came from, or need to pop up a dialog a la `unknownContentType`, we have something to work with in terms of parenting that dialog. It's also likely we'll add permission support for external protocols soon (so webpages can't just force-open, uh, say, the Zoom video calling app...) and there too, we're likely to want to use browsing contexts to work out where to ask for permission and/or if we already have permission (I'm not sure off-hand if we'll need to do that behind this entrypoint or will do it elsewhere). So I'm hesitant about relaxing the requirement. If we did, we should continue to enforce that the browsing context param is passed when invoked in the child process. Do all the consumers here have a content area from where the links they want to open originate? It's not clear to me from looking at the patch, given the lack of context... Matt, what do you think?
(In reply to Magnus Melin [:mkmelin] from comment #5) > Ah, null browsingContext isn't allowed - https://hg.mozilla.org/mozilla-central/diff/d6c5410fc2859ce35774009947841ce7a68f1711/uriloader/exthandler/nsExternalHelperAppService.cpp#l1.15 > > Can we allow that when we just want it to open externally? I'm not convinced we want to. The consumer doesn't normally know whether the URI will be opened externally, that's why it's calling the external helper app service. It's explicitly delegating how to open the URI to the app service. If it specifically wanted to open things externally, it could invoke the relevant helper app with the URI manually (via `launchWithURI`), right? I guess if TB knows that there are never any web protocol handlers, that's sort of an edgecase that wasn't considered, because Firefox can never know that for sure... On the flip side, I had to leave the argument optional for `launchWithURI` on the mime info objects because the mimetype (not URI) case doesn't always have access to a browsing context - but that's more to do with the fact that they run downloads that aren't supposed to be associated to a particular content window. Do all the consumers here have a content area from where the links they want to open originate? It's not clear to me from looking at the patch, given the lack of context... The point of having the browser context is that if we want to load in the same context where the URL came from, or need to pop up a dialog a la `unknownContentType`, we have something to work with in terms of parenting that dialog. It's also likely we'll add permission support for external protocols soon (so webpages can't just force-open, uh, say, the Zoom video calling app...) and there too, we're likely to want to use browsing contexts to work out where to ask for permission and/or if we already have permission (I'm not sure off-hand if we'll need to do that behind this entrypoint or will do it elsewhere). So I'm hesitant about relaxing the requirement. If we did, we should continue to enforce that the browsing context param is passed when invoked in the child process. Matt, what do you think?