Josh Matthews [:jdm] Assignee
	Description • 15 years ago

Currently it is not.

<bsmedberg-away> currently the content process has no profile, so it only has app chrome, not any extension chrome

	Josh Matthews [:jdm] Assignee
	Comment 1 • 14 years ago

This gets basic chrome://package/content/ URIs loading up fine in the content process.  There's no error handling, and there's no check for existing elements, but it gets the job done.

	Josh Matthews [:jdm] Assignee
	Comment 2 • 14 years ago

Now with working resource:// support.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 14 years ago

Just wondering if it would make sense to resolve chrome: and resource: URLs
lazily. So if content process tries to load some url which isn't in the
registry, it would ask parent process if it knows the absolute URL for that.
Or something like that.

	Josh Matthews [:jdm] Assignee
	Comment 5 • 14 years ago

Benjamin and I discussed that possibility, but this way is much easier to implement correctly.

	Benjamin Smedberg
	Comment 6 • 14 years ago

Looking over this patch, I think that we should do this in PContentProcess not PIFrameEmbedding, and we should just send a single message which is an struct/array of chrome registry data (package registrations and override list). We may also need to deal with "current skin/locale" somehow.

I prefer the up-front approach to lazy resolution: there are fewer messages and no sync messages involved.

	Josh Matthews [:jdm] Assignee
	Comment 7 • 14 years ago

This is a much tidier implementation, with a single message being sent on process creation.  I haven't looked into skins or locales yet, but I suspect this would be landable in its current state.

	Josh Matthews [:jdm] Assignee
	Comment 8 • 14 years ago

And of course I've noticed a couple small nits that I've fixed locally:

- extraneous #include <nsXULAppAPI.h> in TabChild.cpp
- the new public methods on nsChromeRegistry should be sandwiched in an #ifdef MOZ_IPC

I think that's it, though.

	Benjamin Smedberg
	Comment 9 • 14 years ago

Comment on attachment 429645 [details] [diff] [review]
Send all chrome registry details to content process children on creation

>+++ b/chrome/src/ChromeTypes.h

>+#ifndef ChromeTypes_h__
>+#define ChromeTypes_h__

Nit, don't use trailing underscores on include guards. The include guard should include the namespace, so mozilla_ChromeTypes_h.

>+#ifndef MOZILLA_INTERNAL_API
>+#include "nsStringAPI.h"
>+#else
>+#include "nsString.h"
>+#endif

You can replace this block with #include "nsStringGlue.h"

But I'm surprised that this code is ever compiled with the external API... where is that happening?

>+struct ChromePackage {
>+  nsCString package;
>+  nsCOMPtr<nsIURI> baseURI;
>+  PRUint32 flags;
>+};

Nit, opening brace of struct/class goes in column 0, here and below. The only place this structure is used is in the IPDL spec, right? In that case I wish you'd not store a nsIURI here but go ahead and convert it to a string when you create this list.

Also the lack of locale/skin mappings here is going to cause problems pretty quickly, I think. Ideally what I'd like here is:

struct ChromePackage
{
  nsCString package;
  nsCString contentBaseURI;
  nsCString localeBaseURI;
  nsCString skinBaseURI;
  PRUint32 flags;
};

>+struct ChromeResource {
>+  nsCString package;
>+  nsCOMPtr<nsIURI> resolvedURI;
>+};

It was completely unclear to me, until I read later parts of this patch, that this has nothing to do with chrome registration and is all about resource:// URI mappings. That should be made more clear, perhaps by calling this ResourceMapping?

>diff --git a/chrome/src/Makefile.in b/chrome/src/Makefile.in

>+LOCAL_INCLUDES += \
>+		-I$(topsrcdir)/dom/ipc \
>+		-I$(topsrcdir)/netwerk/protocol/res/src \
>+		-I$(topsrcdir)/netwerk/base/src \

We need this for nsResProtocolHandler?

>diff --git a/chrome/src/RegistryMessageUtils.h b/chrome/src/RegistryMessageUtils.h

ditto include-guard and string-include comments above

Although I don't really understand why this is a separate file: can we just move these serialize/deserialize bits up into ChromeTypes.h and skip the extra file?

>+namespace IPC {
>+
>+inline void WriteURI(Message* aMsg, nsIURI* aURI)
>+{
>+  nsCString spec;
>+  if (aURI)
>+    aURI->GetSpec(spec);
>+  WriteParam(aMsg, spec);

I'm not sure it matters in this case, but a URI spec can be various charsets, not always UTF8. To be perfectly correct we should entire make sure we're getting the UTF8 spec or pass the charset also.

>diff --git a/chrome/src/nsChromeRegistry.cpp b/chrome/src/nsChromeRegistry.cpp

>+nsChromeRegistry*
>+nsChromeRegistry::GetService()

This should really be an already_AddRefed<nsChromeRegistry>

>+PLDHashOperator
>+nsChromeRegistry::CollectResources(const nsACString& aKey,
>+                                   nsIURI* aURI,
>+                                   void* aArg)
>+{
>+  nsTArray<ChromeResource>* resources =
>+      static_cast<nsTArray<ChromeResource>*>(aArg);
>+  ChromeResource resource = {
>+    nsDependentCString(aKey), aURI
>+  };
>+  resources->AppendElement(resource);
>+  return (PLDHashOperator)PL_DHASH_NEXT;
>+}

It seems a little strange to me that this method is in nsChromeRegistry instead of nsResProtocolHandler. Can it be moved and you just pass it a nsTArray reference to fill?

>+  nsCOMPtr<nsIProtocolHandler> ph;
>+  nsresult rv = io->GetProtocolHandler("resource", getter_AddRefs(ph));
>+  NS_ENSURE_SUCCESS(rv, );
>+  
>+  nsCOMPtr<nsIResProtocolHandler> irph (do_QueryInterface(ph));
>+  nsResProtocolHandler* rph = static_cast<nsResProtocolHandler*>(irph.get());
>+  rph->EnumerateSubstitutions(CollectResources, &resources);

There is a logical hole here: we don't set up substitutions until somebody asks for them the first time; you can leave this as an TODO comment for now and file a followup, but it'll probably bite extension authors and maybe fennec.

As for the object organization: there are definitely some methods of nsChromeRegistry which make sense for both chrome and content processes, but many of them don't: it would probably be better to have a base class and separate nsChromeRegistryChrome/nsChromeRegistryContent classes.

Finally, at least mark a TODO/file a followup about the override map, which you're currently not sending at all.


>diff --git a/chrome/src/nsChromeRegistry.h b/chrome/src/nsChromeRegistry.h

>+  void SendRegisteredChrome(mozilla::dom::ContentProcessParent* aChild);

This could/should be PContentProcessParent, couldn't it?

>diff --git a/dom/ipc/PContentProcess.ipdl b/dom/ipc/PContentProcess.ipdl

>+    registerChrome(ChromePackage[] packages, ChromeResource[] resources);

Please make this StudlyCaps per general IPDL convention, even though PIFrameEmbedding did it wrong. (My fault!)

>diff --git a/netwerk/protocol/res/src/nsResProtocolHandler.h b/netwerk/protocol/res/src/nsResProtocolHandler.h

Want MOZ_IPC ifdefs here.

	Josh Matthews [:jdm] Assignee
	Comment 10 • 14 years ago

> There is a logical hole here: we don't set up substitutions until somebody asks
> for them the first time; you can leave this as an TODO comment for now and file
> a followup, but it'll probably bite extension authors and maybe fennec.

I don't think that's actually a problem; since I need to acquire the chrome registry service to send the data over the wire, the service is initialized and new chrome is registered in time (including substitutions).

	Benjamin Smedberg
	Comment 11 • 14 years ago

I'm not talking about the chrome registry: I'm talking about the resource protocol handler. It only sets up some mappings on-demand (any mapping provided by a resource:foo directory service entry). It may not matter in practice now, though.

	Josh Matthews [:jdm] Assignee
	Comment 12 • 14 years ago

One problem that's come up while trying to get the chrome registry xpcshell tests to work on the child side is that registering custom protocol handlers is completely broken.  I'm not really sure what the best way to fix that is; on-demand synchronous url resolution is starting to look more attractive.

	Josh Matthews [:jdm] Assignee
	Comment 13 • 14 years ago

This revised patch should address all the previous comments.  The most interesting changes include:

* The override table, along with the skins and locales details, are now included in the payload.
* URIs are sent in a SerializedURI tuple of spec+charset
* Brand new test demonstrating working resolution of content/skin/locale/resource/override URIs
* Separate chrome and content objects for the registry

	Benjamin Smedberg
	Comment 14 • 14 years ago

Comment on attachment 431552 [details] [diff] [review]
Send all chrome registry details to content process children on creation v2

>diff --git a/chrome/src/RegistryMessageUtils.h b/chrome/src/RegistryMessageUtils.h

>+struct SerializedURI
>+{
>+  nsCString spec, charset;
>+};

Nit, please declare these separately, mainly so that DXR and error messages can find them better.

>+struct ResourceMapping
>+{
>+  nsCString resource;
>+  SerializedURI resolvedURI ;

Nit, remove extra space.

>diff --git a/chrome/src/nsChromeRegistry.cpp b/chrome/src/nsChromeRegistry.cpp

> NS_IMETHODIMP
> nsChromeRegistry::CheckForNewChrome()
> {

>+  return NS_ERROR_NOT_IMPLEMENTED;
> }

Two notes here:

* it really makes more sense to leave this method as pure-virtual in nsChromeRegistry (that is, don't declare it at all), and do the NOT_IMPLEMENTED bit in nsChromeRegistryContent. That may involve manually declaring some things instead of using NS_DECL_NSICHROMEREGISTRY. If this is really difficult, you can skip it, but it feels cleaner and more obvious to me.

* Add an assertion to the NOT_IMPLEMENTED classes in nsChromeRegistryContent, because we really shouldn't be calling these methods at all.

>diff --git a/chrome/src/nsChromeRegistry.h b/chrome/src/nsChromeRegistry.h

Please include a doccomment for nsChromeRegistry which explains the split between chrome/content.

>-protected:
>+private:
>   nsresult GetDynamicInfo(nsIURI *aChromeURL, PRBool aIsOverlay, nsISimpleEnumerator **aResult);
> 
>   nsresult LoadInstallDataSource();
>   nsresult LoadProfileDataSource();

It looks like these methods are unimplemented, can we just remove their declarations (leftovers from a prior RDF world, I think).

>+protected:
>   PRBool mInitialized;
>   PRBool mProfileLoaded;

mProfileLoaded should only be on nsChromeRegistryChrome, I think

>   // Hash of package names ("global") to PackageEntry objects
>   PLDHashTable mPackagesHash;

I *think* this should only apply to nsChromeRegistryChrome also, but I'm not clear on how you're storing the data structures for nsChromeRegistryContent. I think that nsChromeRegistryContent can have much simpler data structures, with a single package->data mapping which contains a base URL for content/skin/locale, unlike nsChromeRegistryChrome which has the lists of locales/skins and all the selection code.

>   nsCString mSelectedLocale;
>   nsCString mSelectedSkin;

Definitely chrome-only.

	Josh Matthews [:jdm] Assignee
	Comment 15 • 14 years ago

I ended up needing to inline every interface declaration, because all of them end up touching each other with abandon.  This means a significant amount of nsChromeRegistry has ended up being transplanted into nsChromeRegistryChrome, so this diff's quite a bit larger than before.

	Benjamin Smedberg
	Comment 16 • 14 years ago

Comment on attachment 432043 [details] [diff] [review]
Remote chrome registration, with chrome/content functionality split

In nsChromeRegistry you redeclare a bunch of pure-virtual methods (= 0) from the interfaces. You really only need to declare the methods that are implemented, and the compiler knows that the others are still pure-virtual. Also I think that you can remove NS_SCRIPTABLE from the hand declarations. The static analysis knows that if you're override a scriptable method it's still scriptable.

I do not allow NS_ENSURE_* macros in new code: I think that they hide return statements and make the code more confusing in general. Please replace them with

if (NS_FAILED(rv))
  return rv;

You don't need to change the code you copy/pasted, just the new usage in nsChromeRegistry::ConvertChromeURL etc.

r=me with those changes

	Josh Matthews [:jdm] Assignee
	Comment 17 • 14 years ago

All comments addressed, with 100% more rebasing.

	Josh Matthews [:jdm] Assignee
	Comment 18 • 14 years ago

Just fixing up a few nits that prevent subsequent patches in the fennec queue from being applied cleanly.

	Josh Matthews [:jdm] Assignee
	Comment 19 • 14 years ago

Comment on attachment 434130 [details] [diff] [review]
Patch to push v2

Guh, I'm really sorry, this patch is busted in really weird ways.  I'll get a proper, buildable one up shortly.

	Josh Matthews [:jdm] Assignee
	Comment 20 • 14 years ago

Now applies completely cleanly to fennec, and builds as well!  Dreams can come true.

	Doug Turner (:dougt)
	Comment 21 • 14 years ago

http://hg.mozilla.org/projects/electrolysis/rev/38e4a1494075

nice job Josh.

	Josh Matthews [:jdm] Assignee
	Comment 22 • 14 years ago

Build fix.

	Doug Turner (:dougt)
	Comment 23 • 14 years ago

http://hg.mozilla.org/projects/electrolysis/rev/e6a6a55b15a4

	neil@parkwaycc.co.uk
	Comment 24 • 14 years ago

Comment on attachment 434147 [details] [diff] [review]
Patch to push v3

>+static PLDHashOperator
>+EnumerateSubstitution(const nsACString& aKey,
>+                      nsIURI* aURI,
>+                      void* aArg)
>+{
...
>+    ResourceMapping resource = {
>+        nsDependentCString(aKey), uri
>+    };
You get away with this because you know that your keys are all null-terminated, but in general you shouldn't wrap an nsACString in an nsDependentCString.