Bug 121599 – [WIN] Replace CF time functions with Windows API functions in WebHistory

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Patrick R. Gansterer]

[WIN] Replace CF time functions with Windows API functions in WebHistory

[Patrick R. Gansterer]

Created attachment 212047 [details]
Patch

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 212047 [details]
Patch

Attachment 212047 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1955083

[Early Warning System Bot]

Comment on attachment 212047 [details]
Patch

Attachment 212047 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1961107

[Early Warning System Bot]

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

[Patrick R. Gansterer]

Created attachment 212049 [details]
Patch

[Patrick R. Gansterer]

Created attachment 212154 [details]
Patch

[Brent Fulgham]

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.

[Patrick R. Gansterer]

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...

[Patrick R. Gansterer]

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?

[Brent Fulgham]

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.

[Patrick R. Gansterer]

Created attachment 212439 [details]
Patch

[Brent Fulgham]

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(...)").

[Patrick R. Gansterer]

Created attachment 212615 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 212615 [details]
Patch

Clearing flags on attachment: 212615

Committed r156430: <http://trac.webkit.org/changeset/156430>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.