Bug 123658

Summary: [WK2] Update WebGeolocationManager to support multiple WebContent processes
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch andersca: review+

Description Benjamin Poulain 2013-11-01 23:06:01 PDT
[WK2] Update WebGeolocationManager to support multiple WebContent processes
Comment 1 Benjamin Poulain 2013-11-01 23:10:12 PDT
Created attachment 215797 [details]
Patch
Comment 2 Anders Carlsson 2013-11-04 14:26:10 PST
Comment on attachment 215797 [details]
Patch

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

> Source/WebKit2/UIProcess/WebContext.cpp:685
> +        static_cast<WebContextSupplement*>(supplement<WebGeolocationManagerProxy>())->processDidClose(process);

Don’t you want to call this on all the WebContextSupplements?

> Source/WebKit2/UIProcess/WebGeolocationManagerProxy.h:72
> +    HashSet<const CoreIPC::Connection::Client*> m_updateRequesters;

Instead of using a HashSet, could you simply have a counter that keeps track of the number of requests?
Comment 3 Benjamin Poulain 2013-11-04 14:40:46 PST
(In reply to comment #2)
> (From update of attachment 215797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215797&action=review
> 
> > Source/WebKit2/UIProcess/WebContext.cpp:685
> > +        static_cast<WebContextSupplement*>(supplement<WebGeolocationManagerProxy>())->processDidClose(process);
> 
> Don’t you want to call this on all the WebContextSupplements?

The other WebContextSupplement do not support multiple WebProcess. They track the state as if there was a single process communicating with them.
As a result, they do very bad things on processDidClose(). (I filed <rdar://problem/15376383>)

> > Source/WebKit2/UIProcess/WebGeolocationManagerProxy.h:72
> > +    HashSet<const CoreIPC::Connection::Client*> m_updateRequesters;
> 
> Instead of using a HashSet, could you simply have a counter that keeps track of the number of requests?

The HashSet is a defensive mechanism against mistake in the WebProcess. If for any reason the GeolocationController starts or stops querying twice, the HashSet ensure the state on the UIProcess remains valid.
Comment 4 Benjamin Poulain 2013-11-04 15:01:23 PST
Committed r158600: <http://trac.webkit.org/changeset/158600>