Build: Compressed code "OpenLayers.js" can be reduced by 9k

[jorix]

"OpenLayers.js" code in:

if(!singleFile) {
    var jsfiles = new Array(
        "OpenLayers/Util.js",
    ...    
}

never run in single file mode, and has a size of 8k. (6.25% using lite.cfg)

The patch proposes changes in "tools/mergejs.py" to can specify the exclusion of part of the code with:

// @exclude code:start
 ... code to exclude ...
// @exclude code:end

The patch adds "OpenLayers.js" modified to exclude the "if"

(it is possible to exclude more lines, but I choose to keep the code clean)

[fredj]

It's a very good idea to exclude these lines from the build but I don't think that the '@exclude' system is the way to do that.

My proposal is to remove the jsfiles array from OpenLayers.js and move it to an other file (not included in the build).

patch to come.

[fredj]

move jsfiles to OpenLayers.Loader.js, add OpenLayers._loadScripts function

[erilem]

I also recently realized that we can significantly reduce the size of the lib by moving the jsfiles list elsewhere. I guess we may want to go one step further and also move the code that deals with loading the script files.

[fredj]

Replying to erilem:

I also recently realized that we can significantly reduce the size of the lib by moving the jsfiles list elsewhere. I guess we may want to go one step further and also move the code that deals with loading the script files.

It was my first intention to move this code away from OpenLayers.js but this file still need to load OpenLayers.Loader.js.

[ahocevar]

What about the above patch? Let's no longer include OpenLayers.js in the build profiles, and make SingleFile.js smart enough to provide the _getScriptLocation method. The code here is simpler, because we know that we have to look at the last script tag.

[erilem]

Replying to ahocevar:

What about the above patch? Let's no longer include OpenLayers.js in the build profiles, and make SingleFile.js smart enough to provide the _getScriptLocation method. The code here is simpler, because we know that we have to look at the last script tag.

Why would OpenLayers always be the last script tag?

[ahocevar]

Replying to erilem:

Why would OpenLayers always be the last script tag?

At the time this code is executed, it is the last script tag. This is because of the way a browser builds up the dom. I just confirmed this with Safari 5, Firefox 3.6 and IE6.

[erilem]

Replying to ahocevar:

Replying to erilem:

Why would OpenLayers always be the last script tag?

At the time this code is executed, it is the last script tag. This is because of the way a browser builds up the dom. I just confirmed this with Safari 5, Firefox 3.6 and IE6.

So shouldn't it be the same when using autoload?

[ahocevar]

Same as above, but with the _getScriptLocation improvement also in the loader

[ahocevar]

The above won't pass OpenLayers.html tests, because of the new _getScriptLocation closure.

[ahocevar]

Same as above, but with new tests.

[ahocevar]

Note that I had to split the tests from OpenLayers.html into three separate test files. The test coverage is the same, I only also added a script tag after OpenLayers.js to make sure that the we can really rely on the last script tag in the _getScriptLocation closure.

Tests pass in IE6, FF3.6 and Safari5. Thanks for any review.

[erilem]

same as .3 with OpenLayers._scriptName removed

[erilem]

openlayers-2873.4.2.patch removes the OpenLayers._scriptName property, and OpenLayers.js to the exclude list in library.cfg and full.cfg.

Tests pass in FF3, FF nightly, IE7 and Chrome 5.

The "lite" build is 128K without the patch, and 119K with the patch, so we win 9K.

Andreas, please commit.

[ahocevar]

in with r10822

[erilem]

And thanks a lot jorix for bringing up the issue.

[fredj]

in OpenLayers/SingleFile.js, the _getScriptLocation function crash if the src don't match the regexp.

Before -r10822, the function returned an empty string.

[fredj]

Index: lib/OpenLayers/SingleFile.js
===================================================================
--- lib/OpenLayers/SingleFile.js	(revision 10822)
+++ lib/OpenLayers/SingleFile.js	(working copy)
@@ -7,7 +7,8 @@
     singleFile: true,
     _getScriptLocation: (function() {
         var s = document.getElementsByTagName('script');
-        var l = s[s.length-1].getAttribute("src").match(/(^|(.*?\/))(OpenLayers\.js)(\?|$)/)[1];
+        var match = s[s.length-1].getAttribute("src").match(/(^|(.*?\/))(OpenLayers\.js)(\?|$)/);
+        var l = match ? match[1] : "";
         return (function() { return l; });
     })()
 };

Works on Chromium and FF3.6. Please review.

[fredj]

r2873

[ahocevar]

Replying to fredj:

r2873

Make that r10823

[ahocevar]

In case people use an old build profile with a script name other than OpenLayers.js, _getScriptLocation from OpenLayers.js will break. The attached patch fixes this also. In addition, I changed the previous fix to use single character names. Not something to do anywhere else, but just here because it's obvious what happens in the code, and saves us a few bytes :-)

[jorix]

Replying to erilem:

And thanks a lot jorix for bringing up the issue.

Thanks, I'm surprised to see much movement in one of my tickets, I am glad it is useful.

Precautions to consider:

The cache introduced in #2389 for "_getScriptLocation" has been lost. It is used in Map.js and in "Util.getImagesLocation"

My first solution was to copy _getScriptLocation in singleFile.js, once prepared, I dismissed it because I thought it would be rejected, ups! :-)
I worked with some premises that can serve:

• Put bold comments in both files warning to future developers of duplicity (missing).
• Can copy and paste the code of _getScriptLocation function to singleFile.js if changes are made (now are different)
• Ensure compatibility with old *.cfg which does not exclude OpenLayers.js (guaranteed)
• Put the same "@requires" in the two files (in "singleFile.js" missing)

What to do with "VERSION_NUMBER"? Now I've done an SVN Update in my local copy and and "OpenLayers.js" says 10822 and "SingleFile.js" says 10823!

[erilem]

Replying to ahocevar:

In case people use an old build profile with a script name other than OpenLayers.js, _getScriptLocation from OpenLayers.js will break.

Excellent catch.

The attached patch fixes this also. In addition, I changed the previous fix to use single character names. Not something to do anywhere else, but just here because it's obvious what happens in the code, and saves us a few bytes :-)

Also good to me.

Thanks, please commit.

[erilem]

Replying to jorix:

Replying to erilem:

And thanks a lot jorix for bringing up the issue.

Thanks, I'm surprised to see much movement in one of my tickets, I am glad it is useful. Precautions to consider: The cache introduced in #2389 for "_getScriptLocation" has been lost. It is used in Map.js and in "Util.getImagesLocation"

It's still there. The code is:

_getScriptLocation: (function() { 
    var r = new RegExp("(^|(.*?\\/))(" + scriptName + ")(\\?|$)"); 
    var s = document.getElementsByTagName('script'); 
    var m = s[s.length-1].getAttribute("src").match(r); 
    var l = m ? m[1] : ""; 
    return (function() { return l; }); 
})()

The location is cached in the l variable. The anonymous function that finds the location is executed once, when the script is loaded in the page.

[jorix]

Replying to erilem:

The location is cached in the l variable. The anonymous function that finds

Ok, I looked at the body of the function and did not see the return and the parentheses in the declaration, very elegant and compact!

I think it is appropriate to make a comment in the code of the two files to warn of "_getScriptLocation" duplicity.

[ahocevar]

r10831

[ahocevar]

Replying to jorix:

I think it is appropriate to make a comment in the code of the two files to warn of "_getScriptLocation" duplicity.

Done in r10831.

[ahocevar]

Replying to jorix:

My first solution was to copy _getScriptLocation in singleFile.js, once prepared, I dismissed it because I thought it would be rejected, ups! :-)

erilem was indeed not too happy with this solution.

I worked with some premises that can serve: * Put bold comments in both files warning to future developers of duplicity (missing).

done in r10831

* Can copy and paste the code of _getScriptLocation function to singleFile.js if changes are made (now are different)

except for the regular expression, yes. We can do that in 3.0 when we stop supporting build profiles that include OpenLayers.js - I added comments about it.

* Put the same "@requires" in the two files (in "singleFile.js" missing)

Should be handled elsewhere.

What to do with "VERSION_NUMBER"? Now I've done an SVN Update in my local copy and and "OpenLayers.js" says 10822 and "SingleFile.js" says 10823!

VERSION_NUMBER is only relevant for releases. It is set manually by the release manager. I'll update the release procedure wiki to explain this step.

[erilem]

Reopening. There are issues with IE8. This is because IE8 downloads scripts in parallel (*), and doesn't wait for a script to be fully evaluated before adding another <script> tag to the DOM. So this code doesn't work:

var m = s[s.length-1].getAttribute("src").match(/(^|(.*?\/))(OpenLayers\.js)(\?|$)/);

Patch to come.

(*)  http://ajaxian.com/archives/ie-8-and-performance

[erilem]

See openlayers-2873.6.patch . Tests continue to pass. Please review.

[ahocevar]

@erilem: I'd be interested to see a test case that shows this problem. The OpenLayers2.html test should fail if what you're saying is true.

[erilem]

Replying to ahocevar:

@erilem: I'd be interested to see a test case that shows this problem. The OpenLayers2.html test should fail if what you're saying is true.

I knew you were going to say that and request a test case :-) And yes, it looks like my case is different than OpenLayers2.html. But anyway, I think this code is fragile and even wrong, because we can't assume that the browser won't add new script tags to the DOM before it's done with the evaluation of OpenLayers.js, no?

[fredj]

with openlayers-2873.7.patch, tests pass with ie8, FF 3.6 and chrome

[erilem]

openlayers-2873.7.patch includes a new test, OpenLayers4.html. Without modifications to _getScriptLocation, this test fails in IE6, IE7 and IE8, and passes in FF3, FF4, Chrome6 and Safari4. With my modifications to _getScriptLocation the test passes in all browsers. Please review.

[ahocevar]

Very strange way to load scripts. But since you ran into this problem, I guess there must indeed be cases where things are done this way. Please commit.

[erilem]

Replying to ahocevar:

Very strange way to load scripts. But since you ran into this problem, I guess there must indeed be cases where things are done this way. Please commit.

Strange way, yes, but strange environment too. Anyway, as I said, I think this is wrong to assume that the browser won't add new script tags to the DOM before it's done with the evaluation of OpenLayers.js.

Thanks a lot for the review Andreas.

[erilem]

r10862