Closed Bug 794663 Opened 12 years ago Closed 12 years ago

Allow downloading appcache for a specific appid/browserflag

Categories

(Core :: Networking: Cache, defect, P1)

Product:
Core
Component:
Networking: Cache
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: sicking, Assigned: mayhemer)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

v1.1
(deleted), patch
mayhemer
: review+
mayhemer
: checkin+
Details | Diff | Splinter Review 
When we are downloading and updating a hosted app, we don't have a window or nsILoadContext which contains the appid/browserflag which is downloaded.

So we need an API like:

nsIOfflineCacheUpdateService.
  scheduleUpdate(manifestURL, documentURL, appid, inbrowser);


This should cause the cache associated with <manifestURL, appid, inbrowser> group to be updated. And all requests should be done using cookies from the <appid, inbrowser> cookie-jar.
Oh, another thing I forgot here. After an update has been started, we need to be able to get the nsIOfflineCacheUpdate object for the update. I assume that we'd do that by simply having new scheduleUpdate function return the nsIOfflineCacheUpdate object, like the current scheduleUpdate?
blocking-basecamp: --- → ?
Affects installation of third party apps.
blocking-basecamp: ? → +
Honza, can you take this?
Assignee: nobody → honzab.moz
Sure, deadline?
Status: NEW → ASSIGNED
It's a bit up to you when you have time. It's one of the earliest step in a pretty long dependency list for getting hosted apps working end-to-end, but I don't want to set timelines which are too tight.

Would it be possible to have this week? Or mid next week?
(In reply to Jonas Sicking (:sicking) from comment #5)
> Would it be possible to have this week? Or mid next week?

I'll do my best.  I think it is doable.
Attached patch v1 (obsolete) (deleted) — — Splinter Review 
There are some more API changes needed for this feature.  Tested only locally (mochi+xpc).  Fennec for desktop is completely broken (crashes), so I cannot check the e10s parts :(
Attachment #667275 - Flags: review?(jduell.mcbugs)
I'm sorry! The intent was that this was just needed in the parent process. I know we discussed this, but we did so in the other bug so it might not have been clear that it still applied here?
Of course, if it works in child processes too then that is ok. But it's not something we will rely on.
First, I was tired when writing comment 7, so it should better stand as: 

"I made some changes touching e10s that might influence normal existing functionality but I was not able to check I didn't break anything."


Then, to make this (or actually anything about offline cache updates) work also from the child process, I just and only need to reach TabChild to be able to create the child/parent e10s machinery.  We usually do that by QI/GI a dom window given.  There may also be some other way to get it, though, I'm not just aware of it.
Awesome! Just wanted to make sure that you knew our requirements as to make sure that you knew which options were available.

Makes perfect sense.
Blocks: 796278
Blocks: 804495
Comment on attachment 667275 [details] [diff] [review]
v1

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

Looks good except for one error, I think.

+r (no re-review needed) if you just need to set mAppId/mInBrowser.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ -1276,5 @@
>                                                               &mPinned);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    mLoadContext = aLoadContext;
> -

hmm, so you're not replacing the setting of mLoadContext with setting mAppId/mInBrowser.  IIUC looks like you need to do that here (and possibly in InitForUpdateCheck?), just as you do in OfflineCacheUpdateChild.   As it is now, you're never setting these variables except in the constructor to default values, and that looks like an error.

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +89,5 @@
> +    if (!aWindow)
> +        return NS_OK;
> +
> +    nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aWindow);
> +    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(webNav);

Do we need the two-step dance here, or can we just do_GetInterface(nsILoadContext) directly?  Sicking tells me he'd be ok with that (assuming it works).  Seems like this is just a matter of special-casing nsGlobalWindow::GetInterface(nsILoadContext) to do the same FORWARD_TO_OUTER trick that it does for many other interfaces.  I've filed bug 804495 for this.  It doesn't need to block this bug.
Attachment #667275 - Flags: review?(jduell.mcbugs) → review-
Comment on attachment 667275 [details] [diff] [review]
v1

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

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +86,5 @@
> +    *aAppId = NECKO_NO_APP_ID;
> +    *aInBrowser = false;
> +
> +    if (!aWindow)
> +        return NS_OK;

{}

@@ +91,5 @@
> +
> +    nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aWindow);
> +    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(webNav);
> +    if (!loadContext)
> +        return NS_OK;

{}

@@ +94,5 @@
> +    if (!loadContext)
> +        return NS_OK;
> +
> +    nsresult rv;
> +    rv = loadContext->GetAppId(aAppId);

nsresult rv = ...
(In reply to :Ms2ger from comment #13)
> > +    nsresult rv;
> > +    rv = loadContext->GetAppId(aAppId);
> 
> nsresult rv = ...

No.  Sorry, I really don't like this styling:
- it hides the definition of rv among lines since rv is usually used many times in a function body
- you need to modify more lines when you want to add some rv = ... code before this line
- you may by accident redefine rv when out of the block and you don't get a compiler error
- it prolongs the line
Attached patch v1.1 (deleted) — — Splinter Review 
Good catch with the missing assignment, seems to be wrong and is the only thing that needs an update in the patch.

Carrying r+.
Attachment #667275 - Attachment is obsolete: true
Attachment #674739 - Flags: review+
I tried to apply it without success on m-c/m-a. (conflicts)
It needs to be applied on top of bug 751754.   Honza generally prefers to land his own stuff (he often has a long queue of dependent patches and landing them out of order will break his queue).  I emailed him to ask if we can land both now--they seem ready.
https://hg.mozilla.org/mozilla-central/rev/113d5069e67e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Could someone elaborate on hoe risky this would be to take on mozilla-aurora?
(In reply to Jonas Sicking (:sicking) from comment #20)
> Could someone elaborate on hoe risky this would be to take on mozilla-aurora?

This one is a bit more risky then patch for bug 751754, but still, if it sticks with m-c withing few days, then there is no reason not to land it on m-a as well.
Whiteboard: [qa-]
Depends on: 808036
Depends on: 808175
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: