On Tue, Oct 27, 2009 at 8:59 PM, Dmitry Titov <[email protected]> wrote:
> I see. I was thinking if the change to SharedTimer API to pass offset > rather then absolute time would be useful... But now I realize it is more > useful and even necessary to have a single time source rather then assume > that there will be multiple not synchronized timers. > > On a separate note, the fact that Windows versions of currentTime() is such > a rocket science is just sad. > Indeed!!! By the way, I suspect this bug was a GREAT source of flakiness in our tests. I think it at least explains why the ErrorPage UI tests are so flaky. Pawel and I were observing what looked like a WebCore::Timer with a delay of 0 running _after_ a PostTask when it should have been the other way around. I'm very hopeful that we will see a significant reduction in flaky tests as a result of this fix :-) -Darin > > > On Tue, Oct 27, 2009 at 7:34 PM, Darin Fisher <[email protected]> wrote: > >> A central clock is important for the reasons James describes, but using >> the one from WebKit is not a good option since that would make other modules >> in Chromium, like base/ and net/, have a dependency on WebKit. Also, given >> that we started out using our own central clock, until WebKit grew its own, >> it made the most sense to me to add a currentTime function to WebKitClient. >> >> More history: This is the N'th time we have fixed this bug. The last >> time, ChromiumCurrentTime.cpp was introduced, and we configured the build >> system to exclude wtf/CurrentTime.cpp. However, when we switched over to >> using JavaScriptCore.gypi, wtf/CurrentTime.cpp slipped back into the build. >> When linking Chrome, we were then linking against two implementations of >> currentTime! However, because they existed in separate .lib files, the >> linker didn't complain about duplicate symbols. It just picked one of them >> to use, and we never noticed. >> >> James' patch adds a #error if wtf/CurrentTime.cpp is compiled in the >> Chromium build, so we should never repeat this mistake again. >> >> As for which implementation of currentTime is better, I can't say for >> sure. I just know that we put a lot of time into our implementation, and it >> has served us quite well. >> >> -Darin >> >> >> >> On Tue, Oct 27, 2009 at 7:13 PM, Dmitry Titov <[email protected]>wrote: >> >>> Very cool investigation! >>> >>> I see WebKitClient::currentTime() in the upcoming WebKit API so I guess >>> the idea is that client of WebKit would provide "the central clock". I >>> agree "central clock" is a good idea. Currently in Webkit, it's provided by >>> WTF. Embedders (like Safari) use WTF directly, for which WTF stuff is >>> exposed by JavaScriptCore.exp. >>> >>> Do we want to provide central clock from Chromium because we think the >>> Chromium's implementation of currentTime is better? If so, wouldn't it be >>> simpler to just replace the WTF one? ChromiumCurrentTime.cpp looks like a >>> temporary hack... >>> >>> Dmitry >>> >>> >>> On Tue, Oct 27, 2009 at 6:48 PM, Peter Kasting <[email protected]>wrote: >>> >>>> On Tue, Oct 27, 2009 at 4:40 PM, James Robinson <[email protected]>wrote: >>>> >>>>> So what's the problem? WTF::currentTime() is implemented twice, once >>>>> in JavaScriptCore/wtf/CurrentTime.cpp and once in >>>>> webkit/api/src/ChromiumCurrentTime.cpp. The latter simply defers to >>>>> base::Time::Now().ToDoubleT(), the former uses a somewhat different >>>>> algorithm to try to normalize what the system returns. ThreadTimers.cpp >>>>> was >>>>> linking against the JavaScriptCore/wtf/CurrentTime.cpp version and >>>>> webkitclient_impl.cc was linking against the >>>>> webkit/api/src/ChromiumCurrentTime.cpp version. >>>>> >>>> >>>> Arrrgghhhhhh >>>> >>>> Is there a way we can compile-time detect similar symbol names like >>>> this and throw fits (with a whitelist for any we know are OK)? Seems like >>>> this is an issue for something with linking that maruel might have looked >>>> at >>>> long ago. >>>> >>>> PKJ >>>> >>>> >>>> >>>> >>> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
