[WIN] Replace CF time functions with Windows API functions in WebHistory
Created attachment 212047 [details] Patch
Comment on attachment 212047 [details] Patch Attachment 212047 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1955082
Comment on attachment 212047 [details] Patch Attachment 212047 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1955083
Comment on attachment 212047 [details] Patch Attachment 212047 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1961107
Comment on attachment 212047 [details] Patch Attachment 212047 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2006003
Created attachment 212049 [details] Patch
Created attachment 212154 [details] Patch
Comment on attachment 212154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212154&action=review The general intent of this change is fine, but I would prefer not to lose the two methods (ageLimitDate and timeToDate). The other client I was concerned about uses the WebKIt COM API, so switching to DATE over CFAbsoluteTime should be fine. r- to avoid removing the two methods, but otherwise this is fine. > Source/WebKit/win/WebHistory.cpp:645 > + } It would be nice if the above code was contained in a static function that consumed the 'day' argument and returned the 'beginningOfNextDayLocalTime' and 'beginningLocalTime' values. > Source/WebKit/win/WebHistory.cpp:650 > + VariantTimeToSystemTime(date, &beginningOfNextDayLocalTime); ... including this fix-up code. > Source/WebKit/win/WebHistory.cpp:-722 > -CFAbsoluteTime WebHistory::timeToDate(CFAbsoluteTime time) This is needed by ageLimitDate. > Source/WebKit/win/WebHistory.cpp:-738 > -HRESULT WebHistory::ageLimitDate(CFAbsoluteTime* time) I don't think we should get rid of this. I was thinking of making use of it for the WinLauncher program. But it would be okay to use a DATE argument if you wanted. > Source/WebKit/win/WebHistory.h:-142 > - HRESULT ageLimitDate(CFAbsoluteTime* time); We need to keep this. > Source/WebKit/win/WebHistory.h:-144 > - static CFAbsoluteTime timeToDate(CFAbsoluteTime time); Don't remove this, as it is needed by ageLimitDate.
Comment on attachment 212154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212154&action=review >> Source/WebKit/win/WebHistory.cpp:645 >> + } > > It would be nice if the above code was contained in a static function that consumed the 'day' argument and returned the 'beginningOfNextDayLocalTime' and 'beginningLocalTime' values. sure we want to call VariantTimeToSystemTime(day, &systemTime) then twice? Or pass beginningLocalTime to beginningOfNext() and return it as local time (including the fix-up) >> Source/WebKit/win/WebHistory.h:-142 >> - HRESULT ageLimitDate(CFAbsoluteTime* time); > > We need to keep this. who uses it? it's a private method...
Comment on attachment 212154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212154&action=review >> Source/WebKit/win/WebHistory.cpp:-738 >> -HRESULT WebHistory::ageLimitDate(CFAbsoluteTime* time) > > I don't think we should get rid of this. I was thinking of making use of it for the WinLauncher program. But it would be okay to use a DATE argument if you wanted. there is no CF-stuff in the public API, so it can't be used as it is IMO. why keep then a unused method instead of getting it back from svn history when needed?
Comment on attachment 212154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212154&action=review >>> Source/WebKit/win/WebHistory.cpp:645 >>> + } >> >> It would be nice if the above code was contained in a static function that consumed the 'day' argument and returned the 'beginningOfNextDayLocalTime' and 'beginningLocalTime' values. > > sure we want to call VariantTimeToSystemTime(day, &systemTime) then twice? Or pass beginningLocalTime to beginningOfNext() and return it as local time (including the fix-up) Why do we need to call VariantTimeToSystemTime twice? Can't we just return "beginningOfLocalTime" along with "beginningOfNextDayLocalTime" and then use them for the rest of the logic? >>> Source/WebKit/win/WebHistory.cpp:-738 >>> -HRESULT WebHistory::ageLimitDate(CFAbsoluteTime* time) >> >> I don't think we should get rid of this. I was thinking of making use of it for the WinLauncher program. But it would be okay to use a DATE argument if you wanted. > > there is no CF-stuff in the public API, so it can't be used as it is IMO. why keep then a unused method instead of getting it back from svn history when needed? Okay -- you've convinced me. Ditch it and the "timeToDate" method. >>> Source/WebKit/win/WebHistory.h:-142 >>> - HRESULT ageLimitDate(CFAbsoluteTime* time); >> >> We need to keep this. > > who uses it? it's a private method... Okay >> Source/WebKit/win/WebHistory.h:-144 >> - static CFAbsoluteTime timeToDate(CFAbsoluteTime time); > > Don't remove this, as it is needed by ageLimitDate. Disregard this comment.
Created attachment 212439 [details] Patch
Comment on attachment 212439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212439&action=review Looks good to me, with some minor nits. > Source/WebKit/win/WebHistory.cpp:100 > + VariantTimeToSystemTime(day, &systemTime); We have been prefixing these Windows API calls with :: (e.g., "::VariantTimeToSystemTime(...)" and "::SystemTimeToTzSpecificLocalTime(...)").
Created attachment 212615 [details] Patch
Comment on attachment 212615 [details] Patch Clearing flags on attachment: 212615 Committed r156430: <http://trac.webkit.org/changeset/156430>
All reviewed patches have been landed. Closing bug.