Pass through geofencing API calls to the browser process.

Pass through geofencing API calls to the browser process.

BUG=383125
Committed: https://crrev.com/5741bc85329c968d4590ceba67148b1dced8621d
Cr-Commit-Position: refs/heads/master@{#297892}

[Marijn Kruisselbrink, 2014-09-05 19:45:13]

Patchset #3 (id:40001) has been deleted

[Marijn Kruisselbrink, 2014-09-05 21:50:34]

Patchset #3 (id:60001) has been deleted

[Marijn Kruisselbrink, 2014-09-05 21:54:30]

mek@chromium.org changed reviewers:
+ jochen@chromium.org

[Marijn Kruisselbrink, 2014-09-05 21:54:31]

This is the (start of) the content side of implementing geofencing, getting
closer to where I can actually implement these API calls.
This of course depends on the blink side changes in
https://codereview.chromium.org/464073002/ (as well as the fix to the geofencing
layouttests in https://codereview.chromium.org/543203003/)

[jochen (gone - plz use gerrit), 2014-09-11 12:47:27]

overall looks good.

you'll need a security review

I'd recommend getting a review from somebody familiar with geolocation as well

https://codereview.chromium.org/476293002/diff/80001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/476293002/diff/80001/content/child/blink_plat...
content/child/blink_platform_impl.cc:1002: return
web_geofencing_provider_.get();
how will you test the feature in layout tests?

https://codereview.chromium.org/476293002/diff/80001/content/child/geofencing...
File content/child/geofencing/web_geofencing_provider_impl.h (right):

https://codereview.chromium.org/476293002/diff/80001/content/child/geofencing...
content/child/geofencing/web_geofencing_provider_impl.h:27:
blink::WebGeofencingCallbacks* callbacks) OVERRIDE;
no OVERRIDE on blink API implementation

https://codereview.chromium.org/476293002/diff/80001/content/public/common/ge...
File content/public/common/geofencing_status.h (right):

https://codereview.chromium.org/476293002/diff/80001/content/public/common/ge...
content/public/common/geofencing_status.h:5: #ifndef
CONTENT_PUBLIC_COMMON_GEOFENCING_STATUS_H_
why is this in content/public?

[Marijn Kruisselbrink, 2014-09-11 17:57:52]

https://codereview.chromium.org/476293002/diff/80001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/476293002/diff/80001/content/child/blink_plat...
content/child/blink_platform_impl.cc:1002: return
web_geofencing_provider_.get();
On 2014/09/11 12:47:27, jochen wrote:
> how will you test the feature in layout tests?
To be honest I have no idea. The actual feature I'll probably be able to test
with unittests purely on the browserside, which should test 99% of the
functionality since API calls are passed on more or less unmodified. But I'm not
sure how I would test the other 1% to make sure things are passed around
correctly. Any suggestions are welcome.

https://codereview.chromium.org/476293002/diff/80001/content/child/geofencing...
File content/child/geofencing/web_geofencing_provider_impl.h (right):

https://codereview.chromium.org/476293002/diff/80001/content/child/geofencing...
content/child/geofencing/web_geofencing_provider_impl.h:27:
blink::WebGeofencingCallbacks* callbacks) OVERRIDE;
On 2014/09/11 12:47:27, jochen wrote:
> no OVERRIDE on blink API implementation
Done. But why is this? Is this to make it easier to change signatures on the
blink side? Also BlinkPlatformImpl does have OVERRIDE for at least part of its
blink API implementation...

https://codereview.chromium.org/476293002/diff/80001/content/public/common/ge...
File content/public/common/geofencing_status.h (right):

https://codereview.chromium.org/476293002/diff/80001/content/public/common/ge...
content/public/common/geofencing_status.h:5: #ifndef
CONTENT_PUBLIC_COMMON_GEOFENCING_STATUS_H_
On 2014/09/11 12:47:27, jochen wrote:
> why is this in content/public?
That's what I was wondering... Probably because when I wrote it I didn't
understand when to put something in public and when not, and I hadn't yet gotten
around to moving it back to its correct place (although in the future some of it
might need to become public/, there really is no reason to put it there before
it becomes necessary).

[Marijn Kruisselbrink, 2014-09-11 19:16:28]

mek@chromium.org changed reviewers:
+ jln@chromium.org, mvanouwerkerk@chromium.org

