Closed Bug 983687 Opened 10 years ago Closed 8 years ago

Introduce and implement nsIAppChannel

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mayhemer, Assigned: nbp)

References

Details

Attachments

(2 files, 2 obsolete files)

There is SetAppURI method on nsIJARChannel that actually doesn't well belong there, used just at [1].  Other reason is we need to safely recognize a channel is loading something from an 'app://' URL w/o examining the url schema directly.  QI to nsIAppChannel seems to be a better way.  The new interface and channel should go to netwerk/protocol/app.

[1] http://hg.mozilla.org//mozilla-central/annotate/f073b3d6db1f/netwerk/protocol/app/AppProtocolHandler.cpp#l406
Looking at the code, of nsAppProtocolHandler::NewChannel, it seems that we want to either emulate a 404 with a DummyChannel or dispatch to the nsJARChannel.

I looked inside the code of Gecko, and I cannot find any literal QI(nsIJARChannel) for the moment.  So I think this is fine to hide the implementation of the nsJARChannel under a (to be added) nsAppChannel.

Should I attempt to remove the nsIJARChannel from the inheritance and delegate every function calls to a nsJARChannel member?
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Fabrice, why do we have a setAppURI on the nsIJARChannel?
Flags: needinfo?(fabrice)
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Should I attempt to remove the nsIJARChannel from the inheritance and
> delegate every function calls to a nsJARChannel member?

Do:

class nsAppChannel : public nsJARChannel
                   , public nsIAppChannel
{
  NS_DECL_INHERITED_ISUPPORTS
  NS_DECL_NSIAPPCHANNEL
};
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Fabrice, why do we have a setAppURI on the nsIJARChannel?

That's legacy, feel free to refactor. Just don't break xhr code at https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1068
Flags: needinfo?(fabrice)
>  why do we have a setAppURI on the nsIJARChannel?

IIRC the jar code becomes unhappy if its mURI is not "jar:", so the appURI was just a hack for storing the original app: URI (which we always transform into a jar: URI internally).

Like Fabrice said, any refactoring that doesn't break stuff is OK :)
Attached patch Add nsIAppChannel. (obsolete) — Splinter Review
I kind of new in this world of IDL, QI, and NS_*.  The current patch compiles fine, and after reading what the macros are doing I think this should be fine.

I will test it to make sure I am not breaking anything.

Currently the nsAppChannel is just a nsJARChannel which implement the empty nsIAppChannel interface and forward everything to the parent class.

Due to the ambiguous interface inheritance, I had to modify the AppProtocolHandler::NewChannel to add a static cast which is not implictly handled by the forget function of nsAutoPtr.  Is there a nice way to avoid this "forget().take()" ?
Attachment #8393530 - Flags: review?(honzab.moz)
Attachment #8393530 - Flags: review?(fabrice)
Comment on attachment 8393530 [details] [diff] [review]
Add nsIAppChannel.

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

Major issue: don't derive nsIAppChannel from nsIJARChannel, just from nsISupports

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +411,5 @@
>  
>    rv = channel->SetOriginalURI(aUri);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  *aResult = channel.forget().take()->asChannel();

not needed with my suggestions to the IDL.

::: netwerk/protocol/app/nsAppChannel.cpp
@@ +18,5 @@
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsAppChannel, nsIDownloadObserver, nsJARChannel)
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsAppChannel, nsIRequestObserver, nsJARChannel)
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsAppChannel, nsIStreamListener, nsJARChannel)
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsAppChannel, nsIRemoteOpenFileListener, nsJARChannel)
> +    NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsAppChannel, nsIJARChannel, nsJARChannel)

this may probably go away as well when you don't derive nsIAppChannel from nsIJARChannel.

