Bug 39440 - [chromium] Adds Geolocation support to DumpRenderTree.
Summary: [chromium] Adds Geolocation support to DumpRenderTree.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-20 12:01 PDT by Marcus Bulach
Modified: 2010-05-23 02:07 PDT (History)
2 users (show)

See Also:

Attachments
Patch (11.56 KB, patch)
2010-05-20 12:12 PDT, Marcus Bulach 		no flags Details | Formatted Diff | Diff
Patch (14.40 KB, patch)
2010-05-21 03:39 PDT, Marcus Bulach 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Marcus Bulach 2010-05-20 12:01:50 PDT 
[chromium] Adds Geolocation support to DumpRenderTree.
Comment 1 Marcus Bulach 2010-05-20 12:12:56 PDT 
Created attachment 56620 [details]
Patch
Comment 2 Kent Tamura 2010-05-20 20:45:11 PDT 
Comment on attachment 56620 [details]
Patch

Thank you for making the patch.
WebKitTools/DumpRenderTree/chromium is not the primary tester yet.  You need to change webkit/tools/test_shell/ in Chromium too.


WebKit/chromium/public/WebGeolocationServiceMock.h:55
 +      static bool m_mockGeolocationPermission;
We don't prepend "m_" for a static field in WebKit style.


WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1303
 +      WebGeolocationServiceMock::setMockGeolocationError(arguments[0].toInt32(), WebString::fromUTF8(arguments[1].toString()));
nit: We have cppVariantToWebString().


r- for the style error.
Comment 3 Marcus Bulach 2010-05-21 03:39:33 PDT 
Created attachment 56691 [details]
Patch
Comment 4 Marcus Bulach 2010-05-21 03:43:10 PDT 
thanks Tamura-san!

replies inline:

(In reply to comment #2)
> (From update of attachment 56620 [details])
> Thank you for making the patch.
> WebKitTools/DumpRenderTree/chromium is not the primary tester yet.  You need to change webkit/tools/test_shell/ in Chromium too.

yep, the change has been reviewed and I'll submit it soon:
http://codereview.chromium.org/2094003/show

> 
> 
> WebKit/chromium/public/WebGeolocationServiceMock.h:55
>  +      static bool m_mockGeolocationPermission;
> We don't prepend "m_" for a static field in WebKit style.

Done.

> 
> 
> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1303
>  +      WebGeolocationServiceMock::setMockGeolocationError(arguments[0].toInt32(), WebString::fromUTF8(arguments[1].toString()));
> nit: We have cppVariantToWebString().

ahn, thanks! I went ahead and replaced all occurrences so it won't catch the next reader.
however, please, let me know if you'd prefer to this tidy up in a separate patch, and I'd be happy to revert these changes.

would you mind taking another look? thanks!

> 
> 
> r- for the style error.
Comment 5 Kent Tamura 2010-05-21 03:46:00 PDT 
Comment on attachment 56691 [details]
Patch

OK.
Comment 6 WebKit Commit Bot 2010-05-23 02:07:44 PDT 
Comment on attachment 56691 [details]
Patch

Clearing flags on attachment: 56691

Committed r60032: <http://trac.webkit.org/changeset/60032>
Comment 7 WebKit Commit Bot 2010-05-23 02:07:50 PDT 
All reviewed patches have been landed.  Closing bug.