916052 - HTTP cache v2: Make about:cache work

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

about:cache will be intact when the old back-end will be preferred.  But will give 0 (or not realistic view of) items when new back-end will be preferred, since about:cache is still using old cache visiting API directly.

This is more then just moving to the new API blindly.  Since the structure of storages has changed (improved) and we can now easily distinguish by [private X anon X app X inbrowser] while [memory vs disk] becomes just a flag (or feature) of an entry, the structure of about:cache has to change accordingly.

I personally vote for making it work reliably only with the new cache backend, and only in a limited way (with some explanation) for the old backend.


Another way is to just drop support for about:cache completely, but something tells me we need an insight tool for the cache and people are used to this feature.

Comment 1

[Patrick McManus [:mcmanus]]

preferences->network->cached web content looks like it needs updating too. Right now it says "your web content cache is currently using 909KB of disk space" and that's not within several orders of magnitude of true for either the old or new cache in my profile.

/home/mcmanus/.cache/mozilla/firefox/1sj58jfj.default>du --si -s Cache cache2
374M	Cache
1.1G	cache2

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #1)
> preferences->network->cached web content looks like it needs updating too.
> Right now it says "your web content cache is currently using 909KB of disk
> space" and that's not within several orders of magnitude of true for either
> the old or new cache in my profile.
> 
> /home/mcmanus/.cache/mozilla/firefox/1sj58jfj.default>du --si -s Cache cache2
> 374M	Cache
> 1.1G	cache2

That's bug 915296 that is more general - about fixing the API implementation.  This bug is about migrating about:cache to the new API only.

Comment 3

[Honza Bambas (:mayhemer)]

Decided to make work for the new cache first deployment.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: wip1 - backup

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: wip2 - backup

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v0.1

Asking for regular review despite few stuff is missing:
- UI for selection of context (private/anon/appid) in about:cache?storage=... entry list
- few new properties left undisplayed in about:cache-entry view (ID extension, and maybe few others)

What's changed:
- a bit beyond scope of this bug: I simplified the recently added asyncGetCacheSize api impl for the old cache and moved it to OldWrappers where it belongs
- added VisitMetaData to nsICacheEntry (->CacheFile->CacheFileMetadata)

Not much tested for memory cache, completely untested for appcache.  Still first review would be good to have.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v0.1b - actually builds on linux

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v0.1c - I like this more

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v0.2

Following net/unit tests are failing (with cache2):

test_cache2-11-evict-memory.js (checks by visit)
test_cache2-05-visit.js
test_cache2-07-visit-memory.js 
test_cache2-22-anon-visit.js

PushMetadataWrites() doesn't work, metadata are not stored prior the iteration so files are not found and onCacheEntryInfo call is missing for them.  I have to think if to keep the tests or try to fix them.  I more tend to disable them, this API is actually important only for about:cache and few tests.

Comment 10

[Honza Bambas (:mayhemer)]

Note that the patch is still not 100% complete, but it's worth to get the first review.

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 8409366 [details] [diff] [review]
v0.2

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

