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

Issue 180243015: Base: Introduce a new version of FileUtilProxy that works with File. (Closed)

Created:
6 years, 10 months ago by rvargas (doing something else)
Modified:
6 years, 8 months ago
Reviewers:
kinuko, awong
CC:
chromium-reviews, erikwright+watch_chromium.org
Visibility:
Public.

Description

Base: Introduce a new version of FileUtilProxy that works with File. BUG=322664 TEST=net_unittests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257290

Patch Set 1 : #

Total comments: 10

Patch Set 2 : Add comment #

Patch Set 3 : Add DCHECKs #

Total comments: 4

Patch Set 4 : Review comments #

Patch Set 5 : Rebase #

Patch Set 6 : Changed FileInfo test #

Patch Set 7 : Add some times to GetInfo test #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+391 lines, -591 lines) Patch
M base/base.gyp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
A + base/files/file_proxy.h View 1 2 3 2 chunks +82 lines, -136 lines 0 comments Download
A + base/files/file_proxy.cc View 1 2 3 4 4 chunks +205 lines, -291 lines 0 comments Download
A + base/files/file_proxy_unittest.cc View 1 2 3 4 5 6 10 chunks +100 lines, -164 lines 1 comment Download

Messages

Total messages: 23 (0 generated)
rvargas (doing something else)
PTAL
6 years, 9 months ago (2014-02-28 22:44:56 UTC) #1
kinuko
GetFileInfo_File seems broken on linux_chromeos? https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc#newcode217 base/files/file_proxy.cc:217: FileProxy::~FileProxy() { This may ...
6 years, 9 months ago (2014-03-03 05:58:58 UTC) #2
rvargas (doing something else)
Thanks https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc#newcode217 base/files/file_proxy.cc:217: FileProxy::~FileProxy() { On 2014/03/03 05:58:58, kinuko wrote: > ...
6 years, 9 months ago (2014-03-03 20:38:34 UTC) #3
kinuko
lgtm https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc#newcode217 base/files/file_proxy.cc:217: FileProxy::~FileProxy() { Ok... not performing Close automatically in ...
6 years, 9 months ago (2014-03-04 04:07:45 UTC) #4
rvargas (doing something else)
Thanks! +ajwong for owners https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc#newcode217 base/files/file_proxy.cc:217: FileProxy::~FileProxy() { On 2014/03/04 04:07:45, ...
6 years, 9 months ago (2014-03-04 23:53:53 UTC) #5
rvargas (doing something else)
ping (awong)
6 years, 9 months ago (2014-03-07 19:49:51 UTC) #6
rvargas (doing something else)
Brett, do you mind taking a look for owners?
6 years, 9 months ago (2014-03-11 00:37:44 UTC) #7
awong
Finally got around to this. LGTM and removed Brett from owners file. Sorry for slow ...
6 years, 9 months ago (2014-03-11 01:39:44 UTC) #8
rvargas (doing something else)
Thanks!
6 years, 9 months ago (2014-03-11 01:45:35 UTC) #9
rvargas (doing something else)
The CQ bit was checked by rvargas@chromium.org
6 years, 9 months ago (2014-03-11 01:45:39 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rvargas@chromium.org/180243015/240001
6 years, 9 months ago (2014-03-11 01:48:53 UTC) #11
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-11 01:48:58 UTC) #12
commit-bot: I haz the power
Failed to apply patch for base/files/file_proxy.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; A base/files/file_proxy.cc ...
6 years, 9 months ago (2014-03-11 01:48:59 UTC) #13
rvargas (doing something else)
Kinuko: I modified the unit test for GetInfo because one of the bots was failing ...
6 years, 9 months ago (2014-03-12 01:17:17 UTC) #14
kinuko
On 2014/03/12 01:17:17, rvargas wrote: > Kinuko: I modified the unit test for GetInfo because ...
6 years, 9 months ago (2014-03-12 04:16:12 UTC) #15
rvargas (doing something else)
On 2014/03/12 04:16:12, kinuko wrote: > On 2014/03/12 01:17:17, rvargas wrote: > > Kinuko: I ...
6 years, 9 months ago (2014-03-12 18:43:11 UTC) #16
rvargas (doing something else)
Kinuko: I'm assuming that you're OK with the latest version. If that was not the ...
6 years, 9 months ago (2014-03-14 23:14:22 UTC) #17
rvargas (doing something else)
The CQ bit was checked by rvargas@chromium.org
6 years, 9 months ago (2014-03-14 23:14:27 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rvargas@chromium.org/180243015/370001
6 years, 9 months ago (2014-03-14 23:15:33 UTC) #19
commit-bot: I haz the power
Change committed as 257290
6 years, 9 months ago (2014-03-15 05:19:35 UTC) #20
kinuko
https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_unittest.cc File base/files/file_proxy_unittest.cc (right): https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_unittest.cc#newcode214 base/files/file_proxy_unittest.cc:214: EXPECT_TRUE(file.GetInfo(&expected_info)); Sorry I hadn't responded quickly... I'm ok with ...
6 years, 9 months ago (2014-03-17 03:50:27 UTC) #21
rvargas (doing something else)
On 2014/03/17 03:50:27, kinuko wrote: > https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_unittest.cc > File base/files/file_proxy_unittest.cc (right): > > https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_unittest.cc#newcode214 > ...
6 years, 8 months ago (2014-04-07 14:58:20 UTC) #22
kinuko
6 years, 8 months ago (2014-04-07 15:00:57 UTC) #23
Message was sent while issue was closed.
On 2014/04/07 14:58:20, rvargas wrote:
> On 2014/03/17 03:50:27, kinuko wrote:
> >
>
https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u...
> > File base/files/file_proxy_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u...
> > base/files/file_proxy_unittest.cc:214:
> > EXPECT_TRUE(file.GetInfo(&expected_info));
> > Sorry I hadn't responded quickly... I'm ok with this change, but this line'd
> > likely confuse readers in a future.  Would it be possible to make a small
> > follow-up change and add a comment why we do here, along with the bug number
> > you're opening for the issue that we use different impl (which leads to have
> > different precision)?  Thanks.
> 
> Sorry for taking so long... I was hoping the fix would take just a couple of
> days. Anyway, File::GetInfo and file_util::GetFileInfo are unified as of
> r261956.

Cooool! Thanks.

Powered by Google App Engine
This is Rietveld 408576698