|
|
Created:
7 years, 4 months ago by apiccion Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded method to convert base::Time to a Java timestamp.
BUG=247382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224744
Patch Set 1 #Patch Set 2 : Added helper method to time #
Total comments: 6
Patch Set 3 : ' #Messages
Total messages: 20 (0 generated)
lgtm
Can you rename the title to make it clear what your change does? It's not clear if "ForeignSessionHelper returns POSIX timestamps" describes the current bad behavior or the new super awesome behavior. I'd say something like "Make ForeignSessionHelper return POSIX timestamps". A question and a comment: 1. why is this change needed? B. if we really need POSIX timestamps, we should probably add a function WindowsToPosixTime() in time.h or time_util.h. That seems far less error prone than trying to remember whether to add or subtract kWindowsEpochDeltaMicroseconds all over the codebase.
can you change the subject too? android bits lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/22791013/5001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Mark or Darin could you have a look please?
https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc File base/time/time.cc (right): https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc#newcode141 base/time/time.cc:141: int64 Time::ToJavaTime() const { It looks like, given this and ToJsTime’s equivalence, you could use a templated friend or private member function to do the internal work, and have ToJsTime and ToJavaTime call that internal helper. I won’t insist on this as long as there are only two, but I would insist if there were three. https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc#newcode143 base/time/time.cc:143: return 0; You seem to have copied this from ToJsTime, but you lost the comment that came with this return statement. https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc#newcode147 base/time/time.cc:147: return std::numeric_limits<long>::max(); long is the wrong type here.
https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc File base/time/time.cc (right): https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc#newcode141 base/time/time.cc:141: int64 Time::ToJavaTime() const { Thank you! On 2013/09/20 15:46:25, Mark Mentovai wrote: > It looks like, given this and ToJsTime’s equivalence, you could use a templated > friend or private member function to do the internal work, and have ToJsTime and > ToJavaTime call that internal helper. > > I won’t insist on this as long as there are only two, but I would insist if > there were three. https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc#newcode143 base/time/time.cc:143: return 0; On 2013/09/20 15:46:25, Mark Mentovai wrote: > You seem to have copied this from ToJsTime, but you lost the comment that came > with this return statement. Done. https://codereview.chromium.org/22791013/diff/5001/base/time/time.cc#newcode147 base/time/time.cc:147: return std::numeric_limits<long>::max(); On 2013/09/20 15:46:25, Mark Mentovai wrote: > long is the wrong type here. Done.
You seem to have lost chrome/browser/android/foreign_session_helper.cc in patch set 3. Intentional?
Thanks for catching this. Moved those changes to: https://codereview.chromium.org/23514039/ Should be good. Renamed subject/description.
LGTM for base OWNERS approval.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/22791013/21001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/22791013/21001
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/22791013/21001
Message was sent while issue was closed.
Change committed as 224744 |