Plone 4: jquery.highlightsearchterms.js gives error

[Maurits van Rees-3]

Hi,

When loading most pages of a fresh Plone 4 site it takes a while to
load and then Firefox (3.0.15 on Ubuntu 9.04) tells me that a script
has an error or does not respond.  It is line 55 in this script:
Plone/Products/CMFPlone/skins/plone_ecmascript/jquery.highlightsearchterms.js

=====================================================================
  queryStringValue: function(uri, regexp) {
      // Return the decoded value of the key=value pair in the query string
      // uri is the full URI including qs, regexp is a /key=(.*)/ pattern
      if (uri.indexOf('?') < 0) return '';
      uri = uri.substr(uri.indexOf('?') + 1);
      while (uri.indexOf('=') >= 0) {
          var pair = uri.split('&', 1)[0];
          // According to firefox the error is on this line:
          uri = uri.substr(pair.length);
          var match = pair.match(regexp);
          if (match)
              return decodeURIComponent(
                  match[match.length-1].replace(/\+/g, ' '));
      }
      return '';
  },
=====================================================================

The most I think is: yep, sure looks like javascript. ;-)  Probably
the while loop never finishes.  The effect is that on some pages
javascript completely fails (e.g. the actions are all dropped down).
Does anyone see what is wrong here?

Cheers,

--
Maurits van Rees | http://maurits.vanrees.org/
            Work | http://zestsoftware.nl/
"This is your day, don't let them take it away." [Barlow Girl]


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Simone Deponti-2]

Hi,

seems like after splitting, & is the first character and so split('&',1)
returns ['',restOfStuff].

so if we add

if(uri[0] == '&') {
    if(uri.length > 1)
        uri = uri.substr(1);
    else
        return '';
}

just after the while() { it should work.

However I have just tested this with the standalone console js
interpreter, someone might want to do this properly.

On 11/05/2009 04:38 PM, Maurits van Rees wrote:

>   queryStringValue: function(uri, regexp) {
>       // Return the decoded value of the key=value pair in the query string
>       // uri is the full URI including qs, regexp is a /key=(.*)/ pattern
>       if (uri.indexOf('?') < 0) return '';
>       uri = uri.substr(uri.indexOf('?') + 1);
>       while (uri.indexOf('=') >= 0) {
>           var pair = uri.split('&', 1)[0];
>           // According to firefox the error is on this line:
>           uri = uri.substr(pair.length);
>           var match = pair.match(regexp);
>           if (match)
>               return decodeURIComponent(
>                   match[match.length-1].replace(/\+/g, ' '));
>       }
>       return '';
>   },

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Martijn Pieters]

2009/11/5 Maurits van Rees <[hidden email]>:
> When loading most pages of a fresh Plone 4 site it takes a while to
> load and then Firefox (3.0.15 on Ubuntu 9.04) tells me that a script
> has an error or does not respond.  It is line 55 in this script:
> Plone/Products/CMFPlone/skins/plone_ecmascript/jquery.highlightsearchterms.js

The code in question is run for both the current url and the referrer
(if a search engine or our own server, in most cases), but only if
there are GET query parameters. Can you provide an example URL
(referrer or page url) where you see the slowdown?

--
Martijn Pieters

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Martijn Pieters]

2009/11/5 Simone Deponti <[hidden email]>:

> seems like after splitting, & is the first character and so split('&',1)
> returns ['',restOfStuff].
>
> so if we add
>
> if(uri[0] == '&') {
>    if(uri.length > 1)
>        uri = uri.substr(1);
>    else
>        return '';
> }
>
> just after the while() { it should work.

What is the url you get this with?

You can try this out with the plone.app.javascript codebase; the
jquery plugin has tests which work standalone (no Plone install
required):

  https://svn.plone.org/svn/plone/plone.app.javascript/trunk

Execute the tests directly from svn via:

  https://svn.plone.org/svn/plone/plone.app.javascript/trunk/plone/app/javascript/tests/index.html

If someone could come up with a testcase I'd be most grateful.

--
Martijn Pieters

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Simone Deponti-2]

On 11/05/2009 10:32 PM, Martijn Pieters wrote:
>
> What is the url you get this with?
>

I actually tried it offline using Mozilla's Javascript engine from
command line and making up a random url (I just tested the function).

> You can try this out with the plone.app.javascript codebase; the
> jquery plugin has tests which work standalone (no Plone install
> required):
>
>   https://svn.plone.org/svn/plone/plone.app.javascript/trunk
>
> Execute the tests directly from svn via:
>
>   https://svn.plone.org/svn/plone/plone.app.javascript/trunk/plone/app/javascript/tests/index.html
>
> If someone could come up with a testcase I'd be most grateful.
>

