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.

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

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

Merged
merged 1 commit into from
May 30, 2018

Conversation

mossroy
Copy link
Contributor

@mossroy mossroy commented May 30, 2018

Instead of calling displayArticleInForm like in jQuery mode.
I also removed some tests on jQuery/ServiceWorker modes in displayArticleInForm, as this function is now only used in jQuery mode.
And I renamed it to displayArticleContentInIframe

Fixes #382

Instead of calling displayArticleInForm like in jQuery mode.
I also removed some tests on jQuery/ServiceWorker modes in displayArticleInForm, as this function is now only used in jQuery mode.
And I renamed it to displayArticleContentInIframe

Fixes #382
@mossroy mossroy requested a review from Jaifroid May 30, 2018 12:59
@mossroy
Copy link
Contributor Author

mossroy commented May 30, 2018

I've tested on Firefox 60 and Chromium 66, in ServiceWorker and jQuery modes, with various ZIM files (including stackoverflow.com_eng_all_2017-05.zim).

I also removed some tests on jQuery/ServiceWorker modes in displayArticleInForm, as this function is now only used in jQuery mode. And I renamed it to displayArticleContentInIframe.

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

I've tested in ServiceWorker and jQuery mode in Edge, and in jQuery mode in IE10. All working fine on quick tests. I noticed the spinner sometimes doesn't hide until the images have finished loading (in ServiceWorker on Edge). It's quite accurate in a way, because it shows the content is still loading, but it's a change (I think). (It doesn't occur in jQuery mode.)

@@ -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.

@Jaifroid
Copy link
Member

Jaifroid commented May 30, 2018

Here's an example of the spinner still showing while images are being loaded, in Service Worker. I'm not familiar enough with Service Worker to remember if this is the standard behaviour in that mode. (Sorry for the milkiness of this image, I put a breakpoint to be sure I could screenshot the area. Also, typo was deliberate, as I was trying to trigger the .isRedirect code, unsuccessfully!)

image

@Jaifroid
Copy link
Member

PS The code does significantly improve the speed of display of the homepage of the huge Stackexchange ZIM in Edge in ServiceWorker mode.

@mossroy
Copy link
Contributor Author

mossroy commented May 30, 2018

Regarding the spinner issue, I don't reproduce it on Firefox or Chrome.
We call $("#readingArticle").hide(); in the onload event, so it's normal that it disappears after the page is completely ready. But it's also at this moment that we call $("#articleContent").show();, so the article should not be visible before we hide the spinner...
Maybe Edge makes the iframe visible as soon as we set a new src to it?
In any case, both behaviors look acceptable to me.

Regarding the ServiceWorker mode in general, there are many things that should be easier and faster to achieve with it, using the browser capabilities instead of reimplementing things ourself. That's why I'm willing to focus on it for the next hackathon, in order to (hopefully) make it the default mode (when available on the device/browser). It does not mean we would drop jQuery mode support, because it's still necessary in many contexts.

@mossroy
Copy link
Contributor Author

mossroy commented May 30, 2018

Can I merge this PR?

@Jaifroid
Copy link
Member

Yes, looks good to me!

@mossroy mossroy merged commit 78ee035 into master May 30, 2018
@mossroy mossroy deleted the Always-use-iframe-src-in-Service-Worker-mode branch May 30, 2018 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants