Removed wchar_t from Time::FromString

Removed wchar_t from Time::FromString.
Also, some of the test case are moved from pr_time_unittests to time_unittests.


BUG=77962
TEST=base_unittests:TimeTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83048

[shinyak (Google), 2011-04-26 00:44:58]

Brettw, could you review it, please?

[brettw, 2011-04-26 15:33:03]

http://codereview.chromium.org/6903022/diff/1/base/time.cc
File base/time.cc (right):

http://codereview.chromium.org/6903022/diff/1/base/time.cc#newcode104
base/time.cc:104: if (time_string[0] == '\0') {
Be consistent about {} usage for single-line conditionals. This means following
the code around you and not adding them here.

http://codereview.chromium.org/6903022/diff/1/base/time.cc#newcode119
base/time.cc:119: #if 0
Why is this here commented out?

http://codereview.chromium.org/6903022/diff/1/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/6903022/diff/1/base/time_unittest.cc#newcode18
base/time_unittest.cc:18: // See also
See also what?

[shinyak (Google), 2011-04-26 16:06:27]

Brettw,

I have fixed the problems you pointed.
Also, the year in the license statement has been changed because it requires to
pass presubmit checks.

Could you review it again?

[brettw, 2011-04-26 16:32:25]

LGTM

[commit-bot: I haz the power, 2011-04-26 18:17:06]

Change committed as 83048

[wtc, 2011-07-08 22:49:15]

http://codereview.chromium.org/6903022/diff/3001/base/pr_time_unittest.cc
File base/pr_time_unittest.cc (left):

http://codereview.chromium.org/6903022/diff/3001/base/pr_time_unittest.cc#old...
base/pr_time_unittest.cc:278: }
It seems that the last three unit tests, which test
PR_ParseTimeString, should stay in this file.

http://codereview.chromium.org/6903022/diff/3001/base/third_party/nspr/prtime.h
File base/third_party/nspr/prtime.h (right):

http://codereview.chromium.org/6903022/diff/3001/base/third_party/nspr/prtime...
base/third_party/nspr/prtime.h:233: NSPR_API(PRStatus) PR_ParseTimeString (
Is this change right?

http://codereview.chromium.org/6903022/diff/3001/base/time.cc
File base/time.cc (right):

http://codereview.chromium.org/6903022/diff/3001/base/time.cc#newcode116
base/time.cc:116: 
Remove this blank line.

[shinyak (Google), 2011-07-13 14:34:46]

This previous patch was reverted in r83055, however it is due to clang bug.

The svn log in r83055 says that 'This causes regression on
KeywordProviderTest.Edit on clang produced executable.', however clang produces
unexecutable binary without my patch. KeywordProviderTest.Edit is disabled now.

I have reflected wtc's comment and upload the revised patch.

http://codereview.chromium.org/6903022/diff/3001/base/pr_time_unittest.cc
File base/pr_time_unittest.cc (left):

http://codereview.chromium.org/6903022/diff/3001/base/pr_time_unittest.cc#old...
base/pr_time_unittest.cc:278: }
On 2011/07/08 22:49:15, wtc wrote:
> It seems that the last three unit tests, which test
> PR_ParseTimeString, should stay in this file.

Done.

http://codereview.chromium.org/6903022/diff/3001/base/third_party/nspr/prtime.h
File base/third_party/nspr/prtime.h (right):

