Bug 66577 - [chromium] touch tests failing on OSX10.5 after r93358
Summary: [chromium] touch tests failing on OSX10.5 after r93358
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eugene Girard
URL:
Keywords:
Depends on: 66492
Blocks: 70351
  Show dependency treegraph
 
Reported: 2011-08-19 12:42 PDT by Tony Chang
Modified: 2011-10-18 21:31 PDT (History)
5 users (show)

See Also:


Attachments
patch v1 (4.19 KB, patch)
2011-10-10 07:59 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2, fix uptime calculation (4.40 KB, patch)
2011-10-11 00:36 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 with review comments fix (4.41 KB, patch)
2011-10-18 02:34 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Robert Kroeger 2011-08-24 12:08:53 PDT
I can't yet replicate this bug locally. However, https://bugs.webkit.org/show_bug.cgi?id=66492 reverts the most likely cause introduced in r93358.
Comment 2 Johnny(Jianning) Ding 2011-10-10 07:57:35 PDT
after r93358, eventSender.touchEnd  in touch tests will trigger scrollEnd gesture, which calls ScrollAnimatorChromiumMac::snapRubberBand. 

However the NSProcessInfo::systemUptime used by  snapRubberBand is not supported by Mac Leopard, which caused error "NSProcessInfo systemUptime unrecognized selector sent to instance”, then DRT was stuck in NSApplication message loop and caused timeout.

Sent patch to fix it.
Comment 3 Johnny(Jianning) Ding 2011-10-10 07:59:56 PDT
Created attachment 110360 [details]
patch v1
Comment 4 Johnny(Jianning) Ding 2011-10-11 00:36:11 PDT
Created attachment 110483 [details]
patch v2, fix uptime calculation
Comment 5 Tony Chang 2011-10-17 09:53:44 PDT
I don't know enough about Macs/objc to review this.  cc'ing some Mac people.
Comment 6 Avi Drissman 2011-10-17 10:53:00 PDT
Comment on attachment 110483 [details]
patch v2, fix uptime calculation

View in context: https://bugs.webkit.org/attachment.cgi?id=110483&action=review

Looks reasonable. LGTM with fixes, for all my decision is worth here at WK.

> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm:59
> +    // Print how long system has been up. Found by looking getting "boottime" from the kernel.

Print?

> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm:71
> +        NSTimeInterval ressult = uptime.tv_sec + (uptime.tv_usec / 1E+6);

s/ressult/result/ ?
Comment 7 Johnny(Jianning) Ding 2011-10-18 02:34:09 PDT
Created attachment 111412 [details]
patch v2 with review comments fix

(In reply to comment #6)
Thanks for review!
> Print?
done
> s/ressult/result/ ?
done
Comment 8 Avi Drissman 2011-10-18 05:14:16 PDT
For as much as it's worth, SLGTM.
Comment 9 Johnny(Jianning) Ding 2011-10-18 06:38:33 PDT
(In reply to comment #8)
> For as much as it's worth, SLGTM.

Can you or someone grant a r+? :)
Comment 10 Avi Drissman 2011-10-18 06:57:28 PDT
Well, you explicitly asked me for a review, so I gave you one. I'm not a webkit reviewer, though, so I can't r+ you. (That's what I meant by my comments "for as much as it's worth" and "for all my decision is worth".)
Comment 11 WebKit Review Bot 2011-10-18 21:31:38 PDT
Comment on attachment 111412 [details]
patch v2 with review comments fix

Clearing flags on attachment: 111412

Committed r97831: <http://trac.webkit.org/changeset/97831>
Comment 12 WebKit Review Bot 2011-10-18 21:31:43 PDT
All reviewed patches have been landed.  Closing bug.