Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887183003:
Include newly added API method to the framework.order. (Closed)
|
Created:
3 years, 7 months ago by aam Modified:
3 years, 7 months ago CC:
reviews+dom_dartlang.org Target Ref:
refs/heads/releases/2661_work Visibility:
Public. |
DescriptionInclude newly added API method to the framework.order.
This is in preparation for landing https://codereview.chromium.org/2881953002.
BUG=https://github.com/dart-lang/sdk/issues/28264
R=alanknight@google.com, whesse@google.com
Committed: https://src.chromium.org/viewvc/chrome/branches/dart/dartium/src?view=rev&revision=51d367bc20ffec30155fb3d85672dc347444ece3
Patch Set 1 #
Total comments: 2
Patch Set 2 : Maintain some sort of alphabetical order #Patch Set 3 : Maintain some sort of alphabetical order #Messages
Total messages: 22 (6 generated)
aam@google.com changed reviewers: + alanknight@google.com, terry@google.com
PTAL, thanks!
aam@google.com changed reviewers: + whesse@google.com
Now that original cl https://github.com/dart-lang/sdk/commit/d91f228766a2c1708bec8f62bbe2756e4a529f5e was reverted, intent is to submit this first to prevent breakages when original cl gets resubmitted.
LGTM.
Description was changed from ========== Include newly added API method to the framework.order. This is follow-up to https://codereview.chromium.org/2881953002. BUG= ========== to ========== Include newly added API method to the framework.order. This is in preparation for landing https://codereview.chromium.org/2881953002. BUG= ==========
https://codereview.chromium.org/2887183003/diff/1/chrome/app/framework.order File chrome/app/framework.order (right): https://codereview.chromium.org/2887183003/diff/1/chrome/app/framework.order#... chrome/app/framework.order:323: _Dart_CompileSourcesToKernel If we're trying to maintain alphabetical order, this seems off by one. Although there are some at the end that are entirely out of order, so I'm not sure what the convention is.
https://codereview.chromium.org/2887183003/diff/1/chrome/app/framework.order File chrome/app/framework.order (right): https://codereview.chromium.org/2887183003/diff/1/chrome/app/framework.order#... chrome/app/framework.order:323: _Dart_CompileSourcesToKernel On 2017/05/18 at 16:32:13, Alan Knight wrote: > If we're trying to maintain alphabetical order, this seems off by one. Although there are some at the end that are entirely out of order, so I'm not sure what the convention is. Done.
Description was changed from ========== Include newly added API method to the framework.order. This is in preparation for landing https://codereview.chromium.org/2881953002. BUG= ========== to ========== Include newly added API method to the framework.order. This is in preparation for landing https://codereview.chromium.org/2881953002. BUG=https://github.com/dart-lang/sdk/issues/28264 ==========
Terry,
PTAL, as it looks like I need LGTM from you since I'm getting
===
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/app/framework.order
===
and even though you don't seem to be in that OWNER list I see you were able to
submit changes yourself.
lgtm
alanknight@google.com changed reviewers: + jacobr@google.com
Terry's on a plane today. If I'm not in the list, +Jacobr might be.
On 2017/05/18 at 18:13:35, alanknight wrote: > Terry's on a plane today. If I'm not in the list, +Jacobr might be. Thanks Alan. I used rmacnak@ tip to bypass hooks, so was able to get a little bit further, but it fails now with 'update access denied' error. === Attempt 1 of 3 Fetching origin/refs/heads/releases/2661_work... From https://chromium.googlesource.com/dart/dartium/src * [new branch] releases/2661_work -> git-cl-cherry-pick Cherry-picking commit on top of latest refs/heads/releases/2661_work Counting objects: 5, done. Delta compression using up to 12 threads. Compressing objects: 100% (5/5), done. Writing objects: 100% (5/5), 601 bytes | 0 bytes/s, done. Total 5 (delta 4), reused 0 (delta 0) remote: Resolving deltas: 100% (4/4) remote: Counting objects: 5, done remote: Replicating objects... done remote: Processing changes: refs: 1, done error: failed to push some refs to 'https://chromium.googlesource.com/dart/dartium/src.git' To https://chromium.googlesource.com/dart/dartium/src.git ! HEAD:refs/heads/releases/2661_work [remote rejected] (prohibited by Gerrit: ref update access denied) Done ===
I'll see if I can land it.
On 2017/05/18 at 18:29:33, alanknight wrote: > I'll see if I can land it. Thanks!
Description was changed from ========== Include newly added API method to the framework.order. This is in preparation for landing https://codereview.chromium.org/2881953002. BUG=https://github.com/dart-lang/sdk/issues/28264 ========== to ========== Include newly added API method to the framework.order. This is in preparation for landing https://codereview.chromium.org/2881953002. BUG=https://github.com/dart-lang/sdk/issues/28264 R=alanknight@google.com, whesse@google.com Committed: https://src.chromium.org/viewvc/chrome/branches/dart/dartium/src?view=rev&rev... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 51d367bc20ffec30155fb3d85672dc347444ece3.
Message was sent while issue was closed.
DEPS update in https://codereview.chromium.org/2888283003/
Message was sent while issue was closed.
On 2017/05/18 19:41:51, Alan Knight wrote: > DEPS update in https://codereview.chromium.org/2888283003/ Thanks again, Alan! I was going to submit my cl after DEPS are updated so there is no breakage, does it sound right to you?
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Deps change is landed now, and Dartium built successfully on the Mac. |