http://codereview.chromium.org/6903022/diff/3001/base/third_party/nspr/prtime...
base/third_party/nspr/prtime.h:233: NSPR_API(PRStatus) PR_ParseTimeString (
On 2011/07/08 22:49:15, wtc wrote:
> Is this change right?

Since PR_ParseTimeString should not be BASE API any more, I have changed this.

http://codereview.chromium.org/6903022/diff/3001/base/time.cc
File base/time.cc (right):

http://codereview.chromium.org/6903022/diff/3001/base/time.cc#newcode116
base/time.cc:116: 
On 2011/07/08 22:49:15, wtc wrote:
> Remove this blank line.

Done.

[shinyak (Google), 2011-07-13 14:39:15]

Oops, I mistook uploading wrong base/third_party/nspr/prtime.h.
Please wait reviewing until patch set 4...

On 2011/07/13 14:34:46, shinyak wrote:
> This previous patch was reverted in r83055, however it is due to clang bug.
> 
> The svn log in r83055 says that 'This causes regression on
> KeywordProviderTest.Edit on clang produced executable.', however clang
produces
> unexecutable binary without my patch. KeywordProviderTest.Edit is disabled
now.
> 
> I have reflected wtc's comment and upload the revised patch.
> 
> http://codereview.chromium.org/6903022/diff/3001/base/pr_time_unittest.cc
> File base/pr_time_unittest.cc (left):
> 
>
http://codereview.chromium.org/6903022/diff/3001/base/pr_time_unittest.cc#old...
> base/pr_time_unittest.cc:278: }
> On 2011/07/08 22:49:15, wtc wrote:
> > It seems that the last three unit tests, which test
> > PR_ParseTimeString, should stay in this file.
> 
> Done.
> 
>
http://codereview.chromium.org/6903022/diff/3001/base/third_party/nspr/prtime.h
> File base/third_party/nspr/prtime.h (right):
> 
>
http://codereview.chromium.org/6903022/diff/3001/base/third_party/nspr/prtime...
> base/third_party/nspr/prtime.h:233: NSPR_API(PRStatus) PR_ParseTimeString (
> On 2011/07/08 22:49:15, wtc wrote:
> > Is this change right?
> 
> Since PR_ParseTimeString should not be BASE API any more, I have changed this.
> 
> http://codereview.chromium.org/6903022/diff/3001/base/time.cc
> File base/time.cc (right):
> 
> http://codereview.chromium.org/6903022/diff/3001/base/time.cc#newcode116
> base/time.cc:116: 
> On 2011/07/08 22:49:15, wtc wrote:
> > Remove this blank line.
> 
> Done.

[wtc, 2011-07-13 17:01:31]

LGTM.  I reviewed Patch Set 4.  Just two nits below.

http://codereview.chromium.org/6903022/diff/8010/base/third_party/nspr/prtime.h
File base/third_party/nspr/prtime.h (left):

http://codereview.chromium.org/6903022/diff/8010/base/third_party/nspr/prtime...
base/third_party/nspr/prtime.h:232: 
Nit: add this blank line back.

This file is third-party code.  So we want to minimize
unnecessary modifications.

http://codereview.chromium.org/6903022/diff/8010/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/6903022/diff/8010/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:436:
logo_end_string.c_str(), &end_time)) {
The arguments to the four base::Time::FromString calls in
this file may fit on the same line now.  Please give that a
try.

[shinyak (Google), 2011-07-13 23:28:33]

http://codereview.chromium.org/6903022/diff/8010/base/third_party/nspr/prtime.h
File base/third_party/nspr/prtime.h (left):

http://codereview.chromium.org/6903022/diff/8010/base/third_party/nspr/prtime...
base/third_party/nspr/prtime.h:232: 
On 2011/07/13 17:01:31, wtc wrote:
> Nit: add this blank line back.
> 
> This file is third-party code.  So we want to minimize
> unnecessary modifications.

Done.

http://codereview.chromium.org/6903022/diff/8010/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/6903022/diff/8010/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:436:
logo_end_string.c_str(), &end_time)) {
On 2011/07/13 17:01:31, wtc wrote:
> The arguments to the four base::Time::FromString calls in
> this file may fit on the same line now.  Please give that a
> try.

Done.

[shinyak (Google), 2011-07-22 03:58:20]

Could you give LGTM again if OK?
I don't have commit right without a commit queue.

On 2011/07/13 23:28:33, shinyak wrote:
>
http://codereview.chromium.org/6903022/diff/8010/base/third_party/nspr/prtime.h
> File base/third_party/nspr/prtime.h (left):
> 
>
http://codereview.chromium.org/6903022/diff/8010/base/third_party/nspr/prtime...
> base/third_party/nspr/prtime.h:232: 
> On 2011/07/13 17:01:31, wtc wrote:
> > Nit: add this blank line back.
> > 
> > This file is third-party code.  So we want to minimize
> > unnecessary modifications.
> 
> Done.
> 
>
http://codereview.chromium.org/6903022/diff/8010/chrome/browser/web_resource/...
> File chrome/browser/web_resource/promo_resource_service.cc (right):
> 
>
http://codereview.chromium.org/6903022/diff/8010/chrome/browser/web_resource/...
> chrome/browser/web_resource/promo_resource_service.cc:436:
> logo_end_string.c_str(), &end_time)) {
> On 2011/07/13 17:01:31, wtc wrote:
> > The arguments to the four base::Time::FromString calls in
> > this file may fit on the same line now.  Please give that a
> > try.
> 
> Done.

[brettw, 2011-07-22 15:44:24]

LGTM

[shinyak (Google), 2011-07-23 05:22:45]

I changed this patch so that the trunk is buildable.
Also, I need another LGTM from webkit/blob and webkit/fileapi owner to commit
this patch.

Kinuko-san, could you check this patch?

[kinuko, 2011-07-23 08:33:52]

LGTM for webkit/{blob,fileapi}

On 2011/07/23 05:22:45, shinyak wrote:
> I changed this patch so that the trunk is buildable.
> Also, I need another LGTM from webkit/blob and webkit/fileapi owner to commit
> this patch.
> 
> Kinuko-san, could you check this patch?

[shinyak (Google), 2011-07-23 08:49:23]

Pawel,

I need net/ftp OWNER's LGTM to commit this patch.
Only it seems that you are net/ftp's OWNER.

Could you review this patch?

[shinyak (Google), 2011-07-23 08:50:16]

Oops...

Only it seems that you are net/ftp's OWNER.
->
It seems that only you are net/ftp's OWNER.


On 2011/07/23 08:49:23, shinyak wrote:
> Pawel,
> 
> I need net/ftp OWNER's LGTM to commit this patch.
> Only it seems that you are net/ftp's OWNER.
> 
> Could you review this patch?

[wtc, 2011-07-25 17:25:10]

LGTM.

shinyak: I am an owner of the src/net module.  I reviewed
the entire CL.

http://codereview.chromium.org/6903022/diff/23001/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/6903022/diff/23001/base/time_unittest.cc#newco...
base/time_unittest.cc:50: 
Nit: delete this extra blank line.

[shinyak (Google), 2011-07-26 01:56:34]

I believe I have LGTM for all CL, however I cannot use commit queue. Commit
checkbox is disabled yet.
Does any one know the reason? Do I have to get more LGTM?

brettw for base/*
 base/file_util_unittest.cc
 base/metrics/field_trial.cc
 base/pr_time_unittest.cc
 base/third_party/nspr/prtime.h
 base/time.h
 base/time.cc
 base/time_unittest.cc

no OWNERS in chrome
 chrome/browser/web_resource/promo_resource_service.cc
 chrome/common/metrics_helpers.cc

wtc for net/*
 net/ftp/ftp_util_unittest.cc
 net/http/http_response_headers.cc
 net/http/http_response_headers_unittest.cc

kinuko for webkit/blob and webkit/fileapi
 webkit/blob/blob_storage_controller_unittest.cc
 webkit/fileapi/file_system_dir_url_request_job_unittest.cc

[wtc, 2011-07-26 17:52:02]

shinyak: I think the problem may not be with lacking OWNERS' approval.
The problem may be that this code review is already in the "Closed"
status.

You already committed a change (r83048) from this code review.
I guess you did that using the commit queue.  In that case, you
need to do "gcl delete <changelist>" in your Chromium source tree
to manually delete the changelist.  Then, do "gcl change <changelist>"
to create a new changelist for the follow-up changes.

[shinyak (Google), 2011-07-27 07:15:07]

wtc,

Thanks!
I've moved this issue to http://codereview.chromium.org/7470038/

On 2011/07/26 17:52:02, wtc wrote:
> shinyak: I think the problem may not be with lacking OWNERS' approval.
> The problem may be that this code review is already in the "Closed"
> status.
> 
> You already committed a change (r83048) from this code review.
> I guess you did that using the commit queue.  In that case, you
> need to do "gcl delete <changelist>" in your Chromium source tree
> to manually delete the changelist.  Then, do "gcl change <changelist>"
> to create a new changelist for the follow-up changes.