1141748 - Fix in-tree consumers that use non-standard flag argument of String.prototype.{search,match,replace} in CSSUnprefixingService.js.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1131107.

Before fixing bug 1108382, we need to remove all consumers of flag argument.

https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSUnprefixingService.js#145
>        newRightHalf = newRightHalf.replace(strToReplace, replacement, "g");

I think this could be rewritten by .split().join().
I'll post patch shortly.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Do not use non-standard flag argument of String.prototype.replace in CSSUnprefixingService.js.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d24e844aee7

Comment 2

[Daniel Holbert [:dholbert]]

I wonder if split()/join() is slower than replace()?

The disclaimer at the beginning of this article suggests that replace() is faster these days:
  http://www.adequatelygood.com/JS-Find-and-Replace-with-SplitJoin.html
and a one-off local test at http://jsperf.com/test-join-and-split says that split/join is 30% slower.

Perhaps we should replace "stringMap" here with just an array of regular expressions, which include "/g" as part of the regex, and then we can just pass those directly to replace().

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8575635 [details] [diff] [review]
Do not use non-standard flag argument of String.prototype.replace in CSSUnprefixingService.js.

r- since this seems to be slower, per previous comment.

Could you replace "propInfo.stringMap" here with "propInfo.regexpArray", and replace the single map-entry with the equivalent regexp literal, which I think should be:
  /-webkit-transform/transform/g

You can test locally if that breaks anything by rebuilding & running:
 ./mach mochitest-plain layout/style/test/test_unprefixing_service.html

Comment 4

[Daniel Holbert [:dholbert]]

Sorry, I was mistakenly thinking that JS supported "/old/new/g" regex syntax; but from local testing, it looks like it only supports string.replace(/old/g, "new");

So: really I think we need to just replace the string "-webkit-transform" with this regexp-literal:
  /-webkit-transform/g

And then we can drop the "g" flags argument, since it'll be implicit in the first argument.

Comment 5

[Daniel Holbert [:dholbert]]

So I think we really just need to change this one line:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/CSSUnprefixingService.js#135
to use /-webkit-transform/g instead of "-webkit-transform".

Then we can drop the "g" arg at line 145.

(we should probably also rename "strToReplace" to "regexpToReplace", too.)

Sound good / make sense?

Comment 6

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

according to bug 1131107 comment #19, .split().join() is fastest on JIT (ion) execution, but it seems to be slower on non-JIT execution.
so, converting stringMap to regexpArray would be better here.

since regexp could not be a object key, I'll convert it into array of arrays with a pair of regexp and replacement.
I'll post fixed patch after local test.

Comment 7

[Daniel Holbert [:dholbert]]

Ah, didn't realize regexp can't be used as an object key. So comment 5 doesn't work then.

I also don't know if we can assume anything about being JIT vs. non-JIT here. This JS function gets called into from C++, and I don't know how quickly we JIT scripts that are invoked as helper-functions by C++. If it's possible we'll be jitted, I'm also OK sticking with your original .split().join() solution.

Your array-of-arrays suggestion sounds reasonable too, though.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8575635 [details] [diff] [review]
Do not use non-standard flag argument of String.prototype.replace in CSSUnprefixingService.js.

After some discussion with shu in #jsapi, it sounds like there's little-to-no difference here after all, in this case. (And JS regexps are less awesome than I thought.)

So, r+ on original patch. Thanks!

Comment 9

[Tooru Fujisawa [:arai]]

Thank you! :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b9a2f9d6e4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3b9a2f9d6e4