Bug 30206

Summary: Geolocation lacks V8 bindings
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Steve Block <steveblock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, android-webkit-unforking, commit-queue, eric, joth, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch 1 for Bug 30206
none
Patch 2 for Bug 30206
dglazkov: review-
Patch 3 for Bug 30206 none

Description Steve Block 2009-10-08 05:01:03 PDT
Geolocation lacks V8 bindings.
Comment 1 Steve Block 2009-11-26 12:44:20 PST
Created attachment 43930 [details]
Patch 1 for Bug 30206
Comment 2 Steve Block 2009-11-30 07:57:14 PST
Created attachment 44021 [details]
Patch 2 for Bug 30206

Updates build files for Chromium
Comment 3 Adam Barth 2009-11-30 12:51:24 PST
Attachment 44021 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:33:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:51:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:42:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:51:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:69:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Done processing WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp
Done processing WebCore/bindings/v8/DOMObjectsInclude.h
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:107:  Alphabetical sorting problem.  [build/include_order] [4]
Done processing WebCore/bindings/v8/DerivedSourcesAllInOne.cpp
Done processing WebCore/bindings/v8/custom/V8CustomPositionCallback.h
Done processing WebCore/bindings/v8/custom/V8CustomBinding.h
Done processing WebCore/bindings/v8/custom/V8CustomPositionErrorCallback.cpp
Done processing WebCore/bindings/v8/V8Index.cpp
Done processing WebCore/bindings/v8/custom/V8CustomPositionCallback.cpp
Done processing WebCore/bindings/v8/V8Index.h
WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Done processing WebCore/bindings/v8/custom/V8GeolocationCustom.cpp
Done processing WebCore/bindings/v8/custom/V8CustomPositionErrorCallback.h
Total errors found: 12
Comment 4 Dimitri Glazkov (Google) 2009-11-30 12:53:14 PST
Comment on attachment 44021 [details]
Patch 2 for Bug 30206

Yeah, what the style-elf said.

BTW, do we need compile guards around geolocation?
Comment 5 Steve Block 2009-12-01 04:24:03 PST
Created attachment 44067 [details]
Patch 3 for Bug 30206

> Yeah, what the style-elf said.
Fixed. The script still reports a couple of errors regarding include order, because V8GeolocationCustom and V8CoordinatesCustom don't have a 'primary header' that follows the standard naming pattern.

> BTW, do we need compile guards around geolocation?
No, the decision taken for Geolocation was to not guard the (majority of the) code with enable flags, so as to make sure it's always built and doesn't rot (See Bug 25756). The guards are used only in Navigator.idl (to conditionally expose the JS geolocation object) and in GeolocationService.cpp (to satisfy the linker on platforms that don't implement GeolocationService).
Comment 6 WebKit Review Bot 2009-12-01 04:27:48 PST
Attachment 44067 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2
Comment 7 Dimitri Glazkov (Google) 2009-12-01 08:33:50 PST
Comment on attachment 44067 [details]
Patch 3 for Bug 30206

ok.
Comment 8 Eric Seidel (no email) 2009-12-01 08:46:36 PST
(In reply to comment #6)
> Attachment 44067 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/bindings/v8/custom/V8CoordinatesCustom.cpp:27:  Found other header
> before a header this file implements. Should be: config.h, primary header,
> blank line, and then alphabetically sorted.  [build/include_order] [4]
> WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:27:  Found other header
> before a header this file implements. Should be: config.h, primary header,
> blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 2

Do we need to file a bug against check-webkit-style here?
Comment 9 WebKit Commit Bot 2009-12-01 08:46:47 PST
Comment on attachment 44067 [details]
Patch 3 for Bug 30206

Clearing flags on attachment: 44067

Committed r51540: <http://trac.webkit.org/changeset/51540>
Comment 10 WebKit Commit Bot 2009-12-01 08:46:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Adam Barth 2009-12-01 10:50:04 PST
> Do we need to file a bug against check-webkit-style here?

Yes.  I think it doesn't understand includes for the custom bindings.  Can you file this one?
Comment 12 Eric Seidel (no email) 2009-12-01 13:26:33 PST
Filed bug 32033 to cover the false positives.