[Webkit-unassigned] [Bug 67590] Implement DRT support for Android.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67590





--- Comment #9 from Hao Zheng <zhenghao at chromium.org>  2011-09-06 02:53:06 PST ---
(In reply to comment #8)
> (From update of attachment 106387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106387&action=review
> 
> > Source/WebKit/chromium/WebKit.gyp:1130
> > +                    # FIXME(zhenghao): Reserve for upstream in the near future.
> 
> This wasn't in the previous patch and I don't understand the comment. Can you explain?

See comment #4. It breaks other builds, so exclude the newly added file here in !os(android) part. And the os(android) part is reserved for patches in the near future.

> 
> > Tools/DumpRenderTree/chromium/TestShellAndroid.cpp:74
> > +// TODO: Implement the following metheds if needed.
> 
> WebKit uses FIXME, not TODO. And this is rather vague. If you don't know of a reason why they'd be used, I don't think you should use a FIXME - use 'not required on Android' or an ASSERT_NOT_REACHED(). This can always be changed if we later find a reason why they're needed.

We cannot use ASSERT_NOT_REACHED() there, because we will reach them. All we need to do is to provide an null implementation for now. 'not required' is also not suitable, IMHO. I think no comment there is reasonable, like patch 1.

> 
> > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:180
> > +    alarmAction.sa_flags = SA_RESETHAND;
> 
> Mention this fix in the ChangeLog

Ok.

> 
> > Tools/DumpRenderTree/chromium/TestShellGtk.cpp:-188
> > -    signal(SIGALRM, SIG_DFL);
> 
> Why don't we need to reset the previous handler?

alarmAction.sa_flags = SA_RESETHAND;
This will reset the handler.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.