The JavaScript used for displaying banners tends to put a large load on the client computer which slows down the client computer. I would like to propose that the JavaScript be turned off when it's not needed.

(Great module, by the way).

CommentFileSizeAuthor
#9 banner.module.nojs.patch5.19 KBdan_aka_jack
#5 banner.module.2.patch5.89 KBdan_aka_jack
#2 banner.module_3.patch5.82 KBdan_aka_jack

Comments

dan_aka_jack’s picture

dan_aka_jack commented

Hmm... I'm trying to submit a patch but I keep getting a "terminated" response from drupal.org... I'll keep trying

dan_aka_jack’s picture

dan_aka_jack commented
StatusFileSize
new banner.module_3.patch5.82 KB

I've removed the JavaScript call for banner ads.

Before applying my patch, the module displays each banner by outputting this line for each banner:

script type="text/javascript" language="javascript" src="http://mysite.com/banner_file.php?pos=0&path=files&tid=0&max=5"

After applying my patch, the javascript call is removed:

a href="/banner/1" img src="http://mysite.com/files/banners/banner1.png" border=0 width="120" height="120"

(sorry about the formatting... for some reason Drupal.org isn't allowing me to post using the code /code tags)

I developed this patch because the JavaScript call was hurting some other JavaScript on my site.

This patch will prevent rotation.

You can set whether or not the module should use JavaScript in admin/settings/banner

dan_aka_jack’s picture

dan_aka_jack commented
Status: Active » Needs review
dan_aka_jack’s picture

dan_aka_jack commented

Just to say that banner caching needs to be turned on (and working) for my patch to work

dan_aka_jack’s picture

dan_aka_jack commented
StatusFileSize
new banner.module.2.patch5.89 KB

Here's a new version of the patch... all I've added is this:

  $pos = (int) $pos;
  $tid = (int) $tid;
  $max = (int) $max;

at the top of function banner_file. This fixes a bug where my patch prevented banners from showing on /node/ pages.

wulff’s picture

wulff commented

The JavaScript used for displaying banners tends to put a large load on the client computer which slows down the client computer.

I'm not sure I agree with this. The only thing the client computer does is download and run the script generated by one of the cache managers (banner_db.php or banner_file.php). The script consists of one call to document.write(), which shouldn't be too taxing for any recent client.

What might slow the client computer down is that it has to wait for the server to return the script before it can finish rendering the page. Using the file cache should make sure that that the response from the server is as fast as possible.

If the JavaScript generated by this module interacts badly with other scripts (I'd love to see an example), I think we're better off changing the script than disabling it altogether, since disabling the JavaScript removes some of the basic functionality of the module (i.e. ad rotation even on cached pages).

dan_aka_jack’s picture

dan_aka_jack commented

What might slow the client computer down is that it has to wait for the server to return the script before it can finish rendering the page.

Yes, you're right. That's what was slowing down out site. It wasn't the JavaScript itself, it was the time it took the server to provide the file. It was taking about 1 second.

We use JavaScript to output some flash on our site (see http://ukfilm.org ) - the titles on each news item are Flash. When the javascript was enabled on the banner module, the site would load, then wait a second and then the flash headers would appear. This was ugly.

wulff’s picture

wulff commented

Before including this patch, I'd like to get some more opinions on it. As it is, I'm not sure it will go in.

If there is interest in adding this feature, we should probably use the code from banner_db instead of banner_file to choose the banner to display (it is a lot simpler).

dan_aka_jack’s picture

dan_aka_jack commented
StatusFileSize
new banner.module.nojs.patch5.19 KB

we should probably use the code from banner_db instead of banner_file to choose the banner to display (it is a lot simpler)

Ah, yes - good idea. I've attached a new patch which used the code from banner_db (this new patch has been made against today's CVS)

wulff’s picture

wulff commented
Status: Needs review » Closed (fixed)

Marking as closed since the banner module has been abandoned.