Okay, will try to come up with one.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Simone Deponti-2]

Hallo,

On 11/05/2009 10:32 PM, Martijn Pieters wrote:
>
> If someone could come up with a testcase I'd be most grateful.
>

I committed a testcase (along with an additional test hook to bring the
Highlighter class around and not leave it in JS limbo) as r31119.

I tested it from disk it it made my Firefox go kaboom (Mozilla/5.0 (X11;
U; Linux i686; en-US; rv:1.9.1.4) Gecko/20091027 Fedora/3.5.4-1.fc11
Firefox/3.5.4)

I then committed my fix in r31120.

P.S. All the tests I saw were functional tests (or so they seemed to
me). We have no way to unit test the "Javascript classes" in a sane way?
I don't like what I did in r31119 one bit.

Oh, all this in plone.app.javascript :).


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Maurits van Rees-3]

Martijn Pieters, on 2009-11-05:

> 2009/11/5 Maurits van Rees <[hidden email]>:
>> When loading most pages of a fresh Plone 4 site it takes a while to
>> load and then Firefox (3.0.15 on Ubuntu 9.04) tells me that a script
>> has an error or does not respond.  It is line 55 in this script:
>> Plone/Products/CMFPlone/skins/plone_ecmascript/jquery.highlightsearchterms.js
>
> The code in question is run for both the current url and the referrer
> (if a search engine or our own server, in most cases), but only if
> there are GET query parameters. Can you provide an example URL
> (referrer or page url) where you see the slowdown?

- Start with the plone-coredev/branches/4.0 and a fresh, empty Data.fs.

- On the zope root at http://localhost:8080/ click to go to the ZMI.

- There, in the frames, click on 'Add Plone Site'.  That will lead to
  this page:
  http://localhost:8080/@@plone-addsite?site_id=Plone&advanced=true

- Accept the default answers and click 'Create Plone Site'.

- This will of course take a while but once the fresh Plone Site is
  loaded, I get the error.

Cheers,

--
Maurits van Rees | http://maurits.vanrees.org/
            Work | http://zestsoftware.nl/
"This is your day, don't let them take it away." [Barlow Girl]


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Martijn Pieters]

2009/11/5 Simone Deponti <[hidden email]>:
> I committed a testcase (along with an additional test hook to bring the
> Highlighter class around and not leave it in JS limbo) as r31119.

Cool, thanks! I refactored this a little to make this a little cleaner
(in my opinion). I'll probably refactor the uri '&' test a little once
I get through a debug session or two to figure out what goes on.

> P.S. All the tests I saw were functional tests (or so they seemed to
> me). We have no way to unit test the "Javascript classes" in a sane way?
> I don't like what I did in r31119 one bit.

I agree; javascript's scoping rules are a pain to have to work with. I
now provide the Highlighter class as a _highlighter variable on the
plugin to facilitate testing, following your lead.

--
Martijn Pieters

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Martijn Pieters]

2009/11/5 Maurits van Rees <[hidden email]>:
> - Start with the plone-coredev/branches/4.0 and a fresh, empty Data.fs.
>
> - On the zope root at http://localhost:8080/ click to go to the ZMI.
>
> - There, in the frames, click on 'Add Plone Site'.  That will lead to
>  this page:
>  http://localhost:8080/@@plone-addsite?site_id=Plone&advanced=true

The 'go to the ZMI' step was the crucial difference, thanks Maurits!

--
Martijn Pieters

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Martijn Pieters]

2009/11/8 Martijn Pieters <[hidden email]>:
> Cool, thanks! I refactored this a little to make this a little cleaner
> (in my opinion). I'll probably refactor the uri '&' test a little once
> I get through a debug session or two to figure out what goes on.

So, this was basically caused by a big omission on my part; the code
didn't deal at all well with more than one parameter. I've changed
your solution to an explicit 'lstrip' of ampersands (javascript
equivalent).

Thanks again for debugging this!

--
Martijn Pieters

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Simone Deponti-2]

On 11/08/2009 03:41 PM, Martijn Pieters wrote:
>
> Thanks again for debugging this!
>

You're welcome!

I gave a look at the code, and it's indeed much cleaner than my solution.
I was however thinking if it wasn't better, in the "lstrip" [1]
equivalent to use + instead of *.
Right now, it does not make any difference (if there is no & at the
beginning, inserts an empty string at the beginning of the line anyway,
which amounts to not changing it) but I think it's more straightforward
to specify that there is to be atleast one & to perform the replace.

I've attached the diff (okay, the change amounts to one character).

[1] I can't believe Javascript does not have such a basic function:
Mozilla has an incompatible function trim() that however does only trim
whitespace. Isn't there any tiny js library that has all this sort of
utility functions? Without having to pull in an entire framework, I
mean. Reimplementing the wheel each time is tedious.

--
Simone Deponti

Index: plone/app/javascript/browser/jquery.highlightsearchterms.js
===================================================================
--- plone/app/javascript/browser/jquery.highlightsearchterms.js (revision 31206)
+++ plone/app/javascript/browser/jquery.highlightsearchterms.js (working copy)
@@ -51,7 +51,7 @@
             if (uri.indexOf('?') < 0) return '';
             uri = uri.substr(uri.indexOf('?') + 1);
             while (uri.indexOf('=') >= 0) {
-                uri = uri.replace(/^\&*/, '');
+                uri = uri.replace(/^\&+/, '');
                 var pair = uri.split('&', 1)[0];
                 uri = uri.substr(pair.length);
                 var match = pair.match(regexp);

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Martijn Pieters]

2009/11/8 Simone Deponti <[hidden email]>:
> I gave a look at the code, and it's indeed much cleaner than my solution.
> I was however thinking if it wasn't better, in the "lstrip" [1]
> equivalent to use + instead of *.
> Right now, it does not make any difference (if there is no & at the
> beginning, inserts an empty string at the beginning of the line anyway,
> which amounts to not changing it) but I think it's more straightforward
> to specify that there is to be atleast one & to perform the replace.
>
> I've attached the diff (okay, the change amounts to one character).

I don't think changing the * to a + adds anything. It doesn't make it
more readable, and I don't think it'll make a dicky-bird of difference
performance wise. Let's leave it as it is and save us the commit and
merge headache. ;-)

> [1] I can't believe Javascript does not have such a basic function:
> Mozilla has an incompatible function trim() that however does only trim
> whitespace. Isn't there any tiny js library that has all this sort of
> utility functions? Without having to pull in an entire framework, I
> mean. Reimplementing the wheel each time is tedious.

*shrug* I find jquery does the things that matter well enough, and a
quick strip of starting or ending characters with a replace(regexp,
'') is easy enough to whip up. :-)

On the whole Javascript is a very poor language though. Lucky for us,
future development of the language does seem to be driven by ideas
taken from python as Brendan Eich seems to have drunk the python
cool-aid.

--
Martijn Pieters

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Simone Deponti-2]

On 11/08/2009 05:40 PM, Martijn Pieters wrote:
> I don't think changing the * to a + adds anything. It doesn't make it
> more readable, and I don't think it'll make a dicky-bird of difference
> performance wise. Let's leave it as it is and save us the commit and
> merge headache. ;-)

+1. My only concern is that a broken regex implementation could fail,
but we have tests.

> *shrug* I find jquery does the things that matter well enough, and a
> quick strip of starting or ending characters with a replace(regexp,
> '') is easy enough to whip up. :-)

+1 here too. And strip is a bad example, but there is a limited
selection of stuff I really miss in JS. Like say, Python. Anyway this
discussion's OT ;)

> On the whole Javascript is a very poor language though. Lucky for us,
> future development of the language does seem to be driven by ideas
> taken from python as Brendan Eich seems to have drunk the python
> cool-aid.

Amen and let's pray the browser Gods to bring us down more goodness:
including the bloodthirsty God IE that demanded the sacrifice of so many
work hours in His honor.


--
Simone Deponti


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers

[Maurits van Rees-3]

Martijn Pieters, on 2009-11-08:

> 2009/11/5 Maurits van Rees <[hidden email]>:
>> - Start with the plone-coredev/branches/4.0 and a fresh, empty Data.fs.
>>
>> - On the zope root at http://localhost:8080/ click to go to the ZMI.
>>
>> - There, in the frames, click on 'Add Plone Site'.  That will lead to
>>  this page:
>>  http://localhost:8080/@@plone-addsite?site_id=Plone&advanced=true
>
> The 'go to the ZMI' step was the crucial difference, thanks Maurits!

Works like a charm now, thanks to both of you for fixing it!

--
Maurits van Rees | http://maurits.vanrees.org/
            Work | http://zestsoftware.nl/
"This is your day, don't let them take it away." [Barlow Girl]


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers