Goal - I visit https://apps.com/phone.html, a sweet phone app. Me want. - I press the right buttons in the UI to make this replace my current phone app. - When I reboot my phone, my phone app had better be loadable there even if I don't have a data Internet connection. This seems to require: (i) knowing what "fully loaded" means for an app. HTML manifest?; (ii) ensuring all those files are in the app cache; (iii) ensuring those files are never evicted. I suspect this would be pretty easy to implement in current Gecko, depending on (i).

Chris, I understand you wanting this for a phone app, but how about for a game? If what you need is a way to guarantee cache for critical applications (like a phone app) then why not just place that exact restriction on those critical apps? For example your phone app needs to request to become the default. Maybe it calls requestAnswerPermission() which prompts the user to accept or decline. We could simple ignore this function call if the page doesn't contain a cache.manifest file that minimally includes the current page html and executing js as being available offline.

Yes, this feature is intended to be available to any page with the right manifest. Apps that are "installed", using the new APIs being hacking on, should probably be pinned as part of the installation process. For other apps, that the user navigates to through links/URL-bar/etc., I think I lean more towards pinning being an intentional user action instead of a response to a request made by the page itself. The user action could be as simple as "bookmarking" a page is in FF today, press a little button near the URL identifier. In fact we could reuse that bit of UX entirely, make bookmark => pin for apps with manifests. In general, UAs already try to cache pages for performance anyway, and it's not clear that users would be able to intelligently respond to a "Pin: Allow/Deny?" request. It's a somewhat complex concept to communicate concisely, too.

We can make a page and its relative pages never being purged (with maximum expired time). But, how to purge/uninstall an app is important, too. We need a database for pinned apps, more than bookmarking them. The database records URLs of apps and their pages, so we can uninstall any of them later. We also need to consider how to update apps. Either allow users to trigger a refreshing, and allow apps to refresh them-self.

It looks like what is mentioned at https://developer.mozilla.org/en/Offline_resources_in_Firefox . Do I miss any thing?

That's the mechanism we want to use. The additional bit we need is to ensure that resources added to the appcache by installed web apps never get purged from the cache, i.e. are "pinned".

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > That's the mechanism we want to use. The additional bit we need is to > ensure that resources added to the appcache by installed web apps never get > purged from the cache, i.e. are "pinned". Not sure what your "additional bit" means, coz the behavior described follows is already what the AppCache do. For example, if you go to Firefox -> Pref -> Advanced -> Network and click [Clean Now] under "Cached Web Content" section, the cache in Offline AppCache (i.e., website listed in "Offline Web Content and User Data") will not be deleted. I think the thing you want here is just to introduce the Offline AppCache prompt in B2G and Gaia. Anything else than that would involve working with new features similar to the ability to view cached content in "Offline Mode" of Netscape Navigator.

Users being able to forceably clear the cache is one thing. (That I don't particularly care about.) The cache is fixed size (see about:cache --- "Maximum storage size: 512000 KiB"). If the cache fills up, and the user browses to a page P that has an appcache manifest, then gecko has two choices 1. evict old entries from the appcache, install P (likely what happens) 2. fail to install P to appcache We don't want (2) for installed apps (installed through navigator.mozApps). If the user tries to install an app, we want older non-installed offline resources to be evicted to make room for the new app. So that's approach (1). BUT, in (1), what we *don't* want to happen is evicting apps that were installed through navigator.mozApps. The user has explicitly asked that the apps be installed. Removing them should be done by the user, by uninstalling them. If the cache fills up with *installed* apps, so that nothing can be evicted, then we need to fail to install new apps until older ones are uninstalled by the user. Does that make sense?

Should we remove an app if the manifest was removed from origin?

That's a good question. That would be something like the vendor dropping support for the app. For now, I think we should *not* do that. If I pay $5 for a game, and then the vendor goes out of business, I don't want the incidental fact that their website shuts down to remove from all my devices the game that I payed for.

I will take this bug if there is no one to stop me.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9) > That's a good question. That would be something like the vendor dropping > support for the app. > > For now, I think we should *not* do that. If I pay $5 for a game, and then > the vendor goes out of business, I don't want the incidental fact that their > website shuts down to remove from all my devices the game that I payed for. That will break the Offline AppCache standard http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#event-appcache-obsolete If I understand it correctly, the spec states if the browser received 404 or 410 response when attempted to check the manifest, it should remove the application cache. I do agree that Chris's case is a valid use case though.

We're trying to reuse appcache as machinery to support installed applications. Installing an app is slightly different from appcache though, hence the new work here and in the dependent bugs. To take another extreme example, if gaia.org 404's for whatever reason, you don't want your dialer application to be deleted and no longer be able to make emergency calls.

I think we need to find out solutions for potential issues. Maybe to file more bugs. For example, - Provide an option that users can stop updating the applications - Rollback to previous version. For an buggy proxy server or network, we may get a broken manifest and flush out all cached entries. Or, manifest may became empty for some reason. There is no message digest for application manifest files. We need to help people for these non-meaningful or intent occasions.

This bug covers pinning resources to app cache. We can worry about other issues in followup work.

-- Attachment #596483 [details] [diff] - Attachment is obsolete: true

-- Attachment #596484 [details] [diff] - Attachment is obsolete: true

-- Attachment #596485 [details] [diff] - Attachment is obsolete: true

-- Attachment #596482 [details] [diff] - Attachment is obsolete: true

-- Attachment #596568 [details] [diff] - Attachment is obsolete: true

-- Attachment #596569 [details] [diff] - Attachment is obsolete: true

-- Attachment #596570 [details] [diff] - Attachment is obsolete: true

-- Attachment #600243 [details] [diff] - Attachment is obsolete: true

-- Attachment #600245 [details] [diff] - Attachment is obsolete: true

-- Attachment #600246 [details] [diff] - Attachment is obsolete: true

-- Attachment #600247 [details] [diff] - Attachment is obsolete: true

-- Attachment #600248 [details] [diff] - Attachment is obsolete: true

-- Attachment #600249 [details] [diff] - Attachment is obsolete: true

-- Attachment #600250 [details] [diff] - Attachment is obsolete: true

-- Attachment #602633 [details] [diff] - Attachment is obsolete: true

-- Attachment #602635 [details] [diff] - Attachment is obsolete: true

-- Attachment #602636 [details] [diff] - Attachment is obsolete: true

-- Attachment #600244 [details] [diff] - Attachment is obsolete: true

-- Attachment #602630 [details] [diff] - Attachment is obsolete: true

-- Attachment #602631 [details] [diff] - Attachment is obsolete: true

-- Attachment #602632 [details] [diff] - Attachment is obsolete: true

-- Attachment #602634 [details] [diff] - Attachment is obsolete: true

-- Attachment #602715 [details] [diff] - Attachment is obsolete: true

Try run for 868d98d8ae14 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=868d98d8ae14 Results (out of 217 total builds): exception: 2 success: 178 warnings: 37 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-868d98d8ae14

-- Attachment #602716 [details] [diff] - Attachment is obsolete: true

I can't review this code so I'm going to unset myself. Apologies in advance for the bugspam. Thinker, there are a lot of patches here and the problem that's being solved may not be clear to Boris and Jason. Can you write a comment describing what these patches do, and then maybe a sentence or two describing each patch individually? Thanks!

Comment on attachment 602717 [details] [diff] [review] Part 8: Fix testcases using nsIOfflineCacheUpdate, v3 Moving reviews for this bug to Honza Bambas, who knows appcache the best on the necko team. Honza, feel free to reassign/delegate if you need help with these.

Moving this to Core:Networking:Cache both because that's the logical place for it, and because I suspect a lot of folks on the necko team will want to follow it.

Comment on attachment 602720 [details] [diff] [review] Part 3: Implement pinned attribute for SQL cache device (WIP), v4 This patch implements accessors of |nsIApplicationCache::pinned|. It controls whether a is pinned app cache at storage level. It persists the value of |pinned| as one of fields of |moz_cache_groups| table.

Comment on attachment 602721 [details] [diff] [review] Part 4: Special works for pinned app during cache updating (WIP), v4 This patch add the code to deal with pinned app caching for |nsOfflineCacheUpdate|. It does not provide any way to start a pinned app cache here. It assumes callers change the value of |mPinned| to start pinned ones ( although it is impossible here).

Comment on attachment 602723 [details] [diff] [review] Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v4 This patch changes interface nsIOfflineCacheUpdateService that introduce a lot of minor changes for IPC protocols. The major task is to let callers starting pinned app caches through |nsIOfflineCacheUpdateService|. This patch also adds |ALLOW_PINNED| as one of values of "offline-app" perm type. |nsOfflineCachePendingUpdate| checks for this value to decide whether it is a pinned or non-pinned. The applications (browser) are supposed to ask user whether to pin an WEB app and set |ALLOW_PINNED| for pinned WEB apps.

Comment on attachment 602722 [details] [diff] [review] Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP), v3 Fix relative JS code for changes of |nsIOfflineCacheUpdateService|. They start new caches a non-pinned app cache. These code blocks are related to implementation of applications (browsers). They are supposed to revise their code to pass purpose value to start pinned app caching.

those "sentence or two" bits should be in the checkin comments in the patches, no? That's exactly what checkin comments are for! Compare "nsIOfflineCacheUpdateService supports pinned" and comment 53 as checkin comments.... And yes, Honza is a better reviewer for the code here than I am.