::: netwerk/protocol/app/nsAppChannel.h
@@ +9,5 @@
> +
> +#include "nsIAppChannel.h"
> +#include "nsJARChannel.h"
> +
> +namespace mozilla { namespace net {

nit: each on a new line

@@ +26,5 @@
> +    NS_FORWARD_NSIJARCHANNEL(asJARChannel()->)
> +    NS_FORWARD_NSIDOWNLOADOBSERVER(asJARChannel()->)
> +    NS_FORWARD_NSIREQUESTOBSERVER(asJARChannel()->)
> +    NS_FORWARD_NSISTREAMLISTENER(asJARChannel()->)
> +    NS_FORWARD_NSIREMOTEOPENFILELISTENER(asJARChannel()->)

You don't need to this with my proposed changes to the IDL.  nsJARChannel implements all already.

::: netwerk/protocol/app/nsIAppChannel.idl
@@ +7,5 @@
> +
> +[scriptable, builtinclass, uuid(aa9ad971-c3d7-4403-957a-d7e29e2c0bd3)]
> +interface nsIAppChannel : nsIJARChannel 
> +{
> +};

nit: whitespace

Don't derive from nsIJARChannel, just from nsISupports

Move the SetAppURI method here to this new interface.
Attachment #8393530 - Flags: review?(honzab.moz) → feedback-
Comment on attachment 8393530 [details] [diff] [review]
Add nsIAppChannel.

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

clearing review given Honza's feedback
Attachment #8393530 - Flags: review?(fabrice)
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Major issue: don't derive nsIAppChannel from nsIJARChannel, just from
> nsISupports

Isn't that weird to have a channel interface which does not derive from nsIChannel?
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > Major issue: don't derive nsIAppChannel from nsIJARChannel, just from
> > nsISupports
> 
> Isn't that weird to have a channel interface which does not derive from
> nsIChannel?

Good point, derive it from nsIChannel please.  You are right!
Attached patch Add nsIAppChannel. (obsolete) — Splinter Review
This patch add an nsIAppChannel which inherits from the nsIChannel.
This implies that I had to keep the _AMBIGUOUS because the nsAppChannel class inherits from the nsJARChannel implementation.

By keeping the nsRefPtr<nsJARChannel> in AppProtocolHandler::NewChannel, I can keep the same forget function, as the resolve of nsIChannel is not ambiguous.

I am making another patch to move SetAppURI to nsAppChannel, as it involve removing NS_FORWARD_NSICHANNEL, which makes the patch more verbose for one function.  As we want to delegate every call to the nsJARChannel implementation and only redefine GetURI on nsAppChannel.
Attachment #8393530 - Attachment is obsolete: true
Attachment #8404734 - Flags: review?(honzab.moz)
Comment on attachment 8404734 [details] [diff] [review]
Add nsIAppChannel.

I am cancelling the review because this patch seems to break All marionette tests on the B2G emulator. => investigating …

Any idea what might be wrong?
Attachment #8404734 - Flags: review?(honzab.moz)
Ok, the problem was simple, apparently I forgot that NS_FORWARD_NSI* were implementing virtual functions.  So, “asChannel()->” was incorrect, as it was not statically resolved to the nsJAR implementation, but dynamically resolved to the nsAppChannel implementation (i-e infinite loops).

I fix the above by replace it by naming the parent class in the forward declarations.
Attachment #8404734 - Attachment is obsolete: true
Attachment #8406110 - Flags: review?(honzab.moz)
This patch move setAppURI from the nsIJARChannel to the nsIAppChannel.

I think we can keep this function only as part of the implementation instead of being exposed publicly as part of the interface.  What do you think?
Attachment #8406111 - Flags: review?(honzab.moz)
Comment on attachment 8406110 [details] [diff] [review]
bug983687-Add-nsAppChannel.patch

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

Nice!

r=honzab

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +343,5 @@
>  // jar:file:///path/to/profile/webapps/ABCDEF/application.zip!/path/to/file.ext
>  NS_IMETHODIMP
>  AppProtocolHandler::NewChannel(nsIURI* aUri, nsIChannel* *aResult)
>  {
> +  using mozilla::net::nsAppChannel;

rather qualify on use

::: netwerk/protocol/app/moz.build
@@ +13,3 @@
>  SOURCES += [
>      'AppProtocolHandler.cpp',
> +    'nsAppChannel.cpp',

please name the new file AppChannel only (it's the current convention for c file naming)

::: netwerk/protocol/app/nsAppChannel.h
@@ +9,5 @@
> +
> +#include "nsIAppChannel.h"
> +#include "nsJARChannel.h"
> +
> +namespace mozilla { namespace net {

on separate lines
Attachment #8406110 - Flags: review?(honzab.moz) → review+
Comment on attachment 8406111 [details] [diff] [review]
bug983687-2-Move-SetAppURI-to-nsAppChannel.patch

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

Looks good to me.
r=honzab

::: modules/libjar/nsJARChannel.cpp
@@ +349,5 @@
>          if (NS_SUCCEEDED(rv) && scheme.EqualsLiteral("remoteopenfile")) {
>              nsRefPtr<RemoteOpenFileChild> remoteFile = new RemoteOpenFileChild();
> +
> +            nsCOMPtr<nsIURI> appURI;
> +            GetURI(getter_AddRefs(appURI));

this is OK since we never expect AppChannel instance that would not have set its mAppURI.

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +406,5 @@
>    rv = channel->SetOriginalURI(aUri);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsIJARChannel> jarChannel(channel);
> +  jarChannel.forget(aResult);

the original code should work, no?

::: netwerk/protocol/app/nsAppChannel.cpp
@@ +166,5 @@
> +    nsAutoCString scheme;
> +    aURI->GetScheme(scheme);
> +    if (!scheme.EqualsLiteral("app")) {
> +        return NS_ERROR_INVALID_ARG;
> +    }

nit, there is SchemeIs:  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#170

::: netwerk/protocol/app/nsAppChannel.h
@@ +28,5 @@
> +    nsAppChannel();
> +
> +  private:
> +
> +    nsCOMPtr<nsIURI>                mAppURI;

probably no new blank line before
Attachment #8406111 - Flags: review?(honzab.moz) → review+
Comment on attachment 8406111 [details] [diff] [review]
bug983687-2-Move-SetAppURI-to-nsAppChannel.patch

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

::: netwerk/protocol/app/nsAppChannel.cpp
@@ +29,5 @@
> +
> +NS_IMETHODIMP
> +nsAppChannel::GetOriginalURI(nsIURI **aURI)
> +{
> +    return nsJARChannel::GetOriginalURI(aURI);

I assume XPCOM is making you write concrete functions for all these IDL methods like GetOriginalURI, even though we'd normally be able to not write any code and just have the nsJARChannel ones called via the usual C++ inheritance?

::: netwerk/protocol/app/nsIAppChannel.idl
@@ +10,5 @@
>  {
> +    /**
> +     * Forces the uri to be a app:// uri.
> +     */
> +    void setAppURI(in nsIURI uri);

I wonder if we can simply get rid of setAppURI()?  It's only reason originally was to shove in the concept of an app channel to nsJARChannel.  We create the appChannel with an app:// URI, right?  Perhaps if we store it in some other variable besides mURI, we can have the best of both worlds:  mURI will be a jar:// URI (so the underlying JARChannel code is happy), while we can still return the app:// URI for calls to GetURI and GetOriginalURI.

I haven't thought through all the details, but that seems preferable from an interface perspective (it's unintuitive to create a channel with an app: URI and have it's GetURI return a jar:// URI, which is what we'll do so far, right?)

If we need to access both the jar:// and app:// URIs from the IDL (I'm not clear that we do), it would be better to have a getJarUri() method.
Attachment #8406111 - Flags: feedback+
(In reply to Jason Duell (:jduell) from comment #17)
> Comment on attachment 8406111 [details] [diff] [review]
> bug983687-2-Move-SetAppURI-to-nsAppChannel.patch
> 
> Review of attachment 8406111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume XPCOM is making you write concrete functions for all these IDL
> methods like GetOriginalURI, even though we'd normally be able to not write
> any code and just have the nsJARChannel ones called via the usual C++
> inheritance?

Good point.  We can actually just (re)declare GetURI() in the header and save a lot of code.
(In reply to Honza Bambas (:mayhemer) from comment #16)
> ::: netwerk/protocol/app/AppProtocolHandler.cpp
> @@ +406,5 @@
> >    rv = channel->SetOriginalURI(aUri);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  nsCOMPtr<nsIJARChannel> jarChannel(channel);
> > +  jarChannel.forget(aResult);
> 
> the original code should work, no?

No this does not work because the forget function does not know which nsIChannel is the right one in the inheritance tree.

This is for the exact same reason that I have to use NS_DECL_FORWARD, as I understand it.
Because nsIAppChannel is a nsIChannel, as well as nsIJARChannel.  As nsAppChannel implementation derives from both, I need to forward all implementations to nsJARChannel implementation.

(In reply to Jason Duell (:jduell) from comment #17)
> ::: netwerk/protocol/app/nsAppChannel.cpp
> @@ +29,5 @@
> > +
> > +NS_IMETHODIMP
> > +nsAppChannel::GetOriginalURI(nsIURI **aURI)
> > +{
> > +    return nsJARChannel::GetOriginalURI(aURI);
> 
> I assume XPCOM is making you write concrete functions for all these IDL
> methods like GetOriginalURI, even though we'd normally be able to not write
> any code and just have the nsJARChannel ones called via the usual C++
> inheritance?

Are you suggesting that I add an nsAppChannelBase, which does the NS_FORWARD_DECL, and derive nsAppChannel from it such as I can only overload the virtual method GetURI?
I'm not suggesting anything :)  I was just curious about why you had to declare your own version of functions that I figured you might be able to inherit from.  And I don't understand Honza's suggestion: if we (re)declare GetURI, etc, then we still need to implement it again.   In any case, there's no great harm in having the function redefined and just calling into the nsJARChannel versions.

Any thoughts on my plan to get rid of SetAppURI()?
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: