Slow chrome issue

[jesusbagpuss]

Based on post to EPTech (IRStats2: Slow in Chrome?) and proposed solution by David Newman.

Changes to Perl code have been tested.
Not all changes in the phrase files have been fully tested, although a cursory test appears OK.

[mpbraendle]

According to https://developers.google.com/chart/interactive/docs/basic_load_libs#update-library-loader-code

the loader code in cfg.d/z_irstats2.pl should be changed as well.

I'm just testing now on our repo and will report back.

[jesusbagpuss]

@mpbraendle - should the 'protocol' test should also be removed, and https always used for loading google charts loader?

irstats2/cfg/cfg.d/z_irstats2.pl

Lines 147 to 153 in 13c43a4

	my $protocol = $repo->get_secure ? 'https':'http';
	my $head = $repo->make_doc_fragment;
	$head->appendChild( $repo->make_javascript( undef,
	src => "$protocol://www.google.com/jsapi"
	) );

[mpbraendle]

http://www.gstatic.com/charts/loader.js seems to work, too. So you could leave it in.
Also, with the new library, calls now start with google.charts.... (should all be replaced in z_irstats.pl and the files above).

I've tested so far on our repo and that works fine.

[photomedia]

Yes, @mpbraendle , the loader needs to be changed, along with the load calls. I made this fix on our repository and I haven't seen the failed loading of visualizations since. The failed loading isn't just slow, from my experience, they also fail sometimes, so they are not reliable as is. They worked sometimes. True, they worked more often in Firefox than Chrome, but it's not just a browser issue. I proposed my fix in a separate branch a few weeks ago: bbb8653

[jesusbagpuss]

@photomedia - sorry, I didn't spot that branch!

Is it worth submitting it as a pull request to replace this one?

[wfyson]

Ah no - I didn't see this and I think I've just applied the same changes at c1cd42d.

It pretty much all looks the same as far as I can tell? Although am I missing something with the name and id being added as part of the function name in ae610a2 and 2024e7e?

[photomedia]

Hi everyone! I used a branch because I wanted my proposed changes to be reviewed. I submitted my fix (bbb8653) in June 2020 for review as a branch, and posted a message on the list about it. It is the first time that I make a submission of code to IRStats2, so I felt like it should be reviewed. @jesusbagpuss , I'm not sure how and if I should submit it as a pull request, given that there are concurrent fixes proposed (that may be the same or not). The important thing, I believe, is to update the loader code to the currently supported way of doing it, see: https://developers.google.com/chart/interactive/docs/basic_load_libs#update-library-loader-code
That means specifying which version of the Google libraries you want loaded, and using the gstatic.com/charts/loader.js script rather than the jsapi. I suggest specifying the version number to load rather than using "current" , to make sure that it is working before we switch to update the version number as Google updates their code.

[goetzk]

Hi @jesusbagpuss and @photomedia ,
I see we have three different ways of fixing this problem (based only on this ticket). I am happy to help review and merge either this PR or the branch https://github.com/eprintsug/irstats2/compare/GoogleChartsLoadv48), but I suspect I'm less qualified to judge the approaches + JS usage than you two.

Are either of you around to comment on what you think will be the best path forward?

This feels like a large enough fix we should look at doing a release to bazaar after, but lets cross that bridge once this problem is dealt with.

[jesusbagpuss]

I've just had a quick look at the differences between

• c1cd42d (Will)
• ae610a2 (me)
• 2024e7e (David)
• bbb8653 (Tomasz)

A 4-way diff made my head hurt.

