nsOfflineCacheUpdateService is chrome process only. Actually TabParent doesn't handle any offline cache related thing so it make sense to move it to PContent.

Hi Kan-Ru, I think this bug is depended on bug 1020157, because the constructor of OfflineCacheUpdateParent needs OwnOrContainingAppId from ContentParent. I would like to work this after bug 1020157 is landed. What do you think? Thanks.

(In reply to Kershaw Chang [:kershaw] from comment #1) > Hi Kan-Ru, > > I think this bug is depended on bug 1020157, because the constructor of > OfflineCacheUpdateParent needs OwnOrContainingAppId from ContentParent. > I would like to work this after bug 1020157 is landed. What do you think? > > Thanks. Bug 1020157 has got all r+'ed but it depends on 1029352 so it may take sometime to land. I think you could go ahead and create patches for this bug with assumption there will be OwnOrContainingAppId in ContentParent.

Patch V1.

Basically, it's just a straight move from PBrowser to PContent. Note that we assume that OwnOrContainingAppId is already available in ContentParent (see also bug 1020157).

Comment on attachment 8475747 [details] [diff] [review] Make POfflineCacheUpdate be managed by PContent - v1 Review of attachment 8475747 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +3660,5 @@ > + nsRefPtr<mozilla::docshell::OfflineCacheUpdateParent> update = > + new mozilla::docshell::OfflineCacheUpdateParent(OwnOrContainingAppId(), > + mIsForBrowser); > + // Use this reference as the IPDL reference. > + return update.forget().take(); This assumes the one-process-one-app relationship. I think this will break when a process could contain multiple App as in bug 1020157. We should check that and maybe forbid offline cache?

Try result: https://tbpl.mozilla.org/?tree=Try&rev=d4a02ae933da

(In reply to Kershaw Chang [:kershaw] from comment #6) > Try result: > https://tbpl.mozilla.org/?tree=Try&rev=d4a02ae933da I found some test cases failed due to the patch from bug 1020157. Please also take a look at the try run that only includes the patch from bug 1020157: https://tbpl.mozilla.org/?tree=Try&rev=d6dd1abaf6e2 After some investigation, I found mOpener should be also explicitly initialized to nullptr for the constructor used for nuwa case, otherwise b2g process will crash when opening the browser app. Kan-Ru, Could you please add the following change into your patch in bug 1020157? @@ -1915,16 +1915,17 @@ FindFdProtocolFdMapping(const nsTArray<P * * For Nuwa. */ ContentParent::ContentParent(ContentParent* aTemplate, const nsAString& aAppManifestURL, base::ProcessHandle aPid, const nsTArray<ProtocolFdMapping>& aFds) : mAppManifestURL(aAppManifestURL) + , mOpener(nullptr) , mIsForBrowser(false) , mIsNuwaProcess(false) { InitializeMembers(); // Perform common initialization. sContentParents->insertBack(this); // From this point on, NS_WARNING, NS_ASSERTION, etc. should print out the Thanks.

(In reply to Kershaw Chang [:kershaw] from comment #7) > (In reply to Kershaw Chang [:kershaw] from comment #6) > > Try result: > > https://tbpl.mozilla.org/?tree=Try&rev=d4a02ae933da > > I found some test cases failed due to the patch from bug 1020157. Please > also take a look at the try run that only includes the patch from bug > 1020157: https://tbpl.mozilla.org/?tree=Try&rev=d6dd1abaf6e2 > > After some investigation, I found mOpener should be also explicitly > initialized to nullptr for the constructor used for nuwa case, otherwise b2g > process will crash when opening the browser app. > > Kan-Ru, > Could you please add the following change into your patch in bug 1020157? > > @@ -1915,16 +1915,17 @@ FindFdProtocolFdMapping(const nsTArray<P > * > * For Nuwa. > */ > ContentParent::ContentParent(ContentParent* aTemplate, > const nsAString& aAppManifestURL, > base::ProcessHandle aPid, > const nsTArray<ProtocolFdMapping>& aFds) > : mAppManifestURL(aAppManifestURL) > + , mOpener(nullptr) > , mIsForBrowser(false) > , mIsNuwaProcess(false) > { > InitializeMembers(); // Perform common initialization. > > sContentParents->insertBack(this); > > // From this point on, NS_WARNING, NS_ASSERTION, etc. should print out > the > > Thanks. This is fixed in the Nuwa bug 1040561 and followup bug 1057230, they should be land together.

Hi Honza, Fabrice, May I have your feedbacks about this patch? Thanks.

Comment on attachment 8477321 [details] [diff] [review] Make POfflineCacheUpdate be managed by PContent - v2 Review of attachment 8477321 [details] [diff] [review]: ----------------------------------------------------------------- I guess we're doing that for the nested OOP use cases? If so, I wonder how that fits schedule-wise with our plan to deprecate appcache support.

I think Kan-Ru knows more specific details than me. Kan-Ru, would you please help to answer Fabrice's question?

(In reply to Fabrice Desré [:fabrice] from comment #10) > Comment on attachment 8477321 [details] [diff] [review] > Make POfflineCacheUpdate be managed by PContent - v2 > > Review of attachment 8477321 [details] [diff] [review]: > ----------------------------------------------------------------- > > I guess we're doing that for the nested OOP use cases? If so, I wonder how > that fits schedule-wise with our plan to deprecate appcache support. By deprecating appcache do you mean we will stop exposing appache to the web content? I think this is not a question that I can answer..

I think we only have to disable appcache temporary only for the cases that dom.ipc.reuse_parent_app is enabled. When we can get the correct app id from OwnOrContainingAppId in ContentParent, we can file another bug for reenabling appcache. Please correct me if I am wrong. Thanks.

Well, that's really annoying. Breaking an app that wants to use appcache based on something that is totally undetectable is not good. Let's do the things in the right order instead, fixing OwnOrContainingAppId and then landing this one.

Use feature-b2g:2.2? rather than just 2.2.

Hi Kyle, Could you take a look at this patch? It's about changing one API in ContentProcessManager to get a TabContext by the given ContentProcess and Tab ID. Thanks.

Hi Honza, Please take a look at this patch. Since bug 1020172 is landed, we can get the correct app id in ContentParent now. Thanks.

Comment on attachment 8518012 [details] [diff] [review] Patch1: Revise the API in ContentProcessManager - v1 Review of attachment 8518012 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8518023 [details] [diff] [review] Patch 2: Make POfflineCacheUpdate be managed by PContent - v3 Review of attachment 8518023 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/PContent.ipdl @@ +799,5 @@ > + * True if the update was initiated by a document load that referred > + * a manifest. > + * False if the update was initiated by applicationCache.update() call. > + * @param tabId > + * To identify which tab owns the app. Put this comment to the bottom of this whole comment block. And next time read more carefully ;) ::: uriloader/prefetch/OfflineCacheUpdateChild.cpp @@ +435,5 @@ > // a reference to us. Will be released in RecvFinish() that identifies > // the work has been done. > + ContentChild::GetSingleton() > + ->SendPOfflineCacheUpdateConstructor(this, manifestURI, documentURI, > + stickDocument, child->GetTabId()); I'd slightly more prefer the following formatting: CCh::GS()->SPOCUC( this, mani..., stick... );

Rebase and carry reviewer's name.

Thanks for Honza's feedback. Rebase and carry reviewer's name. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2cba8a409556

https://hg.mozilla.org/integration/mozilla-inbound/rev/536546ad1bd9 https://hg.mozilla.org/integration/mozilla-inbound/rev/cea30ee39b92

https://hg.mozilla.org/mozilla-central/rev/536546ad1bd9 https://hg.mozilla.org/mozilla-central/rev/cea30ee39b92