::: netwerk/cache2/CacheStorageService.cpp
@@ +332,5 @@
> +  {
> +    // Push all scheduled metadata writes now so that any new files when
> +    // read back are found.  Note: we don't have a way to find any active
> +    // entry when only having the file name (i.e. the hash) in hands.
> +    CacheFileIOManager::PushMetadataWrites();

But this does not guarantee that all new files could be parsed successfully because not all new entries that are being written or existing entries that are being updated are scheduled for metadata write.

We have a way how to find the active entry. Find an active handle by hash and get the key from it. Then we could get CacheEntry using the key the same way like we notify CacheStorageService about doomed CacheFiles.

@@ +432,5 @@
> +            return NS_OK;
> +
> +          // Iterate all entries in the index for this context to get hashes
> +          // and also to precalculate the size we need to pass to
> +          // onCacheStorageInfo callback.

We don't have to iterate all entries to find out cache size and number of entries. We have this information in CacheIndex::mIndexStats. There is already CacheIndex::GetCacheSize() method so just create a new one for entry count.

@@ +440,5 @@
> +            break; // done (or error?)
> +
> +          // Sum all data sizes
> +          mSize += CacheIndexEntry::GetFileSize(&record) << 10;
> +          memcpy(&mHashes.AppendElement()->h, record.mHash, sizeof(SHA1Sum::Hash));

This whole concept with collecting all hashes is wrong. Just process them one by one as GetNextHash() returns them.

@@ +473,5 @@
> +            continue;
> +
> +          // Read metadata from the file synchronously
> +          nsRefPtr<CacheFileMetadata> metadata = new CacheFileMetadata();
> +          rv = metadata->SyncReadMetadata(file);

As I wrote above, we should get info from active entries from existing CacheFile and we should parse only inactive files here. Also this code should live in CacheFileIOManager. There should be method like CacheFileIOManager::GetEntryInfo(const SHA1Sum::Hash *aHash, nsICacheStorageVisitor *aVisitor).

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #11)
> Comment on attachment 8409366 [details] [diff] [review]
> v0.2
> 
> Review of attachment 8409366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheStorageService.cpp
> @@ +332,5 @@
> > +  {
> > +    // Push all scheduled metadata writes now so that any new files when
> > +    // read back are found.  Note: we don't have a way to find any active
> > +    // entry when only having the file name (i.e. the hash) in hands.
> > +    CacheFileIOManager::PushMetadataWrites();
> 
> But this does not guarantee that all new files could be parsed successfully
> because not all new entries that are being written or existing entries that
> are being updated are scheduled for metadata write.

Yeah, I discovered that during the test checking...  I will probably remove this from the patch at all, it's useless.

> 
> We have a way how to find the active entry. Find an active handle by hash
> and get the key from it. Then we could get CacheEntry using the key the same
> way like we notify CacheStorageService about doomed CacheFiles.

CacheFileUtils::ParseKey(aHandle->Key(), &idExtension, &url);

Got it!  Good idea.

> 
> @@ +432,5 @@
> > +            return NS_OK;
> > +
> > +          // Iterate all entries in the index for this context to get hashes
> > +          // and also to precalculate the size we need to pass to
> > +          // onCacheStorageInfo callback.
> 
> We don't have to iterate all entries to find out cache size and number of
> entries. We have this information in CacheIndex::mIndexStats. There is
> already CacheIndex::GetCacheSize() method so just create a new one for entry
> count.

Unfortunatelly the visiting API (as designed now) of nsICacheStorage works "per context" - so that you can get entries belonging only to "in-browser + appid=1005" etc...  The method you suggest gives only an overall size.

If there is some way to get the size of entries belonging to only a particular context then I'll be happy to use it.

> 
> @@ +440,5 @@
> > +            break; // done (or error?)
> > +
> > +          // Sum all data sizes
> > +          mSize += CacheIndexEntry::GetFileSize(&record) << 10;
> > +          memcpy(&mHashes.AppendElement()->h, record.mHash, sizeof(SHA1Sum::Hash));
> 
> This whole concept with collecting all hashes is wrong. Just process them
> one by one as GetNextHash() returns them.

As wrote above.  I'm not happy with this too, but I don't see another way to sum the disk size.

> 
> @@ +473,5 @@
> > +            continue;
> > +
> > +          // Read metadata from the file synchronously
> > +          nsRefPtr<CacheFileMetadata> metadata = new CacheFileMetadata();
> > +          rv = metadata->SyncReadMetadata(file);
> 
> As I wrote above, we should get info from active entries from existing
> CacheFile and we should parse only inactive files here. Also this code
> should live in CacheFileIOManager. There should be method like
> CacheFileIOManager::GetEntryInfo(const SHA1Sum::Hash *aHash,
> nsICacheStorageVisitor *aVisitor).

So that CacheFileIOManager::GetEntryInfo will dispatch a main thread event with all the necessary info?  What if it only returns the metadata and the consumer handles it as needed?  I somewhat tend to keep this iteration+callback code in one place - the service.  If you insist on moving parts to the IO manager I would not be against tho.  It will just be harder to track the code IMO.

Comment 13

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Unfortunatelly the visiting API (as designed now) of nsICacheStorage works
> "per context" - so that you can get entries belonging only to "in-browser +
> appid=1005" etc...  The method you suggest gives only an overall size.
> 
> If there is some way to get the size of entries belonging to only a
> particular context then I'll be happy to use it.

OK, then we have to iterate over the entries for the given context, but I would prefer to do this internally in CacheIndex since then we can do it without any copying and under a single lock. So what about adding a method CacheIndex::GetCacheStats(nsILoadContextInfo *aInfo, uint32_t *aSize, uint32_t *aCount)


> So that CacheFileIOManager::GetEntryInfo will dispatch a main thread event
> with all the necessary info?  What if it only returns the metadata and the
> consumer handles it as needed?  I somewhat tend to keep this

We would need to create a copy of metadata in case it is an active entry because access to metadata that is in use is synchronized with CacheFile's lock.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #12)

> OK, then we have to iterate over the entries for the given context, but I
> would prefer to do this internally in CacheIndex since then we can do it
> without any copying and under a single lock. So what about adding a method
> CacheIndex::GetCacheStats(nsILoadContextInfo *aInfo, uint32_t *aSize,
> uint32_t *aCount)
> 

According that creating and iterating the entry (context) iterator is cheap, I think this should be OK.  I can also simplify some of the service's Walk code when only the count/sum is demanded.  I'll do that.

> We would need to create a copy of metadata in case it is an active entry
> because access to metadata that is in use is synchronized with CacheFile's
> lock.

I would be OK with that (having a copy) if you are not strongly against.  But I'll reconsider passing the callback in directly as well, maybe that will be even simpler.

Comment 15

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> > OK, then we have to iterate over the entries for the given context, but I
> > would prefer to do this internally in CacheIndex since then we can do it
> > without any copying and under a single lock. So what about adding a method
> > CacheIndex::GetCacheStats(nsILoadContextInfo *aInfo, uint32_t *aSize,
> > uint32_t *aCount)
> > 
> 
> According that creating and iterating the entry (context) iterator is cheap,
> I think this should be OK.  I can also simplify some of the service's Walk
> code when only the count/sum is demanded.  I'll do that.

It is maybe cheap, but my proposal is a way cheaper. Most of the entries will belong to the default context and to get size and count for such context you have to obtain lock, copy CacheIndexRecord and copy the hash for every single entry with the given context. Compare it with a single lock and one iteration over all array items in my proposal.


> > We would need to create a copy of metadata in case it is an active entry
> > because access to metadata that is in use is synchronized with CacheFile's
> > lock.
> 
> I would be OK with that (having a copy) if you are not strongly against. 
> But I'll reconsider passing the callback in directly as well, maybe that
> will be even simpler.

I'm not strictly against it, but keep in mind that metadata can be really big and we want just few numbers from it. So it doesn't make sense to me to copy e.g. 3kB of memory to read just 16 bytes.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #15)
> (In reply to Honza Bambas (:mayhemer) from comment #14)
> > > OK, then we have to iterate over the entries for the given context, but I
> > > would prefer to do this internally in CacheIndex since then we can do it
> > > without any copying and under a single lock. So what about adding a method
> > > CacheIndex::GetCacheStats(nsILoadContextInfo *aInfo, uint32_t *aSize,
> > > uint32_t *aCount)
> > > 
> > 
> > According that creating and iterating the entry (context) iterator is cheap,
> > I think this should be OK.  I can also simplify some of the service's Walk
> > code when only the count/sum is demanded.  I'll do that.
> 
> It is maybe cheap, but my proposal is a way cheaper. Most of the entries
> will belong to the default context and to get size and count for such
> context you have to obtain lock, copy CacheIndexRecord and copy the hash for
> every single entry with the given context. Compare it with a single lock and
> one iteration over all array items in my proposal.

I agree, still you have to iterate the items to call onCacheEntryInfo for each of them.  So, I have to lock/unlock on every one anyway, but when only the count/size is demanded, what you propose is cheaper and saves memory - no need to copy the hashes.

> 
> 
> > > We would need to create a copy of metadata in case it is an active entry
> > > because access to metadata that is in use is synchronized with CacheFile's
> > > lock.
> > 
> > I would be OK with that (having a copy) if you are not strongly against. 
> > But I'll reconsider passing the callback in directly as well, maybe that
> > will be even simpler.
> 
> I'm not strictly against it, but keep in mind that metadata can be really
> big and we want just few numbers from it. So it doesn't make sense to me to
> copy e.g. 3kB of memory to read just 16 bytes.

Yup.

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v0.3

- CacheIndex::GetCacheStats
- CacheFileIOManager::GetEntryInfo
- CacheStorageService::GetCacheEntryInfo(CacheEntry* aEntry, EntryInfoCallback *aVisitor)
- Walk*Runnables adapted to use them
- tests are in much better shape now! only need to update the expected consumption sizes


still not a complete patch but another review insight would be good.

Comment 18

[Michal Novotny [:michal]]

Comment on attachment 8410584 [details] [diff] [review]
v0.3

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

::: netwerk/cache2/CacheFile.cpp
@@ +842,5 @@
>  nsresult
> +CacheFile::VisitMetaData(nsICacheEntryMetaDataVisitor *aVisitor)
> +{
> +  CacheFileAutoLock lock(this);
> +  MOZ_ASSERT(mMetadata);

Please, add MOZ_ASSERT(mReady) here. Metadata must not be accessed until mReady is true.

@@ +845,5 @@
> +  CacheFileAutoLock lock(this);
> +  MOZ_ASSERT(mMetadata);
> +  NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
> +
> +  return mMetadata->Visit(aVisitor);

CacheFileMetadata::Visit() calls aVisitor::OnMetaDataElement() under CacheFile's lock. That's OK, but it needs to be mentioned in nsICacheEntry.idl

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2215,5 @@
> +  ioMan->mHandles.GetHandle(aHash, false, getter_AddRefs(handle));
> +  if (handle) {
> +    nsRefPtr<nsILoadContextInfo> info =
> +      CacheFileUtils::ParseKey(handle->Key(), &enhanceId, &uriSpec);
> +

Add MOZ_ASSERT(info) here since the key from handle must be in correct format.

@@ +2217,5 @@
> +    nsRefPtr<nsILoadContextInfo> info =
> +      CacheFileUtils::ParseKey(handle->Key(), &enhanceId, &uriSpec);
> +
> +    if (!info)
> +      return NS_OK; // ignore

I'm trying to follow formatting rules when adding a new code. Please do following here and at other places in this patch.

if (!info) {
  return NS_OK;
}

@@ +2246,5 @@
> +  nsAutoCString key;
> +  metadata->GetKey(key);
> +
> +  nsRefPtr<nsILoadContextInfo> info =
> +    CacheFileUtils::ParseKey(key, &enhanceId, &uriSpec);

MOZ_ASSERT(info) since the key is valid if SyncReadMetadata() succeeds.

::: netwerk/cache2/CacheStorageService.cpp
@@ +254,3 @@
>          mEntryArray.RemoveElementAt(0);
>  
> +        // Ivokes this->OnEntryInfo, that calls the callback with all

Ivokes -> Invokes

@@ +334,5 @@
> +
> +    // Dispatch to the INDEX level to delay read of the index data as much as
> +    // reasonably possible.  Problem here is that the index gets information
> +    // about new entries very late (after a new file actually opens) what
> +    // happens after few main thread and IO thread loops.

I don't understand this comment. What's wrong with processing entries that are in index at the time of this call?

@@ +342,5 @@
> +    return thread->Dispatch(this, CacheIOThread::INDEX);
> +  }
> +
> +private:
> +  // Invokes OnCacheEntryInfo callback for single found entries.

entries -> entry

::: netwerk/cache2/OldWrappers.cpp
@@ +251,5 @@
> +  if (NS_FAILED(entryInfo->GetExpirationTime(&expirationTime)))
> +    expirationTime = 0;
> +  uint32_t lastModified;
> +  if (NS_FAILED(entryInfo->GetLastModified(&lastModified)))
> +    lastModified = 0;

All default values in case of failure in CacheStorageService::GetCacheEntryInfo() are -1, shouldn't we be consistent?

@@ +477,5 @@
> +
> +NS_IMETHODIMP
> +MetaDataVisitorWrapper::VisitMetaDataElement(char const * key,
> +                                             char const * value,
> +                                             bool *goon)

I'm totally confused here, nsICacheMetaDataVisitor::VisitMetaDataElement() has just 2 arguments.

::: netwerk/protocol/about/nsAboutCache.cpp
@@ +74,2 @@
>  
> +    // Kik it, this goes async.

Did you mean "kick"?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #18)
> Comment on attachment 8410584 [details] [diff] [review]
> @@ +334,5 @@
> > +
> > +    // Dispatch to the INDEX level to delay read of the index data as much as
> > +    // reasonably possible.  Problem here is that the index gets information
> > +    // about new entries very late (after a new file actually opens) what
> > +    // happens after few main thread and IO thread loops.
> 
> I don't understand this comment. What's wrong with processing entries that
> are in index at the time of this call?

I'll try to rephrase, to explain here:

I wanted to delay creation/usage of the index iterator for purpose of the visit as much as possible so that it's a good chance all just created new files are in the index array at the time we start the visit process.  It may not be perfect but in 99% cases it will do.

I'm not sure how else to explain this, if you have an idea on a better comment, feel free to share it.

> ::: netwerk/cache2/OldWrappers.cpp
> @@ +251,5 @@
> > +  if (NS_FAILED(entryInfo->GetExpirationTime(&expirationTime)))
> > +    expirationTime = 0;
> > +  uint32_t lastModified;
> > +  if (NS_FAILED(entryInfo->GetLastModified(&lastModified)))
> > +    lastModified = 0;
> 
> All default values in case of failure in
> CacheStorageService::GetCacheEntryInfo() are -1, shouldn't we be consistent?

nsAboutCache expects != 0 values (otherwise shows something like "unknown" or so) so it's better to have 0 when we fail to get the info.  Good catch, fixed in the service (defaults to 0).

> 
> @@ +477,5 @@
> > +
> > +NS_IMETHODIMP
> > +MetaDataVisitorWrapper::VisitMetaDataElement(char const * key,
> > +                                             char const * value,
> > +                                             bool *goon)
> 
> I'm totally confused here, nsICacheMetaDataVisitor::VisitMetaDataElement()
> has just 2 arguments.

MetaDataVisitorWrapper impls the old API and forwards to the new one, as all in this file (as the name suggests).  The goon bool arg is on the nsICacheMetaDataVisitor (@netwerk/cache/).

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #19)
> MetaDataVisitorWrapper impls the old API and forwards to the new one, as all
> in this file (as the name suggests).  The goon bool arg is on the
> nsICacheMetaDataVisitor (@netwerk/cache/).

Err...:

boolean visitMetaDataElement(...)

see http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsICacheEntryDescriptor.idl#162

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v1

- review comments addressed
- all affected tests fixed and locally passing for cache2=on/off
- _OldVisitCallbackWrapper is now a published VisitCallbackWrapper (OldWrappers.cpp)
- cache v1: fixed this old wrapping visitor when a device is not hit (e.g. memory device has not yet been instantiated but we wanted to visit it)
- cache v1: fixed major bug in nsCacheService::GetDiskCacheDirectory that was mutating the base cache directory (I found my self a Cache dir under Cache dir!)
- nsICacheStorageVisitor.onCacheStorageInfo now also provides the consumption limit and directory (when applicable)
- cache v1: when an detail of a memory-only entry could not find the entry, it's then searched in the disk cache (comments in the code)

https://tbpl.mozilla.org/?tree=Try&rev=97491fef6385 cache2=on
https://tbpl.mozilla.org/?tree=Try&rev=bff06b541241 cache2=off

Comment 22

[Honza Bambas (:mayhemer)]

(Any non-win build problems will be fixed tomorrow)

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v1 -> v2 IDIFF

- UI controls for load context info switch
- cache v1: private entries listed correctly by context
- fixed build warning on linux


Doug, you are listed as a toolkit peer :)  please check the toolkit changes or forward to someone (no idea who should review that, changes are simple tho).  Usage of the script is visible in nsAboutCache::NewChannel (in this patch)

https://tbpl.mozilla.org/?tree=Try&rev=9c11ba805e5a cache2=on
https://tbpl.mozilla.org/?tree=Try&rev=b231df9d7ef6 cache2=off

Comment 24

[Honza Bambas (:mayhemer)]

Attachment: v2 - v2.1 IDIFF

Few more additions I forgot:
- cache v1: fixed view of Private/Disk storage entries that were not found in "disk" storage > storage switched to "memory" in that case
- cache v1: the workaround for not-found-in-memory/look-at-disk is engaged only for the old cache
- cache v1: the context controls are visible also in the overview
- fixed hex offset reset in the data dump

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: v2.1 [complete patch]

...for reference

Comment 26

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #19)
> (In reply to Michal Novotny (:michal) from comment #18)
> > Comment on attachment 8410584 [details] [diff] [review]
> > @@ +334,5 @@
> > > +
> > > +    // Dispatch to the INDEX level to delay read of the index data as much as
> > > +    // reasonably possible.  Problem here is that the index gets information
> > > +    // about new entries very late (after a new file actually opens) what
> > > +    // happens after few main thread and IO thread loops.
> > 
> > I don't understand this comment. What's wrong with processing entries that
> > are in index at the time of this call?
> 
> I'll try to rephrase, to explain here:
> 
> I wanted to delay creation/usage of the index iterator for purpose of the
> visit as much as possible so that it's a good chance all just created new
> files are in the index array at the time we start the visit process.  It may
> not be perfect but in 99% cases it will do.
> 
> I'm not sure how else to explain this, if you have an idea on a better
> comment, feel free to share it.

And why it is important to have all the new entries in the about:cache output? Btw. the old cache has the same problem. The new entry is added to the cache map when it is bound to the device.

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #26)
> And why it is important to have all the new entries in the about:cache
> output? Btw. the old cache has the same problem. The new entry is added to
> the cache map when it is bound to the device.

I think it's a little bit different with the new cache.  We delay writes to disk and much more buffer in memory.  The INDEX post was added before you came with the idea to look for an open handle.  That ensures (100%?) that very recent entries still in phase of write are displayed in about:cache.  And in general, we are trying to be better then the old cache :)  but that may not be a strong argument.

Thanks for the reviews!

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 8411865 [details] [diff] [review]
v1 -> v2 IDIFF

matt, can you take a look at this?

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8411865 [details] [diff] [review]
v1 -> v2 IDIFF

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

I haven't looked at this thoroughly or applied the patch but there's already enough cleanup to get started.

::: netwerk/protocol/about/nsAboutCache.cpp
@@ +64,5 @@
>      mBuffer.AssignLiteral(
> +        "<!DOCTYPE html>\n"
> +        "<html>\n"
> +        "<head>\n"
> +        "  <title>Network Cache Storage Information</title>\n"

Please add <meta charset="utf-8"> (assuming this is the proper charset) as the first child of <head> to avoid a console warning.

@@ +68,5 @@
> +        "  <title>Network Cache Storage Information</title>\n"
> +        "  <link rel=\"stylesheet\" "
> +        "href=\"chrome://global/skin/about.css\" type=\"text/css\"/>\n"
> +        "  <link rel=\"stylesheet\" "
> +        "href=\"chrome://global/skin/aboutCache.css\" type=\"text/css\"/>\n"

Nit: @type="text/css" is the default so isn't necessary. You can clean it up while you're touching this if you want.

@@ +78,5 @@
> +
> +    if (!mOverview || CacheObserver::UseNewCache()) {
> +        // Add the context switch controls
> +        mBuffer.AppendLiteral(
> +            "<input id='#priv' type='checkbox' /> Private\n"

What's with the "#" prefix? I've never seen this used as a prefix on @id and I suspect you got confused with the CSS id selector. Please remove this prefix throughout and update the JS.

@@ +79,5 @@
> +    if (!mOverview || CacheObserver::UseNewCache()) {
> +        // Add the context switch controls
> +        mBuffer.AppendLiteral(
> +            "<input id='#priv' type='checkbox' /> Private\n"
> +            "<input id='#anon' type='checkbox' /> Anonymous\n"

Please wrap all <input>s with <label>s for proper accessibility:

<label><input id='anon' type='checkbox'/> Anonymous</label>

This allows clicking on the label text to focus the input and allows accessibility tools to know which adjacent text is referring to which <input>.

@@ +88,5 @@
> +            // the old cache, simply don't add these controls.
> +            // The appid/inbrowser entries are already mixed in the default
> +            // view anyway.
> +            mBuffer.AppendLiteral(
> +                "<input id='#appid' type='text' value='' /> AppID\n"

I haven't applied the patch so I'm not sure what this is for filtering. If it is, you can use type=search. 

Nit: leave out @value=''

::: toolkit/components/aboutcache/content/aboutCache.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// First, parse and save the incoming arguments ("?storage=name&context=key")
> +var args = location.href.match(/[\?&]([^=]+)=([^&]*)/g);

You shouldn't need to do regexes like this anymore now that we have URLSearchQuery/URL implemented in the DOM.
If you just want the query parameters you can do:
var searchParams = new URLSearchParams(window.location.search);
storage = searchParams.get("storage");

(as an example)

@@ +30,5 @@
> +  $('#priv').checked = !!isPrivate;
> +}, false);
> +
> +// Called by [Update] button on the about:cache page, recreate the "context=" argument
> +// according how user has set the context UI controls on about:cache and navigate with it

Nit: missing words

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Matthew N. [:MattN] from comment #29)
> Comment on attachment 8411865 [details] [diff] [review]
> v1 -> v2 IDIFF

Thanks for the review!  I will update shortly.

> What's with the "#" prefix? I've never seen this used as a prefix on @id and
> I suspect you got confused with the CSS id selector. Please remove this
> prefix throughout and update the JS.

Inspired, not confused.  I just wanted to make it more visible for me.  Will remove.

> 
> @@ +79,5 @@
> > +    if (!mOverview || CacheObserver::UseNewCache()) {
> > +        // Add the context switch controls
> > +        mBuffer.AppendLiteral(
> > +            "<input id='#priv' type='checkbox' /> Private\n"
> > +            "<input id='#anon' type='checkbox' /> Anonymous\n"
> 
> Please wrap all <input>s with <label>s for proper accessibility:
> 
> <label><input id='anon' type='checkbox'/> Anonymous</label>

Yeah, was thinking of it too.

> > +            mBuffer.AppendLiteral(
> > +                "<input id='#appid' type='text' value='' /> AppID\n"
> 
> I haven't applied the patch so I'm not sure what this is for filtering. If
> it is, you can use type=search. 

What is search?  I think this should actually be 'number' or have a '####' formatting.  The appID is an App internal identification number (http://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIPrincipal.idl#198).  0 or empty - no appid, 1001 - app #1001 etc...

> 
> Nit: leave out @value=''

I think w/o it the value is undefined when not set, but I think I have fixed this in js with 'value || ""'.

> > +// First, parse and save the incoming arguments ("?storage=name&context=key")
> > +var args = location.href.match(/[\?&]([^=]+)=([^&]*)/g);
> 
> You shouldn't need to do regexes like this anymore now that we have
> URLSearchQuery/URL implemented in the DOM.
> If you just want the query parameters you can do:
> var searchParams = new URLSearchParams(window.location.search);
> storage = searchParams.get("storage");

Nice, didn't know about that, but - for about:* we use simple URIs, and those have the search property empty despite it's clearly contained in the URL spec.  Also location.search = "something" doesn't navigate.  Not sure now it's expected or a bug.  So, I rather use href directly.  If URLSearchParams works for that too, I will gladly use it.  Otherwise, I will leave this code as is or just regexp the search string from href and then use URLSearchParams.  Would that be OK?  Note: I don't want to fix .search behavior in this bug - only in a followup.

> > +// Called by [Update] button on the about:cache page, recreate the "context=" argument
> > +// according how user has set the context UI controls on about:cache and navigate with it
> 
> Nit: missing words

Not sure I follow which words should be missing here.  I will try to rephrase the comment tho.

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: v2.1 -> v2.2 IDIFF

- addressed review comments from Matthew
- added an error message when storage or context in the url is not recognized
- mEntriesHeaderAdded = false initialized sooner
- doing URLSearchParams() off window.location.href.match(/^.*\?(.*)$/)[1] since location.search doesn't work for about:* URLs (nsSimpleURI)

Comment 32

[Honza Bambas (:mayhemer)]

Attachment: v2.2 [full patch]

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8414741 [details] [diff] [review]
v2.2 [full patch]

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

Looks good.

::: netwerk/protocol/about/nsAboutCache.cpp
@@ +99,5 @@
> +        "<label><input id='submit' type='button' value='Update' onclick='navigate()'/></label>\n"
> +    );
> +
> +    if (!mOverview) {
> +        mBuffer.AppendLiteral("<a href=\"about:cache?storage=&context=");

Nit: "&" should use the HTML entity: "&amp;"

::: toolkit/themes/windows/global/aboutCache.css
@@ +6,5 @@
>    margin-top: 2em;
>  }
>  
> +input[type=text] {
> +  width: 4em;

Nit: I would rather this be set using a @size attribute. Also, it was surprising to me that the window stylesheet also applied on OS X

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 8413727 [details] [diff] [review]
v2.1 -> v2.2 IDIFF

Michal, can you please r only the mEntriesHeaderAdded and nsAboutCache::FireVisitStorage() changes in nsAboutCache.cpp?  Thanks.

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: v2.3

Comment 36

[Michal Novotny [:michal]]

Comment on attachment 8413727 [details] [diff] [review]
v2.1 -> v2.2 IDIFF

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

::: netwerk/protocol/about/nsAboutCache.cpp
@@ +55,5 @@
>          // ...and visit just the specified storage, entries will output too
>          mStorageList.AppendElement(storageName);
>      }
>  
> +    // The entries header is added on encoutner of the first entry

encoutner -> encounter

The same typo in also in nsAboutCache::OnCacheStorageInfo() which I missed in the previous review.

Comment 37

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2557b51a47a0

Comment 38

[Honza Bambas (:mayhemer)]

Backed out for linux build failures (not happening locally!)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d72bfb3cbfb

Comment 39

[Honza Bambas (:mayhemer)]

Attachment: v2.4 [to land]

Fixed build issues (missing includes, unified build hides it)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4b2e930ecd5

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4b2e930ecd5