Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863573004:
Prepare Chrome Extensions for ES2015 RegExp semantics (Closed)
|
Created:
4 years, 8 months ago by nickie Modified:
4 years, 8 months ago CC:
Dan Ehrenberg, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, Michael Hablich Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrepare Chrome Extensions for ES2015 RegExp semantics
This patch maintains Chrome Extensions infrastructure resilient against
monkey-patching changes to RegExp.prototype by using exec directly.
ES2015 makes methods like RegExp.prototype.test and String.prototype.replace
call the current, rather than original, value of RegExp.prototype.exec.
This patch avoids calling those functions.
BUG=v8:4883
R=rdevlin.cronin@chromium.org
TEST=ran previously failing browser_tests with a version of V8 which includes
full ES2015 RegExp semantics, and observed it to pass.
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Minor change and fix rebase #
Total comments: 10
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Fix for 4883 BUG=v8:4883 ========== to ========== Fix for 4883 BUG=v8:4883 ==========
The CQ bit was checked by hablich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863573004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863573004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863573004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863573004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by adamk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863573004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863573004/40001
Description was changed from ========== Fix for 4883 BUG=v8:4883 ========== to ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch makes Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org ==========
littledan@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - adamk@chromium.org, hablich@chromium.org, littledan@chromium.org
Description was changed from ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch makes Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org ========== to ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch makes Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org TEST=ran previously failing browser_tests with a version of V8 which includes full ES2015 RegExp semantics, and observed it to pass. ==========
littledan@chromium.org changed reviewers: + littledan@chromium.org
littledan@chromium.org changed reviewers: + meacer@chromium.org - littledan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'd also update the description a bit to make it clear that this is just maintaining the resilience, rather than making anything more resilient. You could just scrap the first sentence in the CL description. https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... File extensions/renderer/resources/binding.js (right): https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... extensions/renderer/resources/binding.js:305: // If the first character is a digit (we know it must be one of We should add a new comment here explaining why this is so tedious. e.g. // The built-in versions of $String.replace call other built-ins, which // may be clobbered. Instead, manually build the property name. https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... extensions/renderer/resources/binding.js:309: for (var i = 0; i < enumValue.length; i++) { ++i https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... extensions/renderer/resources/binding.js:310: if (i > 0 && $RegExp.exec(/[a-z]/, enumValue[i-1]) && I think this block would be a little easier to read with something like: var next = ''; if (...) next = '_' + enumValue[i]; else if (...) next = '_' else next = enumValue[i]; propertyName += next; https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... File extensions/renderer/safe_builtins.cc (left): https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... extensions/renderer/safe_builtins.cc:83: " ['test']);\n" You need to update the places that used $RegExp.test.
https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... File extensions/renderer/safe_builtins.cc (right): https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... extensions/renderer/safe_builtins.cc:83: " ['exec']);\n" Also, might be worth adding a comment here explaining why we use exec instead of test (which is normally preferred).
Description was changed from ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch makes Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org TEST=ran previously failing browser_tests with a version of V8 which includes full ES2015 RegExp semantics, and observed it to pass. ========== to ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch maintains Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org TEST=ran previously failing browser_tests with a version of V8 which includes full ES2015 RegExp semantics, and observed it to pass. ==========
littledan@chromium.org changed reviewers: + littledan@chromium.org
I am not the owner of this patch, so I fixed the issues from the review in https://codereview.chromium.org/1864733002 https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... File extensions/renderer/resources/binding.js (right): https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... extensions/renderer/resources/binding.js:305: // If the first character is a digit (we know it must be one of On 2016/04/05 at 19:15:50, Devlin wrote: > We should add a new comment here explaining why this is so tedious. e.g. > > // The built-in versions of $String.replace call other built-ins, which > // may be clobbered. Instead, manually build the property name. Done https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... extensions/renderer/resources/binding.js:309: for (var i = 0; i < enumValue.length; i++) { On 2016/04/05 at 19:15:50, Devlin wrote: > ++i Done https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/res... extensions/renderer/resources/binding.js:310: if (i > 0 && $RegExp.exec(/[a-z]/, enumValue[i-1]) && On 2016/04/05 at 19:15:50, Devlin wrote: > I think this block would be a little easier to read with something like: > var next = ''; > if (...) > next = '_' + enumValue[i]; > else if (...) > next = '_' > else > next = enumValue[i]; > propertyName += next; Done https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... File extensions/renderer/safe_builtins.cc (left): https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... extensions/renderer/safe_builtins.cc:83: " ['test']);\n" On 2016/04/05 at 19:15:50, Devlin wrote: > You need to update the places that used $RegExp.test. That had already been done in this patch; there was just one usage that I saw. https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... File extensions/renderer/safe_builtins.cc (right): https://codereview.chromium.org/1863573004/diff/40001/extensions/renderer/saf... extensions/renderer/safe_builtins.cc:83: " ['exec']);\n" On 2016/04/05 at 19:16:39, Devlin wrote: > Also, might be worth adding a comment here explaining why we use exec instead of test (which is normally preferred). Added comments in both usage cases (exec is a valid thing to call directly, so not sure why there'd be a comment here).
This was moved and landed as https://codereview.chromium.org/1864733002/
Description was changed from ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch maintains Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org TEST=ran previously failing browser_tests with a version of V8 which includes full ES2015 RegExp semantics, and observed it to pass. ========== to ========== Prepare Chrome Extensions for ES2015 RegExp semantics This patch maintains Chrome Extensions infrastructure resilient against monkey-patching changes to RegExp.prototype by using exec directly. ES2015 makes methods like RegExp.prototype.test and String.prototype.replace call the current, rather than original, value of RegExp.prototype.exec. This patch avoids calling those functions. BUG=v8:4883 R=rdevlin.cronin@chromium.org TEST=ran previously failing browser_tests with a version of V8 which includes full ES2015 RegExp semantics, and observed it to pass. ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