[Marijn Kruisselbrink, 2014-09-11 19:16:29]

+jln for IPC/security
+mvanouwerkerk as somebody that knows the geolocation code

[jochen (gone - plz use gerrit), 2014-09-12 13:52:54]

https://codereview.chromium.org/476293002/diff/80001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/476293002/diff/80001/content/child/blink_plat...
content/child/blink_platform_impl.cc:1002: return
web_geofencing_provider_.get();
On 2014/09/11 17:57:52, Marijn Kruisselbrink wrote:
> On 2014/09/11 12:47:27, jochen wrote:
> > how will you test the feature in layout tests?
> To be honest I have no idea. The actual feature I'll probably be able to test
> with unittests purely on the browserside, which should test 99% of the
> functionality since API calls are passed on more or less unmodified. But I'm
not
> sure how I would test the other 1% to make sure things are passed around
> correctly. Any suggestions are welcome.

having a closer look, this should probably be in content/renderer's of the blink
platform (which derives from this class).

It has a bunch of hooks to install a mock here. the mock would exist in
content/shell/renderer (see for example the geolocation stuff)

https://codereview.chromium.org/476293002/diff/80001/content/child/geofencing...
File content/child/geofencing/web_geofencing_provider_impl.h (right):

https://codereview.chromium.org/476293002/diff/80001/content/child/geofencing...
content/child/geofencing/web_geofencing_provider_impl.h:27:
blink::WebGeofencingCallbacks* callbacks) OVERRIDE;
On 2014/09/11 17:57:52, Marijn Kruisselbrink wrote:
> On 2014/09/11 12:47:27, jochen wrote:
> > no OVERRIDE on blink API implementation
> Done. But why is this? Is this to make it easier to change signatures on the
> blink side? Also BlinkPlatformImpl does have OVERRIDE for at least part of its
> blink API implementation...

yes, to make it easier.

Apparently not everybody knows this and adds the OVERRIDE macros automatically,
however, the clang plugin will not complain for blink API if you don't

https://codereview.chromium.org/476293002/diff/80001/content/public/common/ge...
File content/public/common/geofencing_status.h (right):

https://codereview.chromium.org/476293002/diff/80001/content/public/common/ge...
content/public/common/geofencing_status.h:5: #ifndef
CONTENT_PUBLIC_COMMON_GEOFENCING_STATUS_H_
On 2014/09/11 17:57:52, Marijn Kruisselbrink wrote:
> On 2014/09/11 12:47:27, jochen wrote:
> > why is this in content/public?
> That's what I was wondering... Probably because when I wrote it I didn't
> understand when to put something in public and when not, and I hadn't yet
gotten
> around to moving it back to its correct place (although in the future some of
it
> might need to become public/, there really is no reason to put it there before
> it becomes necessary).

it should be in public/ if somebody outside of content (e.g. components, chrome,
or content/shell) wants to access it, otherwise it should be in e.g. here
content/common

[Michael van Ouwerkerk, 2014-09-15 13:06:50]

https://codereview.chromium.org/476293002/diff/100001/content/child/blink_pla...
File content/child/blink_platform_impl.h (right):

https://codereview.chromium.org/476293002/diff/100001/content/child/blink_pla...
content/child/blink_platform_impl.h:184: scoped_ptr<WebGeofencingProviderImpl>
web_geofencing_provider_;
Nit: no need to prefix the variable name with 'web_' as there is no chance of
confusion here.

https://codereview.chromium.org/476293002/diff/100001/content/child/geofencin...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/476293002/diff/100001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:66:
blink::WebGeofencingCallbacks* raw_callbacks) {
Why are these variables sometimes called "raw_callbacks" and sometimes just
"callbacks"?

https://codereview.chromium.org/476293002/diff/100001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:93: NOTREACHED() <<
"Re-instantiating TLS GeofencingDispatcher.";
Why can't this be a CHECK?

[Marijn Kruisselbrink, 2014-09-17 00:04:25]

michael: hopefully addressed your comments
jln: can you please take a look at this as a security/IPC OWNER?
jochen: other than the security OWNERS review, anything else I need to do to be
able to land this?

https://codereview.chromium.org/476293002/diff/100001/content/child/blink_pla...
File content/child/blink_platform_impl.h (right):

https://codereview.chromium.org/476293002/diff/100001/content/child/blink_pla...
content/child/blink_platform_impl.h:184: scoped_ptr<WebGeofencingProviderImpl>
web_geofencing_provider_;
On 2014/09/15 13:06:49, Michael van Ouwerkerk wrote:
> Nit: no need to prefix the variable name with 'web_' as there is no chance of
> confusion here.

Done.

https://codereview.chromium.org/476293002/diff/100001/content/child/geofencin...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/476293002/diff/100001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:66:
blink::WebGeofencingCallbacks* raw_callbacks) {
On 2014/09/15 13:06:49, Michael van Ouwerkerk wrote:
> Why are these variables sometimes called "raw_callbacks" and sometimes just
> "callbacks"?
At some point I named them raw_callbacks because I then wrapped them in a
scoped_ptr callbacks to make ownership clear. But then later I changed the code
to store the callbacks directly in the IDMap's, making the scoped_ptrs a bit
pointless. Anyway, I renamed them back to be just callbacks.

https://codereview.chromium.org/476293002/diff/100001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:93: NOTREACHED() <<
"Re-instantiating TLS GeofencingDispatcher.";
On 2014/09/15 13:06:50, Michael van Ouwerkerk wrote:
> Why can't this be a CHECK?
NOTREACHED would be the equivalent of a DCHECK, not a CHECK.
I assume the reason it was written like this in ServiceWorkerDispatcher (where I
copied this code from) is because using a DCHECK would mean duplicating the
condition (once in the DCHECK and once to reset the pointer to NULL in order not
to crash). Of course technically we're not supposed to handle situations where a
DCHECK would be hit...

[Michael van Ouwerkerk, 2014-09-17 10:16:55]

On 2014/09/17 00:04:25, Marijn Kruisselbrink wrote:
> jochen: other than the security OWNERS review, anything else I need to do to
be
> able to land this?

If you install Chromite Butler you can easily see whether you have enough
reviewers for OWNERS. In the current case all files show dark green meaning an
OWNER is in the reviewer list.

https://chrome.google.com/webstore/detail/chromite-butler/bhcnanendmgjjeghama...

[Michael van Ouwerkerk, 2014-09-17 10:17:50]

On 2014/09/17 10:16:55, Michael van Ouwerkerk wrote:
> On 2014/09/17 00:04:25, Marijn Kruisselbrink wrote:
> > jochen: other than the security OWNERS review, anything else I need to do to
> be
> > able to land this?
> 

You might run "git cl try" to verify all is good.

[Michael van Ouwerkerk, 2014-09-17 10:22:31]

On 2014/09/17 00:04:25, Marijn Kruisselbrink wrote:
> > Why can't this be a CHECK?
> NOTREACHED would be the equivalent of a DCHECK, not a CHECK.
> I assume the reason it was written like this in ServiceWorkerDispatcher (where
I
> copied this code from) is because using a DCHECK would mean duplicating the
> condition (once in the DCHECK and once to reset the pointer to NULL in order
not
> to crash). Of course technically we're not supposed to handle situations where
a
> DCHECK would be hit...

Just so you are careful, not all SW code has been thoroughly reviewed. Just
because they are doing it a certain way does not guarantee it is right. So,
always make up your own mind about it.

[Michael van Ouwerkerk, 2014-09-17 10:22:59]

lgtm

[jochen (gone - plz use gerrit), 2014-09-17 18:26:45]

https://codereview.chromium.org/476293002/diff/120001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/476293002/diff/120001/content/child/blink_pla...
content/child/blink_platform_impl.cc:1005: blink::WebGeofencingProvider*
BlinkPlatformImpl::geofencingProvider() {
the WebGeofencingProvider only exists on the main thread, right? Since workers
can't access that directly, why bother with the fancy per thread dispatcher and
filter?

[Marijn Kruisselbrink, 2014-09-18 16:32:06]

On 2014/09/17 10:16:55, Michael van Ouwerkerk wrote:
> If you install Chromite Butler you can easily see whether you have enough
> reviewers for OWNERS. In the current case all files show dark green meaning an
> OWNER is in the reviewer list.
Sorry, my question wasn't quite phrased properly. I know the currently selected
OWNERS cover all files, my question was more about what it would take to get
these OWNERS to give their green light.

https://codereview.chromium.org/476293002/diff/120001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/476293002/diff/120001/content/child/blink_pla...
content/child/blink_platform_impl.cc:1005: blink::WebGeofencingProvider*
BlinkPlatformImpl::geofencingProvider() {
On 2014/09/17 18:26:44, jochen wrote:
> the WebGeofencingProvider only exists on the main thread, right? Since workers
> can't access that directly, why bother with the fancy per thread dispatcher
and
> filter?
No, that's not right. Just like the WebCrypto above, and probably several other
APIs, blink calls these methods on any thread, and thus WebGeofencingProvider
implementations have to be able to deal with its methods being called on
multiple threads as well. To do that I opted to do the same the service worker
code does, with the per-thread dispatcher, and a separate message filter to get
the replies back on the correct thread, but there are of course other ways to
accomplish the same. The WebCrypto code for example just posts messages to a
(renderer side) web crypto thread and deals with crypto stuff there.

So some other possible approaches I could take here to merge
WebGeofencingProviderImpl, GeofencingDispatcher and GeofencingMessageFilter
would involve either routing all API calls through one thread, or somehow using
a thread-safe IDMap to store request.

[jochen (gone - plz use gerrit), 2014-09-18 18:39:26]

ok, lgtm

[Marijn Kruisselbrink, 2014-09-24 17:16:04]

mek@chromium.org changed reviewers:
+ palmer@chromium.org

[Marijn Kruisselbrink, 2014-09-24 17:16:06]

+palmer: hopefully you'll have more time for the IPC OWNERS review

[Marijn Kruisselbrink, 2014-09-25 16:34:59]

mek@chromium.org changed reviewers:
+ wfh@chromium.org

[Marijn Kruisselbrink, 2014-09-25 16:34:59]

+wfh: maybe you'll have time for a IPC OWNERS review?

[palmer, 2014-09-25 19:44:13]

https://codereview.chromium.org/476293002/diff/140001/content/common/geofenci...
File content/common/geofencing_messages.h (right):

https://codereview.chromium.org/476293002/diff/140001/content/common/geofenci...
content/common/geofencing_messages.h:30: std::string /* region_id */,
What is the lexical/syntactic structure of these strings?

(Where possible, I always seek to use some more constrained type.)

https://codereview.chromium.org/476293002/diff/140001/content/common/geofenci...
content/common/geofencing_messages.h:36: std::string /* regionId */)
Nit: "region_id"

[Marijn Kruisselbrink, 2014-09-25 19:55:23]

https://codereview.chromium.org/476293002/diff/140001/content/common/geofenci...
File content/common/geofencing_messages.h (right):

https://codereview.chromium.org/476293002/diff/140001/content/common/geofenci...
content/common/geofencing_messages.h:30: std::string /* region_id */,
On 2014/09/25 19:44:12, Chromium Palmer wrote:
> What is the lexical/syntactic structure of these strings?
> 
> (Where possible, I always seek to use some more constrained type.)
Currently these strings are completely unrestricted. It's up to websites to
attach meaning/structure to their region ids. Although we'll probably need to
add some restrictions on what websites can do with this field; having
multi-megabyte IDs certainly would be problematic; this isn't a part that is
very well specced out yet, so any suggestions are welcome.

https://codereview.chromium.org/476293002/diff/140001/content/common/geofenci...
content/common/geofencing_messages.h:36: std::string /* regionId */)
On 2014/09/25 19:44:12, Chromium Palmer wrote:
> Nit: "region_id"

Done.

[palmer, 2014-09-25 20:17:48]

> Currently these strings are completely unrestricted. It's up to websites to
> attach meaning/structure to their region ids. Although we'll probably need to
> add some restrictions on what websites can do with this field; having
> multi-megabyte IDs certainly would be problematic; this isn't a part that is
> very well specced out yet, so any suggestions are welcome.

Are the strings going to be used in the UX?

Assuming yes, how about limiting them to N * 10^2 Unicode printable characters?
(N is 1 or maybe 2?)

[Marijn Kruisselbrink, 2014-09-25 20:28:35]

On 2014/09/25 20:17:48, Chromium Palmer wrote:
> > Currently these strings are completely unrestricted. It's up to websites to
> > attach meaning/structure to their region ids. Although we'll probably need
to
> > add some restrictions on what websites can do with this field; having
> > multi-megabyte IDs certainly would be problematic; this isn't a part that is
> > very well specced out yet, so any suggestions are welcome.
> 
> Are the strings going to be used in the UX?
> 
> Assuming yes, how about limiting them to N * 10^2 Unicode printable
characters?
> (N is 1 or maybe 2?)
No, they're not expected to be user readable. They are really only meant for
javascript code to be able to associate events with registrations (similar to
the "requestId" in androids geofencing API, which seems to indeed be limited to
100 characters according to
http://developer.android.com/reference/com/google/android/gms/location/Geofen...
). So yes, limiting this to 100 characters in the web geofencing API probably
makes sense as well (although it probably makes most sense to do so on the blink
side; I'll add it to my todo list).

[palmer, 2014-09-25 20:30:24]

It does make sense to do it on the Blink side, but keep in mind that the browser
process can't trust Blink to have done that (in case of compromise). So we still
want to enforce the limitation on the browser side, too.

[Marijn Kruisselbrink, 2014-09-25 20:38:41]

On 2014/09/25 20:30:24, Chromium Palmer wrote:
> It does make sense to do it on the Blink side, but keep in mind that the
browser
> process can't trust Blink to have done that (in case of compromise). So we
still
> want to enforce the limitation on the browser side, too.
Yes, of course. I'll keep that in mind when implementing the browser-side part
of this API, unless you want me to just add that one check here already? Would
it be correct to just reply to the renderer with some kind of
GEOFENCING_STATUS_OPERATION_FAILED error in case of too long IDs (like
GeofencingDispatchHost currently does for all messages), or should this cause
some other "this renderer is compromised" action?

[palmer, 2014-10-02 17:37:26]

|> > process can't trust Blink to have done that (in case of compromise). So we
> still
> > want to enforce the limitation on the browser side, too.
> Yes, of course. I'll keep that in mind when implementing the browser-side part
> of this API, unless you want me to just add that one check here already?

Doing it now seems better, yes.

> Would
> it be correct to just reply to the renderer with some kind of
> GEOFENCING_STATUS_OPERATION_FAILED error in case of too long IDs (like
> GeofencingDispatchHost currently does for all messages), or should this cause
> some other "this renderer is compromised" action?

I think just responding with GEOFENCING_STATUS_OPERATION_FAILED is fine.

[Marijn Kruisselbrink, 2014-10-02 18:19:50]

On 2014/10/02 17:37:26, Chromium Palmer wrote:
> |> > process can't trust Blink to have done that (in case of compromise). So
we
> > still
> > > want to enforce the limitation on the browser side, too.
> > Yes, of course. I'll keep that in mind when implementing the browser-side
part
> > of this API, unless you want me to just add that one check here already?
> 
> Doing it now seems better, yes.
> 
> > Would
> > it be correct to just reply to the renderer with some kind of
> > GEOFENCING_STATUS_OPERATION_FAILED error in case of too long IDs (like
> > GeofencingDispatchHost currently does for all messages), or should this
cause
> > some other "this renderer is compromised" action?
> 
> I think just responding with GEOFENCING_STATUS_OPERATION_FAILED is fine.

Okay, I added sanity checks for the region_id when it is received in an IPC.

[palmer, 2014-10-02 18:24:00]

LGTM. Thanks! And I apologize again for the delay.

https://codereview.chromium.org/476293002/diff/180001/content/browser/geofenc...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/476293002/diff/180001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:13: static const int
kMaxRegionIdLength = 200;
Note that, in the future, if you need to increase this, that is OK. It's just
that we want to have *some* limit.

https://codereview.chromium.org/476293002/diff/180001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:58: if
(region_id.length() > kMaxRegionIdLength) {
At some point, a unit test for this would be good. (Can come in a later CL.)

[Marijn Kruisselbrink, 2014-10-02 19:07:32]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-10-02 19:09:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/476293002/180001

[commit-bot: I haz the power, 2014-10-02 20:38:31]

Committed patchset #8 (id:180001) as 57493bea77b3aff03ff6dc51edfe3214824a06ba

[commit-bot: I haz the power, 2014-10-02 20:39:14]

Patchset 8 (id:??) landed as
https://crrev.com/5741bc85329c968d4590ceba67148b1dced8621d
Cr-Commit-Position: refs/heads/master@{#297892}