Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

In ServiceWorker mode, set the iframe src to display articles. #383

Merged
merged 1 commit into from
May 30, 2018
Merged

In ServiceWorker mode, set the iframe src to display articles. #383

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions www/js/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,11 +743,26 @@ define(['jquery', 'zimArchiveLoader', 'util', 'uiUtil', 'cookies','abstractFiles
* @param {DirEntry} dirEntry
*/
function readArticle(dirEntry) {
if (dirEntry.isRedirect()) {
selectedArchive.resolveRedirect(dirEntry, readArticle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize the code that provides the Service Worker with the content does its own resolveRedirect, but wouldn't it save a hop if we allowed this redirect code to run for both modes (jQuery and ServiceWorker) here, then set the iframe's src to the dirEntry.namespace + url of the new dirEntry? Maybe it's unlikely we'd actually encounter a redirect here? Anyway, just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would certainly work and speed up a little bit.
It's completely possible to encounter a redirect here.
On the other hand, we would "only" save the binary search done by getDirEntryByTitle.
And we had here the opportunity to do things exactly the same between an article opened after a search (or a random search, or the main article), and an article opened while browsing (after a click on a hyperlink). If we handle the redirection differently, we re-introduce a little difference.
I'm torn between doing it or not (or doing it later). Maybe we should benchmark it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very minor. I suggest doing it later if we find a need. If it saves anything, it would be a near-instantaneous lookup. I tried hitting the random button several times and typing misspellings of names into search several times, and I could never get the isRedirect code to fire in the code that provides the Service Worker with content.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I created #384 for that, let's leave this portion as it is, for now.

if (contentInjectionMode === 'serviceworker') {
// In ServiceWorker mode, we simply set the iframe src and show it when it's ready.
// (reading the backend is handled by the ServiceWorker itself)
var iframeArticleContent = document.getElementById('articleContent');
iframeArticleContent.onload = function () {
iframeArticleContent.onload = function () {};
// Actually display the iframe content
$("#readingArticle").hide();
$("#articleContent").show();
};
iframeArticleContent.src = dirEntry.namespace + "/" + dirEntry.url;
}
else {
selectedArchive.readUtf8File(dirEntry, displayArticleInForm);
// In jQuery mode, we read the article content in the backend and manually insert it in the iframe
if (dirEntry.isRedirect()) {
selectedArchive.resolveRedirect(dirEntry, readArticle);
}
else {
selectedArchive.readUtf8File(dirEntry, displayArticleContentInIframe);
}
}
}

Expand Down Expand Up @@ -825,15 +840,13 @@ define(['jquery', 'zimArchiveLoader', 'util', 'uiUtil', 'cookies','abstractFiles
* @param {DirEntry} dirEntry
* @param {String} htmlArticle
*/
function displayArticleInForm(dirEntry, htmlArticle) {
function displayArticleContentInIframe(dirEntry, htmlArticle) {
// Scroll the iframe to its top
$("#articleContent").contents().scrollTop(0);

if (contentInjectionMode === 'jquery') {
// Replaces ZIM-style URLs of img, script and link tags with a data-url to prevent 404 errors [kiwix-js #272 #376]
// This replacement also processes the URL to remove the path so that the URL is ready for subsequent jQuery functions
htmlArticle = htmlArticle.replace(regexpTagsWithZimUrl, "$1data-kiwixurl$2$3");
}
// Replaces ZIM-style URLs of img, script and link tags with a data-url to prevent 404 errors [kiwix-js #272 #376]
// This replacement also processes the URL to remove the path so that the URL is ready for subsequent jQuery functions
htmlArticle = htmlArticle.replace(regexpTagsWithZimUrl, "$1data-kiwixurl$2$3");

// Compute base URL
var urlPath = regexpPath.test(dirEntry.url) ? urlPath = dirEntry.url.match(regexpPath)[1] : '';
Expand Down Expand Up @@ -862,15 +875,12 @@ define(['jquery', 'zimArchiveLoader', 'util', 'uiUtil', 'cookies','abstractFiles
$("#articleContent").show();
// Allow back/forward in browser history
pushBrowserHistoryState(dirEntry.namespace + "/" + dirEntry.url);
// If the ServiceWorker is not useable, we need to fallback to parse the DOM
// to inject images, CSS etc, and replace links with javascript calls
if (contentInjectionMode === 'jquery') {
parseAnchorsJQuery();
loadImagesJQuery();
loadCSSJQuery();
//JavaScript loading currently disabled
//loadJavaScriptJQuery();
}

parseAnchorsJQuery();
loadImagesJQuery();
loadCSSJQuery();
//JavaScript loading currently disabled
//loadJavaScriptJQuery();
};

// Load the blank article to clear the iframe (NB iframe onload event runs *after* this)
Expand Down