Issue 29336525: Issue 3584 - Work around WebKit uppercase ID matching bug (Closed)
|
Created:
Feb. 16, 2016, 7:43 p.m. by kzar Modified:
Feb. 17, 2016, 4:02 p.m. Reviewers:
Sebastian Noack CC:
dean Visibility:
Public. |
DescriptionIssue 3584 - Work around WebKit uppercase ID matching bug
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use [id="Whatever"] syntax #
Total comments: 2
Patch Set 3 : Don't forget IDs at the end of the string #
Total comments: 13
Patch Set 4 : Addressed more feedback #
Total comments: 6
Patch Set 5 : Addressed nit I missed before #Patch Set 6 : Addressed further feedback #MessagesTotal messages: 10
Patch Set 1
https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js#ne... abp2blocklist.js:296: // FIXME - Hack to work around WebKit bug whereby element IDs are I feel that this won't be just a temporary workaround. The earliest this will get fixed is the next Safari major release, leaving some devices behind. So that we cannot get rid of this workaround any time soon. So it seems more appropriate to say "as of Safari 9.0" instead of "FIXME". https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js#ne... abp2blocklist.js:299: /#[\w-]+/g, match => match.toLowerCase() ID selectors can have escaped special chars, e.g #foo\:bar. ID selectors can also use non-ascii characters (which aren't matched by \w) without escaping. https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js#ne... abp2blocklist.js:302: addRule({ Ideally, we merge the lowercase variants with the other selectors rather than adding another rule.
Patch Set 2 : Use [id="Whatever"] syntax https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js#ne... abp2blocklist.js:296: // FIXME - Hack to work around WebKit bug whereby element IDs are On 2016/02/17 08:04:28, Sebastian Noack wrote: > I feel that this won't be just a temporary workaround. The earliest this will > get fixed is the next Safari major release, leaving some devices behind. So that > we cannot get rid of this workaround any time soon. So it seems more appropriate > to say "as of Safari 9.0" instead of "FIXME". Done. https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js#ne... abp2blocklist.js:299: /#[\w-]+/g, match => match.toLowerCase() On 2016/02/17 08:04:28, Sebastian Noack wrote: > ID selectors can have escaped special chars, e.g #foo\:bar. ID selectors can > also use non-ascii characters (which aren't matched by \w) without escaping. Acknowledged. https://codereview.adblockplus.org/29336525/diff/29336526/abp2blocklist.js#ne... abp2blocklist.js:302: addRule({ On 2016/02/17 08:04:28, Sebastian Noack wrote: > Ideally, we merge the lowercase variants with the other selectors rather than > adding another rule. Acknowledged.
Patch Set 3 : Don't forget IDs at the end of the string
https://codereview.adblockplus.org/29336525/diff/29336528/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336528/abp2blocklist.js#ne... abp2blocklist.js:283: else if (chr < "0" && chr != "-" || I'd find the logic here easier to read if you'd check for the special cases separately rather than in between the range checks: else if (chr != "-" && chr != "_" && (chr < "0" || chr > "9" && chr < "A" chr > "Z" && chr < "a" chr > "z" && chr < "\x80")) https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:259: if (selector.indexOf("#") == -1) Does this optimization has any measurable impact on the overall performance? https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:288: IDpositions.push({start: IDstart, end: i}); I wonder we should do the replace inline: selector = selector.substring(0, IDstart) + "[id=" + selector.substring(IDstart + 1, i) + "]" + selector.substring(i); i += 4; This logic would have to move to a function, to avoid duplication. Also we might want to calculate the offset (4) rather than hardcoding it. But I could imagine that it will still be simpler than the dance we do below, plus we would need less state. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:296: if (IDpositions.length == 0) Does this optimization has any measurable impact on the overall performance? https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:305: newSelector += '[id="' + selector.substring(ID.start + 1, ID.end) + '"]'; The quotes are actually redundant, as we copy an unquoted token here, which cannot possibly have any spaces.
https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:257: function attributeSyntaxForIDs(selector) Perhaps a better name for this function: convertIDSelectorsToAttributeSelectors https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:264: let IDstart = null; Nit: Since we only have one kind of start value and positions, omit the ID prefix? It looks kinda ugly as it breaks the camel case notation. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:288: IDpositions.push({start: IDstart, end: i}); On 2016/02/17 14:22:19, Sebastian Noack wrote: > I wonder we should do the replace inline: > > selector = selector.substring(0, IDstart) + > "[id=" + selector.substring(IDstart + 1, i) + "]" + > selector.substring(i); > i += 4; > > This logic would have to move to a function, to avoid duplication. Also we might > want to calculate the offset (4) rather than hardcoding it. But I could imagine > that it will still be simpler than the dance we do below, plus we would need > less state. Here is an example how that could look like: function replaceIDSubSelector(selector, start , end) { return selector.substring(0, start) + "[id=" + selector.substring(start + 1, end) + "]" + selector.substring(end); } ... let newSelector = replaceIDSubSelector(selector, IDStart, i); i += newSelector.length - selector.length; selector = newSelector;
Patch Set 4 : Addressed more feedback https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:257: function attributeSyntaxForIDs(selector) On 2016/02/17 14:40:59, Sebastian Noack wrote: > Perhaps a better name for this function: convertIDSelectorsToAttributeSelectors Done. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:259: if (selector.indexOf("#") == -1) On 2016/02/17 14:22:19, Sebastian Noack wrote: > Does this optimization has any measurable impact on the overall performance? No, removed. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:264: let IDstart = null; On 2016/02/17 14:40:59, Sebastian Noack wrote: > Nit: Since we only have one kind of start value and positions, omit the ID > prefix? It looks kinda ugly as it breaks the camel case notation. Done. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:288: IDpositions.push({start: IDstart, end: i}); On 2016/02/17 14:41:00, Sebastian Noack wrote: > On 2016/02/17 14:22:19, Sebastian Noack wrote: > > I wonder we should do the replace inline: > > > > selector = selector.substring(0, IDstart) + > > "[id=" + selector.substring(IDstart + 1, i) + "]" + > > selector.substring(i); > > i += 4; > > > > This logic would have to move to a function, to avoid duplication. Also we > might > > want to calculate the offset (4) rather than hardcoding it. But I could > imagine > > that it will still be simpler than the dance we do below, plus we would need > > less state. > > Here is an example how that could look like: > > function replaceIDSubSelector(selector, start , end) > { > return selector.substring(0, start) + > "[id=" + selector.substring(start + 1, end) + "]" + > selector.substring(end); > } > > ... > > let newSelector = replaceIDSubSelector(selector, IDStart, i); > i += newSelector.length - selector.length; > selector = newSelector; Mind if I leave this as is? It works fine and I find it easier to understand how it is. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:296: if (IDpositions.length == 0) On 2016/02/17 14:22:20, Sebastian Noack wrote: > Does this optimization has any measurable impact on the overall performance? No, removed. https://codereview.adblockplus.org/29336525/diff/29336530/abp2blocklist.js#ne... abp2blocklist.js:305: newSelector += '[id="' + selector.substring(ID.start + 1, ID.end) + '"]'; On 2016/02/17 14:22:19, Sebastian Noack wrote: > The quotes are actually redundant, as we copy an unquoted token here, which > cannot possibly have any spaces. Acknowledged.
https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js#ne... abp2blocklist.js:269: else if (chr == sep) // don't split within quoted text Nit: This code has bee ncopied from cssProperties.js, however this comment isn't accurrate in the context here. Perhaps "touch" instead of "split"? https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js#ne... abp2blocklist.js:294: let newSelector = ""; Concatenating strings in a loop is expensive. You usually would use an array and join it afterwards. https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js#ne... abp2blocklist.js:296: for (let ID of positions) Nit: The variable shouldn't start with uppercase. "position" or "pos" seems to be a better name here anyway.
Patch Set 5 : Addressed nit I missed before Patch Set 6 : Addressed further feedback https://codereview.adblockplus.org/29336525/diff/29336528/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336528/abp2blocklist.js#ne... abp2blocklist.js:283: else if (chr < "0" && chr != "-" || On 2016/02/17 14:22:19, Sebastian Noack wrote: > I'd find the logic here easier to read if you'd check for the special cases > separately rather than in between the range checks: > > else if (chr != "-" && > chr != "_" && (chr < "0" || > chr > "9" && chr < "A" > chr > "Z" && chr < "a" > chr > "z" && chr < "\x80")) Done. https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js#ne... abp2blocklist.js:269: else if (chr == sep) // don't split within quoted text On 2016/02/17 15:24:37, Sebastian Noack wrote: > Nit: This code has bee ncopied from cssProperties.js, however this comment isn't > accurrate in the context here. Perhaps "touch" instead of "split"? Done. https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js#ne... abp2blocklist.js:294: let newSelector = ""; On 2016/02/17 15:24:37, Sebastian Noack wrote: > Concatenating strings in a loop is expensive. You usually would use an array and > join it afterwards. Done. https://codereview.adblockplus.org/29336525/diff/29336532/abp2blocklist.js#ne... abp2blocklist.js:296: for (let ID of positions) On 2016/02/17 15:24:37, Sebastian Noack wrote: > Nit: The variable shouldn't start with uppercase. "position" or "pos" seems to > be a better name here anyway. Done.
LGTM 
|
