Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412533002:
[i18n] Avoid name conflicts caused by naive minifier (Closed)
Description[i18n] Avoid name conflicts caused by minifier
Our minifier (tools/jsmin.py) shortens variable names it comes across.
It generally tries to avoid name conflicts caused by renamed variables,
but cannot handle lambda function syntax.
This is what happens here. Both lambda function parameters 'x' and 'y'
are not recognized as identifiers by the minifier and it thus potentially
causes naming conflicts.
BUG=v8:5505
Committed: https://crrev.com/32b2d7c8148683da6c04a5e35172b430fa82428a
Cr-Commit-Position: refs/heads/master@{#40198}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Format #Messages
Total messages: 20 (12 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [i18n] Avoid name conflicts caused by naive minifier Our minifier (tools/jsmin.py) shortens variable names it comes across. However, it is fairly stupid and can, for instance, rename a function parameter to 'x' even though 'x' is already in use within the function body. This is what happens here. Both 'x' and 'y' could cause name collisions with the minified parameter names. Work around this by choosing longer variable names which the minifier does not clobber. BUG= ========== to ========== [i18n] Avoid name conflicts caused by minifier Our minifier (tools/jsmin.py) shortens variable names it comes across. It generally tries to avoid name conflicts caused by renamed variables, but cannot handle lambda function syntax. This is what happens here. Both lambda function parameters 'x' and 'y' are not recognized as identifiers by the minifier and it thus potentially causes naming conflicts. BUG=v8:5505 ==========
jgruber@chromium.org changed reviewers: + yangguo@chromium.org
LGTM. How hard would it be to teach JSMin about arrow functions? https://codereview.chromium.org/2412533002/diff/1/src/js/i18n.js File src/js/i18n.js (right): https://codereview.chromium.org/2412533002/diff/1/src/js/i18n.js#newcode79 src/js/i18n.js:79: boundMethod = ANONYMOUS_FUNCTION((fst, snd) => implementation(this, fst, snd)); can we keep the 80-char limit?
I've thought about that, and it's on my TODO list. Sounds like very delicate work though. As discussed yesterday, it would be awesome to replace jsmin.py with a real minifier. https://codereview.chromium.org/2412533002/diff/1/src/js/i18n.js File src/js/i18n.js (right): https://codereview.chromium.org/2412533002/diff/1/src/js/i18n.js#newcode79 src/js/i18n.js:79: boundMethod = ANONYMOUS_FUNCTION((fst, snd) => implementation(this, fst, snd)); On 2016/10/12 04:56:55, Yang wrote: > can we keep the 80-char limit? Done.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2412533002/#ps20001 (title: "Format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/12 07:03:08, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or.... we only have to keep this working until we don't ship any JS code anymore.
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [i18n] Avoid name conflicts caused by minifier Our minifier (tools/jsmin.py) shortens variable names it comes across. It generally tries to avoid name conflicts caused by renamed variables, but cannot handle lambda function syntax. This is what happens here. Both lambda function parameters 'x' and 'y' are not recognized as identifiers by the minifier and it thus potentially causes naming conflicts. BUG=v8:5505 ========== to ========== [i18n] Avoid name conflicts caused by minifier Our minifier (tools/jsmin.py) shortens variable names it comes across. It generally tries to avoid name conflicts caused by renamed variables, but cannot handle lambda function syntax. This is what happens here. Both lambda function parameters 'x' and 'y' are not recognized as identifiers by the minifier and it thus potentially causes naming conflicts. BUG=v8:5505 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [i18n] Avoid name conflicts caused by minifier Our minifier (tools/jsmin.py) shortens variable names it comes across. It generally tries to avoid name conflicts caused by renamed variables, but cannot handle lambda function syntax. This is what happens here. Both lambda function parameters 'x' and 'y' are not recognized as identifiers by the minifier and it thus potentially causes naming conflicts. BUG=v8:5505 ========== to ========== [i18n] Avoid name conflicts caused by minifier Our minifier (tools/jsmin.py) shortens variable names it comes across. It generally tries to avoid name conflicts caused by renamed variables, but cannot handle lambda function syntax. This is what happens here. Both lambda function parameters 'x' and 'y' are not recognized as identifiers by the minifier and it thus potentially causes naming conflicts. BUG=v8:5505 Committed: https://crrev.com/32b2d7c8148683da6c04a5e35172b430fa82428a Cr-Commit-Position: refs/heads/master@{#40198} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/32b2d7c8148683da6c04a5e35172b430fa82428a Cr-Commit-Position: refs/heads/master@{#40198} |