(In reply to Boris Zbarsky (:bz) from comment #55) > those "sentence or two" bits should be in the checkin comments in the > patches, no? That's exactly what checkin comments are for! Compare > "nsIOfflineCacheUpdateService supports pinned" and comment 53 as checkin > comments.... It is my misunderstanding about "comment". Update these patches later.

Thinker, I started to look at the patches, so please don't update them or you destroy my draft review comments in splinter. Can you please add an outline of how the process of "pinning" an app will be done from top of the UI? I mean, what the order of operations can be, when what happens etc. Thanks!

Is there a description anywhere what changes are being made to the appcache here. It's awesome you're working on the App cache, and I think we can make it work for the use cases mentioned in this bug. But we should we have for a while been planning some pretty major changes to the app cache and I want to make sure that these changes fit in that. Also, if you'd be available to implement those changes, in addition to the ones planned here, that would be super awesome :-)

(In reply to Jonas Sicking (:sicking) from comment #58) > we have for a > while been planning some pretty major changes to the app cache Bug # ?

Well, we don't have a bug filed yet since we don't know exactly what changes to make. But at the very least: * Make AppCache an actual cache, i.e. don't have a prompt but instead toss out data once we've filled up X MB of total AppCache data (pinned sites would be excluded from this quota). * Ensure that installed OWA apps get "pinned" (i.e. that there's no prompt but that they don't count towards quota). * Figure out a solution for the current "user only sees updated site on second visit" problem. Lots of different strategies we can use here. We'll probably want to use different strategies for pinned sites. I didn't actually realize that anyone else was working on the appcache. The discussion so far has mostly been between me and Bent.

Thanks, I just wanted to be sure there is nothing I wouldn't know about. FYI: I'm a maintainer of that code, or maybe these days more just a reviewer.

(In reply to Jonas Sicking (:sicking) from comment #60) > * Make AppCache an actual cache, i.e. don't have a prompt but instead toss > out data once we've filled up X MB of total AppCache data (pinned sites > would be excluded from this quota). > > * Ensure that installed OWA apps get "pinned" (i.e. that there's no prompt > but that they don't count towards quota). > Yep, this is the goal here. > * Figure out a solution for the current "user only sees updated site on > second visit" problem. Lots of different strategies we can use here. We'll > probably want to use different strategies for pinned sites. > Note sure I understand this.

(In reply to Jonas Sicking (:sicking) from comment #60) > * Make AppCache an actual cache, i.e. don't have a prompt but instead toss > out data once we've filled up X MB of total AppCache data (pinned sites > would be excluded from this quota). This patch set picks and evicts an non-pinned app from cache randomly (most old one actually) for caching a pinned app while we have reached the maximum size of the storage. The rules of picking for evicting can be refined later. (another bug?) > > * Figure out a solution for the current "user only sees updated site on > second visit" problem. Lots of different strategies we can use here. We'll > probably want to use different strategies for pinned sites. I am not sure I understand this, too. Do you mean versioning app caches? User can rollback its apps to elder versions. Since users bought an app from a store, the may not happy to see a broken app caused by an incorrect update of manifest. They should be able to rollback their apps to elder versions.

(In reply to Honza Bambas (:mayhemer) from comment #57) > Can you please add an outline of how the process of "pinning" an app will be > done from top of the UI? I mean, what the order of operations can be, when > what happens etc. For a new app, the browser would receive an "MozApplicationManifest" chrome event from nsDocument. - nsDocument would check root node of the loaded document for "manifest" attribute, and send a chrome event "MozApplicationManifest" if "manifest" is existing. - The browser calls nsContentUtils::OfflineAppAllowed() to check if the app has previously allowed for caching, once the browser have received a "MozApplicationManifest". (see "offline-app" permission. - If the app has allowed, the browser does nothing. - Otherwise, the browser prompts the user for whether to cache/or pin the app. - Once the user agree to cache/or pin an app, - the browser calls nsIOfflineCacheUpdate::scheduleUpdate() to cache/or pin the app. - add permission "offline-app" for the app (URI) with value - ALLOW_ACTION, for a normal caching, or - ALLOW_PINNED, for a pinned app. For an app already in an app cache, the browser would also receive an "MozApplicationManifest" chrome event. But, it does nothing since its URL has an "offline-app" permission (ALLOW_ACTION or ALLOW_PINNED). Intead, nsContentSink will call nsIOfflineCacheUpdate::ScheduleOnDocumentStop() to start an updating. - nsContentSink::ProcessOfflineManifest() checks root node of the loaded document for "manifest" attribute. - If there is an "manifest" attribute, it - calls nsContentUtils::OfflineAppAllowed() for checking if the app have been allowed for caching. - call nsIOfflineCacheUpdate::ScheduleOnDocumentStop() if the app have been allowed. - ScheduleOnDocumentStop() calls nsIOfflineCacheUpdate::offlineAppPinned() to check if the app is pinned. Is it clear?

(In reply to Thinker Li [:sinker] from comment #64) > (In reply to Honza Bambas (:mayhemer) from comment #57) > Is it clear? Yes, thanks. I actually know all of these, I was more interested in what new you were adding to this. I was confused with "pinning" what here is different from what "pinning" means on desktop Firefox (to have a small persistent app tab: http://imagecdn.maketecheasier.com/2010/08/firefox4-app-tab.png) As I understand, the only changes are: - for apps the prompt will change to "do you want to pin" instead of saying just "do you want to cache offline" - domains with PINNED permission will rule with different cache limit priority, i.e. PINNED domain can make ALLOWED domains evict I play with an idea of having a whole new database and cache folder for the PINNED cache instead of messing the ALLOWED and PINNED databases together. This is not thought through very much, though, raising it more to hear your opinion since you've spent a lot of time on these changes already. (In reply to Jonas Sicking (:sicking) from comment #60) > * Figure out a solution for the current "user only sees updated site on > second visit" problem. Lots of different strategies we can use here. We'll > probably want to use different strategies for pinned sites. To explain: an already cached page performs an update check (nsIOfflineCacheUpdate::ScheduleOnDocumentStop()). When an update is found, it is not used until the page is reloaded by the user. We would like to have a way to "update" - what actually means reload - the page immediately. I solved this in my web app with a prompt saying "Update ready, apply now?" [Yes] [No]. On clicking yes I just do location.reload().

(In reply to Honza Bambas (:mayhemer) from comment #65) > (In reply to Thinker Li [:sinker] from comment #64) > > (In reply to Honza Bambas (:mayhemer) from comment #57) > > Is it clear? > > Yes, thanks. I actually know all of these, I was more interested in what > new you were adding to this. I was confused with "pinning" what here is > different from what "pinning" means on desktop Firefox (to have a small > persistent app tab: > http://imagecdn.maketecheasier.com/2010/08/firefox4-app-tab.png) "Pinning" means making an app being cached and never being evicted for the offline cache storage's reaching its maximum size. It is nothing about "Pin as App Tab" of firefox. Do you have a better name? > > As I understand, the only changes are: > - for apps the prompt will change to "do you want to pin" instead of saying > just "do you want to cache offline" > - domains with PINNED permission will rule with different cache limit > priority, i.e. PINNED domain can make ALLOWED domains evict Correct! > > > I play with an idea of having a whole new database and cache folder for the > PINNED cache instead of messing the ALLOWED and PINNED databases together. > This is not thought through very much, though, raising it more to hear your > opinion since you've spent a lot of time on these changes already. Does "a whole new database and cache folder" means to instantiate both nsOfflineCacheUpdateService and nsDiskCacheDevice two times, one for pinned and another for non-pinned? If I am correct, the current nsOfflineCacheUpdate in m-c never evicts old caches. It just stops caching new apps and entries while it is filled. The idea of two instances makes no different between PINNED and ALLOWED except where they are and separated quota. But, it is a fail of current implementation, we can fix it up with evicting entries randomly for ALLOWED apps for reaching maximum size. It is more easy to understand separated devices for PINNED/installed apps and ALLOWED/cached apps for users. Actually, I think users would like to have a unlimited quota for PINNED apps XD

@Honza, is there any update or comment?

(In reply to Thinker Li [:sinker] from comment #67) > @Honza, is there any update or comment? I'll get to this on Monday. Just to note that I probably have left the idea of two databases. It would mean more complication then simplification.

(In reply to Thinker Li [:sinker] from comment #66) > > - for apps the prompt will change to "do you want to pin" instead of saying > > just "do you want to cache offline" > > - domains with PINNED permission will rule with different cache limit > > priority, i.e. PINNED domain can make ALLOWED domains evict > > Correct! Hmm... according this you don't need any of these large patches you've made to implement this feature. To explain how the current (just "offline") implementation works: - ability to cache offline is indicated by "offline-app" permission (ALLOW/DENY/UNKNOWN/ALLOW_NO_WARNING) for the whole DOMAIN - when manifest= attribute is found in the document then the permission is checked for the document's domain - on UNKNOWN - pop up the prompt to allow - on "Allow" click - set the "offline-app" permission to ALLOW - schedule the (first) update to fetch the cache content - on "Don't allow" click obviously do nothing - on ALLOWED - update is scheduled (this may even be the first update, since there may be more offline apps on a single domain) Since you just want some details to change for the special "pinned" case, you may just go the way of permissions and not flags in the database with carrying and setting the pinned flags here and there. If the operation of "Allow" in the prompt is actually to "pin", then just set beside the "offline-app" permission also a new permission "pin-app" to ALLOW for the domain. Updates will read the permission it self using the permission manager. When the permission is set, then just don't obsolete the cache on 40X or do any specific actions you need. The "pinned" information doesn't need to be in the database at all.

(In reply to Honza Bambas (:mayhemer) from comment #69) > (In reply to Thinker Li [:sinker] from comment #66) > > > - for apps the prompt will change to "do you want to pin" instead of saying > > > just "do you want to cache offline" > > > - domains with PINNED permission will rule with different cache limit > > > priority, i.e. PINNED domain can make ALLOWED domains evict > > > > Correct! > > > Hmm... according this you don't need any of these large patches you've made > to implement this feature. > > To explain how the current (just "offline") implementation works: > > - ability to cache offline is indicated by "offline-app" permission > (ALLOW/DENY/UNKNOWN/ALLOW_NO_WARNING) for the whole DOMAIN > - when manifest= attribute is found in the document then the permission is > checked for the document's domain > - on UNKNOWN > - pop up the prompt to allow > - on "Allow" click > - set the "offline-app" permission to ALLOW > - schedule the (first) update to fetch the cache content > - on "Don't allow" click obviously do nothing > - on ALLOWED > - update is scheduled (this may even be the first update, > since there may be more offline apps on a single domain) > > Since you just want some details to change for the special "pinned" case, > you may just go the way of permissions and not flags in the database with > carrying and setting the pinned flags here and there. > > If the operation of "Allow" in the prompt is actually to "pin", then just > set beside the "offline-app" permission also a new permission "pin-app" to > ALLOW for the domain. > > Updates will read the permission it self using the permission manager. > > When the permission is set, then just don't obsolete the cache on 40X or do > any specific actions you need. The "pinned" information doesn't need to be > in the database at all. It means to move all code in the patches from C++ to JS and implemented by applications(browsers) them-self. Right?! It seems not to make thing easier. It just moves the same code around. The benefits are we don't need to change nsIOfflineCacheUpdateService and all things in JS. The disadvantage is every application is responsible for the implementation of "pin-app".

(In reply to Thinker Li [:sinker] from comment #70) > It means to move all code in the patches from C++ to JS and implemented by > applications(browsers) them-self. Right?! It seems not to make thing > easier. It just moves the same code around. > The benefits are we don't need to change nsIOfflineCacheUpdateService and > all things in JS. > The disadvantage is every application is responsible for the implementation > of "pin-app". I think I had mis-undertood your point. After the explanation of fabrice, I think you like to remove all flag from db and check permission manager instead of "pinned" flag in DB.

I am working on removing pinned flag from DB and querying permission manager instead. But, I think there are two disadvantages on this solution. 1. We should query permission manager everywhere that is not so efficient. 2. Offline cache will be tightly coupled with a global state at permission manager. It means to query permission manager for every cached group, one by one, to pick a group for eviction. It is inefficient to scan all cache groups from head to tail for a non-pinned while most apps in the cache are pinned.

(In reply to Thinker Li [:sinker] from comment #73) > Created attachment 607921 [details] [diff] [review] > Use pin-app permission at permission manager This patch remove "pinned" flag from DB, and ask permission manager for "pin-app" permission. The testcase test_pinned_app_cache.js have passed. I will re-organize all patches later.

Thinker, I'm looking at the patches now. Thanks for the update. It is much simpler. I will have few comments to change few things, but not anything major (this time). One of them is that I can see a major problem with discarding an existing cache group. Doing it on the main thread will kill (unexpectedly, means hard to track) the main thread. We cannot do that. Also, I don't see much reason to do that, as you say your self there will be just few non-pinned offline apps, so the affect may be just little. Based on that I think the solution here is to allow larger space for the whole offline cache capacity. I don't see that done in the patch. We may change the pref for b2g app as a workaround (or already done in a different bug?). Thinking of a more correct solution, this carries me back to introducing a new database. We would have a new storage policy STORE_OFFLINE_PINNED and a new instance of nsOfflineCacheDevice working with a different physical directory on the device disk and having (mainly!) a different mCacheCapacity setting. I have to think about this more deeply, but probably having this in a followup is sufficient and finally may not be that hard to implement. (Please don't update the patch yet, there will be more comments.)

Comment on attachment 608253 [details] [diff] [review] Part 1: Pinned apps get higher priority for cache. Review of attachment 608253 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1368,5 @@ > mObsolete = true; > if (mPreviousApplicationCache) { > + if (mPinned) { > + // Do not obsolete a pinned application. > + NotifyState(nsIOfflineCacheUpdateObserver::STATE_NOUPDATE); If you don't want to obsolete the cache, then don't set mObsolete flag. Notification is OK but not enough. (mObsolete = !mPinned) @@ +1431,5 @@ > rv = item->GetRequestSucceeded(&succeeded); > > + PRUint32 dummy_cache_type; > + rv = mApplicationCache->GetTypes(item->mCacheKey, &dummy_cache_type); > + bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed We should have an IsContained method for this. Followup is OK for that. @@ +1435,5 @@ > + bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed > + > + while (mPinned && item_doomed && > + (item->mItemType & (nsIApplicationCache::ITEM_EXPLICIT | > + nsIApplicationCache::ITEM_FALLBACK))) { why are you checking for item->mItemType & (nsIApplicationCache::ITEM_EXPLICIT | nsIApplicationCache::ITEM_FALLBACK) ? @@ +1442,5 @@ > + > + rv = EvictOneNonPinned(); > + if (NS_FAILED(rv)) break; > + rv = RescheduleUpdate(); > + if (NS_FAILED(rv)) break; You don't need to reschedule the whole update, just re-fetch the current item again. You need to introduce a new counter or, check there actually has been some data removed. @@ +1843,5 @@ > return rv; > } > > +nsresult > +nsOfflineCacheUpdate::EvictOneNonPinned() { Style is: funcName() { } @@ +1863,5 @@ > + > + for (i = 0; i < count; i++) { > + bool pinned; > + nsDependentCString group_name(groups[i]); > + nsCOMPtr<nsIURI> uri; We're used to declare stuff on/close to its first use. @@ +1870,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = cacheService->GetActiveCache(group_name, getter_AddRefs(cache)); > + if (NS_FAILED(rv)) { > + break; continue; ? @@ +1877,5 @@ > + rv = nsOfflineCacheUpdateService::OfflineAppPinned(uri, &pinned); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (!pinned) { > + rv = cache->Discard(); This is a big problem: on the main thread we may cause a large unexpected and not to any current user action bound (means hard to track) hang. This must be done asynchronously or must not be done at all. It would mean to make the space for pinned apps just larger. ::: uriloader/prefetch/nsOfflineCacheUpdate.h @@ +310,5 @@ > PRUint32 mRescheduleCount; > > nsRefPtr<nsOfflineCacheUpdate> mImplicitUpdate; > + > + bool mPinned; Initialize this in the constructor. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp @@ +591,5 @@ > } > + > +nsresult > +nsOfflineCacheUpdateService::OfflineAppPinned(nsIURI *aDocumentURI, > + bool *aPinned) This method should share the code with nsOfflineCacheUpdateService::OfflineAppAllowedForURI and the name should be nsOfflineCacheUpdateService::OfflineAppPinnedForURI

Comment on attachment 608253 [details] [diff] [review] Part 1: Pinned apps get higher priority for cache. Review of attachment 608253 [details] [diff] [review]: ----------------------------------------------------------------- You should also discard the cache with the oldest fetch time. ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1866,5 @@ > + nsDependentCString group_name(groups[i]); > + nsCOMPtr<nsIURI> uri; > + > + rv = ios->NewURI(group_name, NULL, NULL, getter_AddRefs(uri)); > + NS_ENSURE_SUCCESS(rv, rv); And you have NS_NewURI for this.

(In reply to Honza Bambas (:mayhemer) from comment #77) > Thinking of a more correct solution, this carries me back to introducing a > new database. We would have a new storage policy STORE_OFFLINE_PINNED and a > new instance of nsOfflineCacheDevice working with a different physical > directory on the device disk and having (mainly!) a different mCacheCapacity > setting. I have to think about this more deeply, but probably having this > in a followup is sufficient and finally may not be that hard to implement. > I agree with you. It would be better to have a separated database for pinned apps in a follow up. And, I also think a separated database is a more correct solution.

(In reply to Honza Bambas (:mayhemer) from comment #78) > Comment on attachment 608253 [details] [diff] [review] > Part 1: Pinned apps get higher priority for cache. > > Review of attachment 608253 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp > @@ +1368,5 @@ > > mObsolete = true; > > if (mPreviousApplicationCache) { > > + if (mPinned) { > > + // Do not obsolete a pinned application. > > + NotifyState(nsIOfflineCacheUpdateObserver::STATE_NOUPDATE); > > If you don't want to obsolete the cache, then don't set mObsolete flag. > Notification is OK but not enough. (mObsolete = !mPinned) You are right! It will obsolete the cache for conditions other than mPreviousApplicationcache && mPinned in next update. > > @@ +1431,5 @@ > > rv = item->GetRequestSucceeded(&succeeded); > > > > + PRUint32 dummy_cache_type; > > + rv = mApplicationCache->GetTypes(item->mCacheKey, &dummy_cache_type); > > + bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed > > We should have an IsContained method for this. Followup is OK for that. I will file a followup bug. > > @@ +1435,5 @@ > > + bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed > > + > > + while (mPinned && item_doomed && > > + (item->mItemType & (nsIApplicationCache::ITEM_EXPLICIT | > > + nsIApplicationCache::ITEM_FALLBACK))) { > > why are you checking for item->mItemType & > (nsIApplicationCache::ITEM_EXPLICIT | nsIApplicationCache::ITEM_FALLBACK) ? Should an update fail for URIs that are not specified in a manifest explicitly? This checking make sure an update does not fail for the error of loading an URI that is not specified in the manifest explicitly.

-- Attachment #608253 [details] [diff] - Attachment is obsolete: true

For pinned apps, we may be able to rollback to earlier versions while latest version is broken. People would not like to stay at broken version for an accident on manifest or server. So, it would be better to give user a chance to rollback an app to an earlier revision.

-- Attachment #608254 [details] [diff] - Attachment is obsolete: true

-- Attachment #609193 [details] [diff] - Attachment is obsolete: true

Comment on attachment 608653 [details] [diff] [review] Part 1: Pinned apps get higher priority for cache., v2 Review of attachment 608653 [details] [diff] [review]: ----------------------------------------------------------------- r=0, please let me check the final version ones more. But this looks good. ::: netwerk/base/public/nsIApplicationCacheService.idl @@ +97,5 @@ > + /** > + * Get the list of application cache groups in the order of > + * activating time. > + */ > + void getGroupsTimeOrder([optional] out unsigned long count, Ordered ? ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1302,5 @@ > if (!succeeded || !mManifestItem->ParseSucceeded()) { > return NS_ERROR_FAILURE; > } > > + if (mRescheduleCount == 0 && !mManifestItem->NeedsUpdate()) { Probably don't need mRescheduleCount == 0 in the cond anymore? @@ +1451,5 @@ > + > + rv = item->Cancel(); > + if (NS_FAILED(rv)) break; > + > + mPinnedEntryRetriesCount++; Reset this on successful load. @@ +1452,5 @@ > + rv = item->Cancel(); > + if (NS_FAILED(rv)) break; > + > + mPinnedEntryRetriesCount++; > + mCurrentItem--; Maybe just ++ this after this block and add a comment here that ProcessNextURI will process the current one since we didn't move the counter? @@ +1456,5 @@ > + mCurrentItem--; > + ProcessNextURI(); > + > + return; > + } BTW, I really don't like this while cycle. I understand it and sometimes miss a break from if my self, but please move this to a separate method returning true when eviction succeeded or something like that. @@ +1507,5 @@ > NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR); > } > > + if (NS_FAILED(aStatus)) { > + nsresult rv = RescheduleUpdate(); Personally, I think this change (new method) is not need anymore. Please remove it to keep the patch and reviewing it as simple as possible. @@ +1878,5 @@ > + &pinned); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (!pinned) { > + rv = cache->Discard(); This needs a followup fixed before any (pre)production public release. We must not do this extremely expensive operation on the main thread. ::: uriloader/prefetch/nsOfflineCacheUpdate.h @@ +263,5 @@ > + * Restart this update. > + * > + * Return error for retring for a maximum times. > + */ > + nsresult RescheduleUpdate(); Probably remove this. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp @@ +573,3 @@ > > if (perm == nsIPermissionManager::UNKNOWN_ACTION) { > static const char kPrefName[] = "offline-apps.allow_by_default"; This means offline-apps.allow_by_default = true will allow also pinning automatically, I trust you know what you're doing. But I would personally don't do that...

Comment on attachment 608989 [details] [diff] [review] Part 2: Testcase for pinned application caches, v6 Can you please write a summary (also to the test file directly) of what the test is doing? It makes understanding it easier (also for you). Thanks.

-- Attachment #608653 [details] [diff] - Attachment is obsolete: true

-- Attachment #608989 [details] [diff] - Attachment is obsolete: true

-- Attachment #609280 [details] [diff] - Attachment is obsolete: true

(In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > (iii) ensuring those files are never evicted. > > I suspect this would be pretty easy to implement in current Gecko, depending > on (i). Although it seems simple to implement, there is a huge limitation: when Gecko crashes, the cache is reset. This makes the cache totally unusable as an app storage mechanism for mobile devices, AFAICT. The Necko cache is optimized for avoiding disk I/O (avoiding fsyncs and the like), and is not optimized for persistence. This fundamental design choice is going to cause all kinds of problems if we try to rely on our cache as an app storage mechanism. There is not even a plan yet as to how to rectify it--i.e. it would be extremely optimistic to expect a solution in Q2 if things go according to current plans.

(In reply to Brian Smith (:bsmith) from comment #92) > Although it seems simple to implement, there is a huge limitation: when > Gecko crashes, the cache is reset... Brian, you are talking about HTTP "normal" cache. This bug is all about using offline app cache. There it is different.

Comment on attachment 609646 [details] [diff] [review] Part 1: Pinned apps get higher priority for cache, v3 Review of attachment 609646 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with some last comments addressed. I think we may land this now to start playing with this new feature. To solve some other requirements like capacity and also eviction of some lower priority stuff, we have to introduce the secondary instance of the database with a different quota limit. ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1439,5 @@ > rv = item->GetRequestSucceeded(&succeeded); > > + PRUint32 dummy_cache_type; > + rv = mApplicationCache->GetTypes(item->mCacheKey, &dummy_cache_type); > + bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed You should actually do this under if (mPinned) {} since this may be expensive and you don't need it for otherwise. This could regress on main thread perf for Fx mobile and desktop. @@ +1462,5 @@ > + return; > + } > + > + mPinnedEntryRetriesCount++; > + // Do a retrying for current item, so mCurrentItem is not advanced. Just s/Do a retrying for/Retry/ ? @@ +1902,5 @@ > + nsCOMPtr<nsIApplicationCache> cache; > + rv = cacheService->GetActiveCache(group_name, getter_AddRefs(cache)); > + // Maybe someone in another thread or process have deleted it. > + if (NS_FAILED(rv)) > + continue; Err... this check is not enough. The result may be NS_OK + cache = null. Add also "|| !cache" to the condition.

Comment on attachment 609646 [details] [diff] [review] Part 1: Pinned apps get higher priority for cache, v3 Review of attachment 609646 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1888,5 @@ > } > > +static nsresult > +EvictOneOfCacheGroups(nsIApplicationCacheService *cacheService, > + PRUint32 count, const char * const *groups) { One more comment - the style is: funcName() { } Braces on new lines.

Comment on attachment 609647 [details] [diff] [review] Part 2: Testcase for pinned application caches, v7 Review of attachment 609647 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab but address the following comments please: The test takes 26 seconds (debug build) on a very fast i7 machine with ssd disk and a tun of memory. It is unacceptable. Please lower the limits by a magnitude. I think the limit for 10k and each page having 1k will work well too. Then, I've seen this in the test log: TEST-PASS | undefined | [undefined : undefined] true == true Just before "Start pinned App2 for success" TEST-INFO output. Looks like there is something wrong with do_check_true on line 218. ::: netwerk/test/unit/test_pinned_app_cache.js @@ +4,5 @@ > + * are > + * > + * - start_cache_nonpinned_app1() > + * > + * - Request nsOfflineCacheService to skep pages (4) of app1 on to "skep" ? maybe just a gap in my english.. @@ +58,5 @@ > +const kHttpLocation_ip = "http://127.0.0.1:4444/"; > + > +function manifest1_handler(metadata, response) { > + do_print("manifest1\n"); > + response.setHeader("content-type", "text/cache-manifest"); "text/cache-manifest" is no longer needed. @@ +82,5 @@ > + do_print("datafile_handler\n"); > + let data = ""; > + > + while(data.length < kDataFileSize) { > + data = data + Math.random().toString(36).substring(0, 8); Nice :), maybe just 2, 15 for substring. @@ +124,5 @@ > + getService(Ci.nsICacheService); > + cache_service.evictEntries(Ci.nsICache.STORE_OFFLINE); > +} > + > +function do_app_cache(manifestURL, pageURL, pinned) { maybe call this start_app_cache_update @@ +156,5 @@ > + > + return update; > +} > + > +function start_n_watch_app_cache(manifestURL, s/_n_/_and_/ ? @@ +191,5 @@ > + > + case STATE_ERROR: > + do_throw_todo("App1 cache state = " + state); > + break; > + } NOUPDATE and OBSOLETE should be considered unexpected here and you should do_throw for them. @@ +215,5 @@ > + function (update, state) { > + switch(state) { > + case STATE_FINISHED: > + do_check_true(error_count[0] > 0, > + "This request should run out of the cache storage"); Do you think we can do error_count[0] == 1 ? @@ +222,5 @@ > + > + case STATE_ERROR: > + error_count[0]++; > + break; > + } Same for NOUPDATE and OBSOLETE here.

Comment on attachment 609648 [details] [diff] [review] Part 3: Evict cache asynchronized, v2 I'll review this later. I also think we may not need this patch at all, since discarding may be removed when we have a second space-unlimited database.

-- Attachment #609646 [details] [diff] - Attachment is obsolete: true

-- Attachment #609647 [details] [diff] - Attachment is obsolete: true

Comment on attachment 610054 [details] [diff] [review] Part 1: Pinned apps get higher priority for cache, v4 Review of attachment 610054 [details] [diff] [review]: ----------------------------------------------------------------- You've added a lot of white space only changes, please remove them before landing.

Comment on attachment 610055 [details] [diff] [review] Part 2: Testcase for pinned application caches, v8 Review of attachment 610055 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/test/unit/test_pinned_app_cache.js @@ +180,5 @@ > + */ > +function start_cache_nonpinned_app() { > + do_print("Start non-pinned App1"); > + start_and_watch_app_cache(kHttpLocation + "app1.appcache", > + kHttpLocation + "app1", Indention.

Thinker, I think you can move the part 3 patch to a followup bug and just land these two with the last few nits and let this bug be closed.

Try run for cdbccb5c015c is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=cdbccb5c015c Results (out of 221 total builds): exception: 2 success: 183 warnings: 36 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-cdbccb5c015c

(In reply to Brian Smith (:bsmith) from comment #92) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > > (iii) ensuring those files are never evicted. > > > > I suspect this would be pretty easy to implement in current Gecko, depending > > on (i). > > Although it seems simple to implement, there is a huge limitation: when > Gecko crashes, the cache is reset. This makes the cache totally unusable as > an app storage mechanism for mobile devices, AFAICT. > Is the app cache cleared when gecko crashes? That would be annoying, and we should fix it, but it's not a showstopper. We can recover core cached apps from a pristine read-only package that will be stashed away on device, and we can recover web apps next time network is up. > The Necko cache is optimized for avoiding disk I/O (avoiding fsyncs and the > like), and is not optimized for persistence. This fundamental design choice > is going to cause all kinds of problems if we try to rely on our cache as an > app storage mechanism. There is not even a plan yet as to how to rectify > it--i.e. it would be extremely optimistic to expect a solution in Q2 if > things go according to current plans. If this is true for app cache, then we very much need to fix it. There is a fair amount of this kind of stuff in gecko we need to fix as well :/.

Try run for 75aa7826a464 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=75aa7826a464 Results (out of 219 total builds): exception: 22 success: 151 warnings: 30 failure: 16 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-75aa7826a464

-- Attachment #610054 [details] [diff] - Attachment is obsolete: true

Try run for ff31dab19922 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ff31dab19922 Results (out of 221 total builds): exception: 3 success: 197 warnings: 21 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-ff31dab19922

(In reply to Chris Jones [:cjones] [:warhammer] from comment #104) In general, Brian was describing the "normal" HTTP web content cache, not offline cache. > Is the app cache cleared when gecko crashes? No. Only the sqlite db could potentially be damaged, but I have to check the level of sync on the connection. > If this is true for app cache, then we very much need to fix it. There is a > fair amount of this kind of stuff in gecko we need to fix as well :/. I don't think it's true but some investigation should be taken to find out what may not be in favor with what we need for B2G.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd005fde96d https://hg.mozilla.org/integration/mozilla-inbound/rev/dd0291644c67 https://hg.mozilla.org/integration/mozilla-inbound/rev/a69635b22490 Had to un-bitrot part 3 a bit, FWIW.

And a bustage fix since I didn't notice the double braces when un-bitrotting. https://hg.mozilla.org/integration/mozilla-inbound/rev/db5dbc168429

Thinker, see comment 102. I've never R+'ed the part 3 patch. Back it out ASAP!

Part 3 backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/ee28936ed0ab

Marking fixed since comment 102 suggests moving part 3 to another bug. https://hg.mozilla.org/mozilla-central/rev/cdd005fde96d https://hg.mozilla.org/mozilla-central/rev/dd0291644c67 https://hg.mozilla.org/mozilla-central/rev/a69635b22490

Let's try again. (This replaces comment 113) Part 1: https://hg.mozilla.org/mozilla-central/rev/cdd005fde96d Part 2: https://hg.mozilla.org/mozilla-central/rev/dd0291644c67 Bustage fix: https://hg.mozilla.org/mozilla-central/rev/db5dbc168429 (And part 3 has been backed out since un-reviewed and will be moved to another bug)

Is there anything testable in this feature from an end-user perspective?