Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585903002:
Ship Intl.DateTimeFormat.formatToParts() (Closed)
|
Created:
4 years ago by jungshik at Google Modified:
4 years ago CC:
v8-reviews_googlegroups.com, Michael Hablich, adamk, jochen (gone - plz use gerrit) Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionShip Intl.DateTimeFormat.formatToParts()
Move Intl.DateTimeFormat.formatToParts() to HARMONY_SHIPPING bucket.
Spec discussion: https://github.com/tc39/ecma402/issues/30
It's in stage 4 and Firefox shipped it in Firefox 51.0.
(
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/formatToParts#Browser_compatibility )
BUG=v8:5244
TEST=intl/date-format/date-format-to-parts.js
TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/*
Review-Url: https://codereview.chromium.org/2585903002
Cr-Commit-Position: refs/heads/master@{#41871}
Committed: https://chromium.googlesource.com/v8/v8/+/a6749915d3d0167893153526de0ba61b16ab0d26
Patch Set 1 #Patch Set 2 : test/intl/OWNERS update #
Total comments: 4
Patch Set 3 : address comments #
Messages
Total messages: 21 (9 generated)
The CQ bit was checked by jshin@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...
jshin@chromium.org changed reviewers: + littledan@chromium.org
I'll send an email to v8-{dev,users} about intent-to-ship.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/16 22:18:37, jungshik at google wrote:
> I'll send an email to v8-{dev,users} about intent-to-ship.
Email sent to v8-dev and v8-users.
Dan, can you take a look?
Thanks
Just a couple nits, I think this is ready to ship. https://codereview.chromium.org/2585903002/diff/20001/test/intl/OWNERS File test/intl/OWNERS (right): https://codereview.chromium.org/2585903002/diff/20001/test/intl/OWNERS#newcode2 test/intl/OWNERS:2: jshin@chromium.org Could you separate this change into a separate patch? https://codereview.chromium.org/2585903002/diff/20001/test/intl/date-format/d... File test/intl/date-format/date-format-to-parts.js (left): https://codereview.chromium.org/2585903002/diff/20001/test/intl/date-format/d... test/intl/date-format/date-format-to-parts.js:6: Could you leave the Flags: line in until the later patch to remove the flag altogether? This would make a potential revert more minimal.
On 2016/12/16 23:40:08, Dan Ehrenberg wrote: > Just a couple nits, I think this is ready to ship. > > https://codereview.chromium.org/2585903002/diff/20001/test/intl/OWNERS > File test/intl/OWNERS (right): > > https://codereview.chromium.org/2585903002/diff/20001/test/intl/OWNERS#newcode2 > test/intl/OWNERS:2: https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=jshin@chromium.org > Could you separate this change into a separate patch? > > https://codereview.chromium.org/2585903002/diff/20001/test/intl/date-format/d... > File test/intl/date-format/date-format-to-parts.js (left): > > https://codereview.chromium.org/2585903002/diff/20001/test/intl/date-format/d... > test/intl/date-format/date-format-to-parts.js:6: > Could you leave the Flags: line in until the later patch to remove the flag > altogether? This would make a potential revert more minimal. Thanks, Dan. Updated per your comments.
https://codereview.chromium.org/2585903002/diff/20001/test/intl/OWNERS File test/intl/OWNERS (right): https://codereview.chromium.org/2585903002/diff/20001/test/intl/OWNERS#newcode2 test/intl/OWNERS:2: jshin@chromium.org On 2016/12/16 23:40:08, Dan Ehrenberg wrote: > Could you separate this change into a separate patch? Dropped https://codereview.chromium.org/2585903002/diff/20001/test/intl/date-format/d... File test/intl/date-format/date-format-to-parts.js (left): https://codereview.chromium.org/2585903002/diff/20001/test/intl/date-format/d... test/intl/date-format/date-format-to-parts.js:6: On 2016/12/16 23:40:08, Dan Ehrenberg wrote: > Could you leave the Flags: line in until the later patch to remove the flag > altogether? This would make a potential revert more minimal. Done.
lgtm
On 2016/12/19 21:02:52, Dan Ehrenberg wrote: > lgtm Thanks, Dan. BTW, I just got one approval email in response to my 'Intent to ship' (from you). How important is to meet the requirement for three approvals?
On 2016/12/19 21:25:48, jungshik at google wrote: > On 2016/12/19 21:02:52, Dan Ehrenberg wrote: > > lgtm > > Thanks, Dan. BTW, I just got one approval email in response to my 'Intent to > ship' (from you). How important is to meet the requirement for three approvals? +adamk, jochen I don't see an approval requirement in https://github.com/v8/v8/wiki/Feature%20Launch%20Process . I'm not an API owner, so I wouldn't be able to LGTM a Blink intent. Should we have a stricter process, like the rest of Chrome? Should we cc blink-dev on intent threads to get more exposure that might lead to more approvals?
adamk@chromium.org changed reviewers: + adamk@chromium.org
I think this is ready to ship (lgtm fwiw); even if we bcc it to blink-dev (which would be a fine), Blink API owners don't have "authority" over what we expose here. Questions about whether we need more process shouldn't be handled on this CL, and I don't see any reason to hold this to a different standard than any of the other language features we've shipped this year.
On 2016/12/19 22:31:55, adamk wrote: > I think this is ready to ship (lgtm fwiw); even if we bcc it to blink-dev (which > would be a fine), Blink API owners don't have "authority" over what we expose > here. > > Questions about whether we need more process shouldn't be handled on this CL, > and I don't see any reason to hold this to a different standard than any of the > other language features we've shipped this year. Thank you, Adam and Dan. I'm going to land this CL.
The CQ bit was checked by jshin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482258125118380,
"parent_rev": "5354e28c8dbf40239f842bfed5fff72c72e9ddf5", "commit_rev":
"a6749915d3d0167893153526de0ba61b16ab0d26"}
Message was sent while issue was closed.
Description was changed from ========== Ship Intl.DateTimeFormat.formatToParts() Move Intl.DateTimeFormat.formatToParts() to HARMONY_SHIPPING bucket. Spec discussion: https://github.com/tc39/ecma402/issues/30 It's in stage 4 and Firefox shipped it in Firefox 51.0. ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... ) BUG=v8:5244 TEST=intl/date-format/date-format-to-parts.js TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/* ========== to ========== Ship Intl.DateTimeFormat.formatToParts() Move Intl.DateTimeFormat.formatToParts() to HARMONY_SHIPPING bucket. Spec discussion: https://github.com/tc39/ecma402/issues/30 It's in stage 4 and Firefox shipped it in Firefox 51.0. ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... ) BUG=v8:5244 TEST=intl/date-format/date-format-to-parts.js TEST=test262/intl402/DateTimeFormat/prototype/formatToParts/* Review-Url: https://codereview.chromium.org/2585903002 Cr-Commit-Position: refs/heads/master@{#41871} Committed: https://chromium.googlesource.com/v8/v8/+/a6749915d3d0167893153526de0ba61b16a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/a6749915d3d0167893153526de0ba61b16a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
