GNOME Bugzilla – Bug 781032
Simplify GdPdfLoader, improve caching, etc..
Last modified: 2017-04-20 16:12:33 UTC
In bug 774937, as a side-effect of fixing loading of LOKDocView-supported OneDrive documents, we moved a big chunk of the OneDrive loading code path from GdPdfLoader to DocCommon and SkydriveDocument. This was enabled by the fact that EvView is no longer the only renderer we have. The obvious benefit is that it replaces a maze of asynchronous C code with JavaScript. Secondly, we were needlessly hitting the network when loading a remote document even if there was a valid copy in the cache because we were creating a ZpjEntry or GDataEntry as the first step. The new JS-based code paths don't do that. They only use the network if there is a cache miss. Thirdly, ODFs and OOXMLs from ownCloud were not being cached at all since the introduction of LOKDocView. This also tweaks the structure of the cache a bit. The old scheme was: ~/.cache/gnome-documents/gnome-documents-<g_str_hash(id)>.pdf The new one is: ~/.cache/gnome-documents/skydrive/<SHA1(id)>.<original-extension> Debugging is easier if (a) the cache is namespaced by the source of the documents because one can selectively nuke a subset of the cache, and (b) uses a hash function that is easily replicated by other tools because one can "manually" match the original ID to the cache entry.
Created attachment 349467 [details] [review] documents: Specify uriToLoad for GoogleDocument
Created attachment 349468 [details] [review] documents: Implement downloadImpl for GoogleDocument
Created attachment 349473 [details] [review] documents, pdf-loader: Reduce the reliance on GdPdfLoader for Google
Created attachment 349484 [details] [review] documents: Use a global named constant for the ownCloud prefix
Created attachment 349488 [details] [review] documents, pdf-loader: Reduce the reliance on GdPdfLoader for ownCloud
Created attachment 349493 [details] [review] pdf-loader: Remove the unused unoconv-based PDF conversion code
Is there any logging facility other than 'log' in gjs? I am wondering whether we can add some debug messages. In this case, it will be useful to log the different loading stages so that I can convince myself that the caching is working as expected. However, those messages are going to be noise for most users.
Created attachment 349494 [details] [review] documents: Add a generic load implementation to DocCommon
(In reply to Debarshi Ray from comment #7) > Is there any logging facility other than 'log' in gjs? > > I am wondering whether we can add some debug messages. In this case, it will > be useful to log the different loading stages so that I can convince myself > that the caching is working as expected. However, those messages are going > to be noise for most users. Maps implements its own debug function: https://git.gnome.org/browse/gnome-maps/tree/src/utils.js#n45
Review of attachment 349467 [details] [review]: Looks good.
Review of attachment 349468 [details] [review]: Feel free to push with this fixed. ::: src/documents.js @@ +942,3 @@ + + try { + return; Single quotes.
Review of attachment 349473 [details] [review]: Nice!
Review of attachment 349484 [details] [review]: ++
Review of attachment 349488 [details] [review]: Nice!
Review of attachment 349493 [details] [review]: Yay!
Review of attachment 349494 [details] [review]: ++
(In reply to Debarshi Ray from comment #7) > Is there any logging facility other than 'log' in gjs? > > I am wondering whether we can add some debug messages. In this case, it will > be useful to log the different loading stages so that I can convince myself > that the caching is working as expected. However, those messages are going > to be noise for most users. I don't think gjs has it natively, but gnome-documents has an Utils.debug() function that you could use for this.
(In reply to Alessandro Bono from comment #9) > (In reply to Debarshi Ray from comment #7) > > Is there any logging facility other than 'log' in gjs? > > > > I am wondering whether we can add some debug messages. In this case, it will > > be useful to log the different loading stages so that I can convince myself > > that the caching is working as expected. However, those messages are going > > to be noise for most users. > > Maps implements its own debug function: > https://git.gnome.org/browse/gnome-maps/tree/src/utils.js#n45 Nice. That's also the same thing Cosimo pointed out: (In reply to Cosimo Cecchi from comment #17) > I don't think gjs has it natively, but gnome-documents has an Utils.debug() > function that you could use for this. Thanks!
Created attachment 349726 [details] [review] documents: Implement downloadImpl for GoogleDocument Replaced double-quotes around "pdf" with single quotes and pushed.
Created attachment 349727 [details] [review] pdf-loader: Remove unused function This should have been a part of attachment 349473 [details] [review]. I had it in my local version of the patch but forgot to update the one that was attached to this bug.
Review of attachment 349727 [details] [review]: Even better!
(In reply to Debarshi Ray from comment #0) > The obvious benefit is that it replaces a maze of asynchronous C code with > JavaScript. > > Secondly, we were needlessly hitting the network when loading a remote > document even if there was a valid copy in the cache because we were > creating a ZpjEntry or GDataEntry as the first step. The new JS-based code > paths don't do that. They only use the network if there is a cache miss. > > Thirdly, ODFs and OOXMLs from ownCloud were not being cached at all since > the introduction of LOKDocView. > > This also tweaks the structure of the cache a bit. The old scheme was: > ~/.cache/gnome-documents/gnome-documents-<g_str_hash(id)>.pdf > The new one is: > ~/.cache/gnome-documents/skydrive/<SHA1(id)>.<original-extension> > > Debugging is easier if (a) the cache is namespaced by the source of the > documents because one can selectively nuke a subset of the cache, and (b) > uses a hash function that is easily replicated by other tools because one > can "manually" match the original ID to the cache entry. Another positive side-effect is that we can now load encrypted PDFs from Google. Earlier we'd delete the cached copy when we got the first EV_DOCUMENT_ERROR_ENCRYPTED, and we never specify the password (even if the user entered one) until we got the first EV_DOCUMENT_ERROR_ENCRYPTED. This meant that we used to keep asking the user for the password indefinitely.