[webkit-reviews] review denied: [Bug 121599] [WIN] Replace CF time functions with Windows API functions in WebHistory : [Attachment 212154] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 121599: [WIN] Replace CF time functions with Windows API functions in
WebHistory
https://bugs.webkit.org/show_bug.cgi?id=121599

Attachment 212154: Patch
https://bugs.webkit.org/attachment.cgi?id=212154&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
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.