First questions - which is more correct (I'll try and find an answer to this - but if someone knows, please tell!):

• google.setOnLoadCallback(function(){...} -or-
• document.observe("dom:loaded",function(){...})
  and
• https://www.google.com/jsapi -or-
• https://www.gstatic.com/charts/loader.js

[wfyson]

Hi @jesusbagpuss, thanks for looking into this...

First questions - which is more correct (I'll try and find an answer to this - but if someone knows, please tell!):

• google.setOnLoadCallback(function(){...} -or-
• document.observe("dom:loaded",function(){...})
  and
• https://www.google.com/jsapi -or-
• https://www.gstatic.com/charts/loader.js

google.setOnLoadCallback(function(){...} is the important change I think, that's what actually fixes the slow Chrome issue.

It looks like https://www.google.com/jsapi simply redirects to https://www.gstatic.com/charts/loader.js and the Quick Start guide to Google Charts says to use the gstatic version too: https://developers.google.com/chart/interactive/docs/quick_start

[photomedia]

@jesusbagpuss, I agree with @wfyson, these are more correct:

google.setOnLoadCallback(function(){...} and https://www.gstatic.com/charts/loader.js

In addition, I would add the following as important:

1. Specify version number of the Google library to load when calling google.charts.load
2. We need to pass the container ID of the div where the visualization is to appear when calling new google.visualization.

[wfyson]

Hi all,

I've been having another look at this and have been trying to work out all of the most pertinent bits of the various commits. Because this pull request and the different branches have got a bit complicated, my thinking is we just grab all the most pertinent bits from the various commits, add these to master in the and close the pull request without merging.

With that in mind I've just made a new commit at 26ae544 which removes the protocol line (as suggested by @jesusbagpuss above) and I've updated the load statement a little as per @photomedia's suggestion.

I've not included all of your changes @photomedia (i.e. those found in bbb8653) - when I tried implementing them they didn't seem to work nicely with all the different ways that graphs can be called by IRStats2. For example when rendering a graph on a summary page via a plugin, it didn't like the fact that google.charts.load wasn't called before google.charts.setOnLoadCallback. Maybe there was something I was missing, but I also can't see the harm in the current approach so hopefully that'll be ok?

With the two commits (c1cd42d and 26ae544) I think all the main points are covered for this issue, but if anyone has a moment to review them that would be much appreciated! I've now increased the version to 1.1.3 too.

[photomedia]

@wfyson, sounds good to me, as long as it works, I have no issue :) There could be something slightly different in how I bring in the visualizations on summary pages; I remember that this wasn't enabled by default in the plugin when I installed it. Speaking of testing different use cases, did you check that it works for when there are multiple graphs on the screen at the same time? There are some screens like that, which display a small visualization and a larger one on the same page.

[jesusbagpuss]

NB definitely need to merge #17 and also resolve base_url issue before new release is packaged!

[wfyson]

@photomedia With regards to multiple graphs on the screen it should work but this depends on how you render them on screen, with the important thing being that each is passed a unique container_id that matches the id of a div on the page.

I think there are two main approaches, you can either add the HTML and Javascript straight on the page, e.g. as part of the summary page citation. In which case you end up with something like this:

                <div class="irstats2_graph" id="irstats2_summary_page_downloads"/>
                    <script type="text/javascript">
                     google.setOnLoadCallback(function(){
                        var irstats2_summary_page_eprintid = '<epc:print expr="eprintid"/>';

                        new EPJS_Stats_GoogleGraph ( { 'context': {
                            'range':'6m',
                            'set_name': 'eprint',
                            'set_value': irstats2_summary_page_eprintid,
                            'datatype':'downloads' },
                            'options': {
                            'container_id': 'irstats2_summary_page_downloads', 'date_resolution':'month','graph_type':'column'
                            } } );
                    });
                    </script>
                    <p><a href="/cgi/stats/report/eprint/{eprintid}">View details</a></p>
                </div>
            </div>

And then when you wanted to add another you just make sure your container_id for the next one matches a new div.

The other main approach I think is to call it programmatically. In this case it'll depend on what parameters are passed on with $c->{irstats2}->{report} in z_irstats2.pl providing an example of this. Looking at how the reports are defined here, and then used in the context of View/KeyFigures.pm the ID seems to be taken from the metric parameter. (Apologies if you're already familiar with all this!) It seems to me the easiest approach is just to include the JS straight in the summary page!

[wfyson]

Hi @jesusbagpuss - oh yeah I quite agree that needs sorting before any sort of release. I wasn't planning on pushing to the Bazaar just yet! I'm just trying to get into the habit of making sure the epm/epmi stays up to date all the time!