Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(285)

Issue 124143002: Include icu.isolate instead of listing icudt.dll (Closed)

Created:
6 years, 11 months ago by jungshik at Google
Modified:
6 years, 11 months ago
Reviewers:
Mark Mentovai, M-A Ruel
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, erikwright+watch_chromium.org, jam
Visibility:
Public.

Description

Include icu.isolate instead of listing icudt.dll icu.isolate file added in https://codereview.chromium.org/99073008/ is used where icudt.dll is directly listed for 'isolate_dependency_tracked'. icu.isolate includes either icudtl.dat or icudt.dll depending on 'OS' and 'icu_use_data_file_flag'. Besides, 'icu_use_data_file_flag' is added to build/isolate.gypi as a config-variable. Once the above icu-side CL is checked in, this CL will also includes icu DEPS roll. This is a prerequisite for the following CLs to use icudtl.dat : Windows : https://codereview.chromium.org/99473012 Linux : https://codereview.chromium.org/102413007 Mac : https://codereview.chromium.org/109013004 BUG=72633 TEST=All tests utilizing isolate pass on bots. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245136

Patch Set 1 #

Total comments: 4

Patch Set 2 : roll icu to 243376 #

Patch Set 3 : rebased to ToT #

Patch Set 4 : fix gtest setup.py on android #

Total comments: 4

Patch Set 5 : add comments per maruel #

Patch Set 6 : use '0' (quoted) instead of 0 (bare) in setup.py #

Total comments: 3

Patch Set 7 : use # for comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -6 lines) Patch
M base/base_unittests.isolate View 1 chunk +3 lines, -1 line 0 comments Download
M build/android/pylib/gtest/setup.py View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M build/isolate.gypi View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/chrome.isolate View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/interactive_ui_tests.isolate View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/unit_tests.isolate View 2 chunks +3 lines, -1 line 0 comments Download
M content/content_unittests.isolate View 1 chunk +3 lines, -1 line 0 comments Download
M net/net_unittests.isolate View 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 20 (0 generated)
jungshik at Google
Can you take a look? Thanks. https://codereview.chromium.org/124143002/diff/1/chrome/chrome.isolate File chrome/chrome.isolate (right): https://codereview.chromium.org/124143002/diff/1/chrome/chrome.isolate#newcode82 chrome/chrome.isolate:82: '../third_party/icu/icu.isolate', icu.isolate has ...
6 years, 11 months ago (2014-01-03 19:56:06 UTC) #1
Mark Mentovai
LGTM, but I also defer to Marc-Antoine for isolate particulars again. https://codereview.chromium.org/124143002/diff/1/chrome/chrome.isolate File chrome/chrome.isolate (right): ...
6 years, 11 months ago (2014-01-03 20:17:49 UTC) #2
jungshik at Google
Thank you, Mark. @maruel, can you take a look? thanks.
6 years, 11 months ago (2014-01-06 18:00:35 UTC) #3
M-A Ruel
Sorry for the delays, I was still OOO. lgtm with one nit. https://codereview.chromium.org/124143002/diff/1/net/net_unittests.isolate File net/net_unittests.isolate ...
6 years, 11 months ago (2014-01-07 13:56:46 UTC) #4
jungshik at Google
Thank you, maruel. I'm afraid I asked you to review prematurely. I'm looking into fixing ...
6 years, 11 months ago (2014-01-07 18:55:53 UTC) #5
M-A Ruel
On 2014/01/07 18:55:53, Jungshik Shin wrote: > Thank you, maruel. > I'm afraid I asked ...
6 years, 11 months ago (2014-01-07 19:10:23 UTC) #6
jungshik at Google
On 2014/01/07 19:10:23, M-A Ruel wrote: > On 2014/01/07 18:55:53, Jungshik Shin wrote: > > ...
6 years, 11 months ago (2014-01-07 19:31:39 UTC) #7
jungshik at Google
I filed https://code.google.com/p/chromium/issues/detail?id=332594 for an isolate.py issue.
6 years, 11 months ago (2014-01-09 00:55:56 UTC) #8
jungshik at Google
On 2014/01/09 00:55:56, Jungshik Shin wrote: > I filed https://code.google.com/p/chromium/issues/detail?id=332594 for an > isolate.py issue. ...
6 years, 11 months ago (2014-01-13 19:35:28 UTC) #9
jungshik at Google
On 2014/01/13 19:35:28, Jungshik Shin wrote: > On 2014/01/09 00:55:56, Jungshik Shin wrote: > > ...
6 years, 11 months ago (2014-01-15 00:15:38 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/124143002/240001
6 years, 11 months ago (2014-01-15 01:34:32 UTC) #11
jungshik at Google
On 2014/01/15 01:34:32, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
6 years, 11 months ago (2014-01-15 19:26:58 UTC) #12
M-A Ruel
lgtm with nit. Thanks for fixing. https://codereview.chromium.org/124143002/diff/630002/build/android/pylib/gtest/setup.py File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/124143002/diff/630002/build/android/pylib/gtest/setup.py#newcode111 build/android/pylib/gtest/setup.py:111: isolate_cmd = [ ...
6 years, 11 months ago (2014-01-15 20:40:46 UTC) #13
jungshik at Google
Thanks for the review. I came across another problem, which I think is trivial to ...
6 years, 11 months ago (2014-01-15 23:13:09 UTC) #14
jungshik at Google
https://codereview.chromium.org/124143002/diff/630002/build/android/pylib/gtest/setup.py File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/124143002/diff/630002/build/android/pylib/gtest/setup.py#newcode111 build/android/pylib/gtest/setup.py:111: isolate_cmd = [ On 2014/01/15 20:40:47, M-A Ruel wrote: ...
6 years, 11 months ago (2014-01-15 23:14:14 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/124143002/720001
6 years, 11 months ago (2014-01-15 23:20:49 UTC) #16
M-A Ruel
lgtm https://codereview.chromium.org/124143002/diff/720001/build/android/pylib/gtest/setup.py File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/124143002/diff/720001/build/android/pylib/gtest/setup.py#newcode111 build/android/pylib/gtest/setup.py:111: // This needs to be kept in sync ...
6 years, 11 months ago (2014-01-15 23:21:19 UTC) #17
jungshik at Google
On 2014/01/15 23:21:19, M-A Ruel wrote: > lgtm > > https://codereview.chromium.org/124143002/diff/720001/build/android/pylib/gtest/setup.py > File build/android/pylib/gtest/setup.py (right): ...
6 years, 11 months ago (2014-01-15 23:40:42 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/124143002/970001
6 years, 11 months ago (2014-01-16 01:26:19 UTC) #19
commit-bot: I haz the power
6 years, 11 months ago (2014-01-16 05:22:00 UTC) #20
Message was sent while issue was closed.
Change committed as 245136

Powered by Google App Engine
This is Rietveld 408576698