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)
Tracking
()
People
(Reporter: sicking, Assigned: mayhemer)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
(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.
Reporter | ||
Comment 1•12 years ago
|
||
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?
Reporter | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Reporter | ||
Comment 5•12 years ago
|
||
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?
![]() |
Assignee | |
Comment 6•12 years ago
|
||
(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.
![]() |
Assignee | |
Comment 7•12 years ago
|
||
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)
Reporter | ||
Comment 8•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 9•12 years ago
|
||
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.
Reporter | ||
Comment 10•12 years ago
|
||
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.
Reporter | ||
Comment 11•12 years ago
|
||
Jason: review ping?
Reporter | ||
Updated•12 years ago
|
Priority: -- → P1
Comment 12•12 years ago
|
||
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 13•12 years ago
|
||
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 = ...
![]() |
Assignee | |
Comment 14•12 years ago
|
||
(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
![]() |
Assignee | |
Comment 15•12 years ago
|
||
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+
Comment 16•12 years ago
|
||
I tried to apply it without success on m-c/m-a. (conflicts)
Comment 17•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 18•12 years ago
|
||
Comment on attachment 674739 [details] [diff] [review] v1.1 https://hg.mozilla.org/integration/mozilla-inbound/rev/113d5069e67e
Attachment #674739 -
Flags: checkin+
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/113d5069e67e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Reporter | ||
Comment 20•12 years ago
|
||
Could someone elaborate on hoe risky this would be to take on mozilla-aurora?
![]() |
Assignee | |
Comment 21•12 years ago
|
||
(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.
Comment 22•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/be0ce9f1a08d
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Updated•12 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•