Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#1242 closed Bug (fixed)

Comments are not always stripped when compressing

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone:
Component: Project : CKPackager Version:
Keywords: Confirmed Review+ Cc:

Description

The following input:

if ( /*is.ns5 &&*/ subDivs[idx].className=='DLG_edittext' && subDivs[idx].style.width ) // fix firefox bug
    button152.enable(bMode&&bEdit/*TODO &&theApp.clipboard.canPaste(theApp.clipboard.titleData.title.arChld[0])*/);  //

var testFunc = function(tempNode) { 
  var fs = tempNode.style.fontSize; 
  fs = parseInt(fs); //.substr(0,fs.length-2); // remove px
  fs = Math.round( fs / 1.3333 ) + "pt"; // convert to points
  var bs = fs * 2; 
  return bs 
}

Outputs like this:

if (/*is.ns5&&*/ subDivs[idx].className=='DLG_edittext' && subDivs[idx].style.width ) //fix firefox bug button152.enable(bMode&&bEdit/*TODO&&theApp.clipboard.canPaste(theApp.clipboard.titleData.title.arChld[0])*/);  //var testFunc=function(A) {var B=A.style.fontSize;B=parseInt(B);B=Math.round(B / 1.3333 ) + "pt"; //convert to points var C=B*2;return C};

The comments have not been stripped out, breaking the code in some cases.

Attachments (2)

1242.patch (573 bytes) - added by Frederico Caldeira Knabben 11 years ago.
1242_2.patch (1.1 KB) - added by Frederico Caldeira Knabben 11 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 years ago by Wojciech Olchawa

Keywords: Confirmed added

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 1242.patch added

comment:2 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review? added

The problem here is in the regex used to isolate and save strings and regular expressions. It was not looking for the "*" char before "/" in the regular expression start.

There is also a big problem with the division literal (/). In the following example:

var a = 1 ; b = 1 ; g = 1 ;
var c = a / b / g ;

The processor was considering / b / as a regular expression.

There would be possible to solve this problem, but PHP doesn't support flexible length on lookbehind assertions. So, the patch also includes two further assertions that impose some rules for regular expressions and divisions:

  • There must be a semicolon at the end of lines containing regular expressions. It helps us avoiding the problem in the following line:
fs = Math.round( fs / 1.3333 ) + "pt"; // convert to points
  • There must be a space after the division character. So, "(((a / b / g)))" is considered a division. If instead we have "a/b/g", "/b/" is considered a regular expression.

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 1242_2.patch added

comment:3 Changed 11 years ago by Frederico Caldeira Knabben

Opps... I've wrongly reverted some changes before creating the first patch. The new one should be ok.

comment:4 Changed 11 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: newassigned

#1343 must be fixed together with this one, otherwise our code will get broken.

comment:5 Changed 11 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:6 Changed 11 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [2237].

comment:7 Changed 11 years ago by Frederico Caldeira Knabben

The rule of requiring lines containing regexes to be terminated by ; does not apply to cases like the following:

if ( /regex/.test( '' ) )

So, [2237] broke our code. I've fixed it with [2242], so now only division literals are required to be followed by spaces.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy