Add a Physical Web data source

Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=630769
Committed: https://crrev.com/c565df2c11f431f8711db353e7ec00d48ab9901c
Cr-Commit-Position: refs/heads/master@{#410829}

[mattreynolds, 2016-06-29 21:51:58]

mattreynolds@chromium.org changed reviewers:
+ olivierrobin@chromium.org

[mattreynolds, 2016-06-29 21:51:58]

Hi Olivier, PTAL

[Olivier, 2016-06-30 08:14:11]

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/browser/ios_...
File ios/chrome/browser/ios_chrome_main_parts.mm (right):

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/browser/ios_...
ios/chrome/browser/ios_chrome_main_parts.mm:142:
PhysicalWebDataSource::SetInstance(
Do we want to create this object if the experimental flag is disabled ?

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_data_source_ios.mm (right):

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_data_source_ios.mm:18: [scanner_
setNetworkRequestEnabled:(BOOL)network_request_enabled];
no C cast.
Use static_cast<BOOL> instead

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/ios_chrome.gyp
File ios/chrome/ios_chrome.gyp (right):

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/ios_chrome.g...
ios/chrome/ios_chrome.gyp:780:
'common/physical_web/physical_web.gyp:physical_web',
alpha

[mattreynolds, 2016-06-30 17:51:58]

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/browser/ios_...
File ios/chrome/browser/ios_chrome_main_parts.mm (right):

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/browser/ios_...
ios/chrome/browser/ios_chrome_main_parts.mm:142:
PhysicalWebDataSource::SetInstance(
On 2016/06/30 08:14:11, Olivier Robin wrote:
> Do we want to create this object if the experimental flag is disabled ?

Done.

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/physical_web_data_source_ios.mm (right):

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/physical_web_data_source_ios.mm:18: [scanner_
setNetworkRequestEnabled:(BOOL)network_request_enabled];
On 2016/06/30 08:14:11, Olivier Robin wrote:
> no C cast.
> Use static_cast<BOOL> instead

Done.

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/ios_chrome.gyp
File ios/chrome/ios_chrome.gyp (right):

https://codereview.chromium.org/2113473002/diff/20001/ios/chrome/ios_chrome.g...
ios/chrome/ios_chrome.gyp:780:
'common/physical_web/physical_web.gyp:physical_web',
On 2016/06/30 08:14:11, Olivier Robin wrote:
> alpha

Done.

[mattreynolds, 2016-06-30 17:56:00]

mattreynolds@chromium.org changed reviewers:
+ sdefresne@chromium.org

[mattreynolds, 2016-06-30 17:56:00]

Hi Sylvain, PTAL

[mattreynolds, 2016-07-08 18:07:53]

ping

[olivierrobin, 2016-07-11 12:34:18]

olivierrobin@google.com changed reviewers:
+ olivierrobin@google.com

[olivierrobin, 2016-07-11 12:34:19]

lgtm

[mattreynolds, 2016-07-12 19:09:47]

Description was changed from

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++.

BUG=600616
==========

to

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++.

BUG=600616
==========

[mattreynolds, 2016-07-12 19:09:47]

mattreynolds@chromium.org changed reviewers:
+ blundell@chromium.org, rohitrao@chromium.org

[mattreynolds, 2016-07-12 19:11:31]

Rohit, please review the changes in ios/chrome/*

Colin, please review the changes in components/*

[blundell, 2016-07-13 08:47:05]

The new component should have OWNERS (ideally, more than just you, and ideally
one of them should review this CL as well).

What's the logic for the separation between //components and //ios/chrome? Is
there an intent to bring this up on other platforms as well?

The CL description could use some fleshing out (e.g., to address the above
question).

[sdefresne, 2016-07-13 10:03:49]

On 2016/07/13 08:47:05, blundell wrote:
> The new component should have OWNERS (ideally, more than just you, and ideally
> one of them should review this CL as well).
> 
> What's the logic for the separation between //components and //ios/chrome? Is
> there an intent to bring this up on other platforms as well?
> 
> The CL description could use some fleshing out (e.g., to address the above
> question).

There is an effort to remove small components that no longer have a valid reason
to be there (mostly because they only have one embedder using them). Given this,
answering the question that Colin raised should be considered blocking for
adding this component.

[mattreynolds, 2016-07-13 18:07:52]

On 2016/07/13 08:47:05, blundell (OOO until July 18) wrote:
> The new component should have OWNERS (ideally, more than just you, and ideally
> one of them should review this CL as well).
> 
> What's the logic for the separation between //components and //ios/chrome? Is
> there an intent to bring this up on other platforms as well?
> 
> The CL description could use some fleshing out (e.g., to address the above
> question).

Olivier, can I add you as an owner of //components/physical_web_data_source? 
(assuming we don't find a better place for it)

Yes, if this experiment is successful we'd like to share as much as possible
with a future Android implementation.  However, the main reason I created this
as a new component is to make the Physical Web metadata accessible from an
omnibox autocomplete provider.  The providers are in //components/omnibox and
probably shouldn't depend directly on //ios/chrome.  All the other data source
dependents are in ios or ios_internal.  The same issue affects the "Open from
Clipboard" autocomplete provider which uses //components/open_from_clipboard to
capture the iOS clipboard content and make it available as an autocomplete
match.

[mattreynolds, 2016-07-13 18:10:11]

Description was changed from

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++.

BUG=600616
==========

to

==========
Add a Physical Web data source
=========|=========|=========|=========|=========|=========|=========|=========|
Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=600616
==========

[mattreynolds, 2016-07-13 18:10:35]

Description was changed from

==========
Add a Physical Web data source
=========|=========|=========|=========|=========|=========|=========|=========|
Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=600616
==========

to

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=600616
==========

[blundell, 2016-07-18 06:39:57]

OK, this reasoning seems pretty sound -- there is no way currently for the
embedder to inject providers into the autocomplete component (although I doubt
that would be too tricky to add).

Do you have a design doc for this work? The new Chrome policy is to have design
docs for all "new nontrivial efforts or features", which this seems to fall into
(search for the internal chrome-team@ thread "Changes to Chrome design doc and
eng review processes".)

In addition to covering design aspects such as the intended number of platforms
supported, overall feature architecture, etc, this could also cover smaller
questions like why a singleton is needed.

Doing this now (in addition to being a new requirement for embarking on this
sort of feature work in Chromium) will likely make your future CLs easier to
land, although it will of course slow this one down.

[chromium-reviews, 2016-07-18 14:31:35]

Tes, you can add me as owner.
I am OOO this week so I answer mails slowly

Olivier

Le 13 juil. 2016 8:07 PM, <mattreynolds@chromium.org> a écrit :

> On 2016/07/13 08:47:05, blundell (OOO until July 18) wrote:
> > The new component should have OWNERS (ideally, more than just you, and
> ideally
> > one of them should review this CL as well).
> >
> > What's the logic for the separation between //components and
> //ios/chrome? Is
> > there an intent to bring this up on other platforms as well?
> >
> > The CL description could use some fleshing out (e.g., to address the
> above
> > question).
>
> Olivier, can I add you as an owner of
> //components/physical_web_data_source?
> (assuming we don't find a better place for it)
>
> Yes, if this experiment is successful we'd like to share as much as
> possible
> with a future Android implementation. However, the main reason I created
> this
> as a new component is to make the Physical Web metadata accessible from an
> omnibox autocomplete provider. The providers are in //components/omnibox
> and
> probably shouldn't depend directly on //ios/chrome. All the other data
> source
> dependents are in ios or ios_internal. The same issue affects the "Open
> from
> Clipboard" autocomplete provider which uses
> //components/open_from_clipboard to
> capture the iOS clipboard content and make it available as an autocomplete
> match.
>
>
>
> https://codereview.chromium.org/2113473002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mattreynolds, 2016-07-26 00:24:14]

The design doc is up, PTAL:

https://docs.google.com/document/d/1CXN2Dj3QtvLXobXcu--hBsKnWKN8BsOvo9dN9QlzZeM

[blundell, 2016-07-26 06:41:20]

On 2016/07/26 00:24:14, mattreynolds wrote:
> The design doc is up, PTAL:
> 
>
https://docs.google.com/document/d/1CXN2Dj3QtvLXobXcu--hBsKnWKN8BsOvo9dN9QlzZeM

Hi Matt,

That design doc is great, thanks. I left some comments and questions on it. It's
not clear to me what stage the proposal is in from looking at the comments on
the doc (I left a comment on the doc about that :).

[mattreynolds, 2016-07-26 19:00:47]

Description was changed from

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=600616
==========

to

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=630769
==========

[mattreynolds, 2016-07-27 19:05:19]

On 2016/07/26 06:41:20, blundell wrote:
> On 2016/07/26 00:24:14, mattreynolds wrote:
> > The design doc is up, PTAL:
> > 
> >
>
https://docs.google.com/document/d/1CXN2Dj3QtvLXobXcu--hBsKnWKN8BsOvo9dN9QlzZeM
> 
> Hi Matt,
> 
> That design doc is great, thanks. I left some comments and questions on it.
It's
> not clear to me what stage the proposal is in from looking at the comments on
> the doc (I left a comment on the doc about that :).

Thanks for the comments, I made the changes you suggested in the doc.  I don't
think there are any explicit sign-offs required before we start implementation,
but the Chrome new feature process is new to me so perhaps I'm missing
something.

[blundell, 2016-07-28 11:12:25]

https://codereview.chromium.org/2113473002/diff/100001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/2113473002/diff/100001/components/components....
components/components.gyp:71: 'physical_web_data_source.gypi',
nit: This should just be physical_web.gypi now.

https://codereview.chromium.org/2113473002/diff/100001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/100001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:21: static
PhysicalWebDataSource* GetInstance();
Singletons are generally an anti-pattern. Is there any particular reason that
this needs to be a singleton (as opposed to e.g. being a KeyedService with one
instance per Profile/ChromeBrowserState)?

https://codereview.chromium.org/2113473002/diff/100001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2113473002/diff/100001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:14: // iOS
implementation of PhysicalWebDataSource
Will this class only be used by Chrome on iOS (i.e., not by the Today
extension)? If so, it seems like it could be in //ios/chrome/browser, be a
BrowserStateKeyedService, and get around the singleton problem that way. There
would then be one PhysicalWebScanner instance per ChromeBrowserState but from
looking at that class that doesn't seem problematic to me (correct me if I'm
wrong though).

[mattreynolds, 2016-07-29 01:36:31]

https://codereview.chromium.org/2113473002/diff/100001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/2113473002/diff/100001/components/components....
components/components.gyp:71: 'physical_web_data_source.gypi',
On 2016/07/28 11:12:25, blundell wrote:
> nit: This should just be physical_web.gypi now.

Done.

https://codereview.chromium.org/2113473002/diff/100001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/100001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:21: static
PhysicalWebDataSource* GetInstance();
On 2016/07/28 11:12:25, blundell wrote:
> Singletons are generally an anti-pattern. Is there any particular reason that
> this needs to be a singleton (as opposed to e.g. being a KeyedService with one
> instance per Profile/ChromeBrowserState)?

I don't think there's any issue with running multiple instances of the scanner,
but we should limit Chrome to a single instance if possible since the scanner
caches a list of all nearby URLs and requests metadata from our server for each
one.  I don't expect the amount of memory used by the list to be very large
(typically less than a dozen beacons, with <1 kB metadata for each) but it would
be best to not duplicate it unnecessarily or cause extra load on our metadata
service.

In what situations does Chrome use multiple profiles?  I suspect on mobile it's
only for incognito, and we'll likely want to disable physical web features on
incognito anyway so there will typically only be one active scanner.  However,
we wouldn't want to fetch the same metadata more than once on platforms that
support multiple logged-in profiles.

I think we could justify keeping this as a global singleton since the
information it provides is tied to the user's device and shouldn't differ
between profiles.  ClipboardRecentContent is a singleton that solves a similar
problem -- clipboard content is also tied to the device and shouldn't differ
between profiles.

https://codereview.chromium.org/2113473002/diff/100001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2113473002/diff/100001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:14: // iOS
implementation of PhysicalWebDataSource
On 2016/07/28 11:12:25, blundell wrote:
> Will this class only be used by Chrome on iOS (i.e., not by the Today
> extension)? If so, it seems like it could be in //ios/chrome/browser, be a
> BrowserStateKeyedService, and get around the singleton problem that way. There
> would then be one PhysicalWebScanner instance per ChromeBrowserState but from
> looking at that class that doesn't seem problematic to me (correct me if I'm
> wrong though).

The Today extension will most likely not use this class.

[blundell, 2016-07-29 12:10:22]

I'm OOO for the next two weeks. Sylvain is both a //components OWNER and an
//ios OWNER; I recommend that he take over the review to avoid delaying you. I
talked to him about this CL yesterday so he should be basically up-to-date.

If you're tied to using a singleton, I'd prefer having the singleton be
embedder-level instead of components-level.
That is:

- Have physical_web_data_source.h just be a pure virtual interface that is
injected into the //components-level code that will need it.

- In //ios/chrome, have ApplicationContext own an instance of
IOSChromePhysicalDataSource the way that it does many other objects, and when
you need to invoke relevant //components-level code from //ios/chrome, access
the instance via GetApplicationContext()->GetPhysicalWebDataSource().

This way we wouldn't have global state leaking down into the //components level
where it doesn't really belong (of course, I'm sure there are existing examples,
some of which I may have even created :P). In addition, we're making use of the
existing ApplicationContext abstraction in //ios/chrome, which exists
essentially exactly for this purpose (and similarly in //chrome there's
BrowserProcess).

That said, I don't really understand the API of PhysicalWebDataSource that well
wrt the caching issue. Given that obtaining physical URLs and resolving them is
asynchronous, how does the client know when to call GetMetadata() after calling
StartDiscovery()? If there are multiple clients of a single
PhysicalWebDataSource instance, how does client 2 know whether it needs to call
StartDiscovery() or whether client 1 has already done it and client 2 should
just immediately be calling GetMetadata()?

I would expect the API surface of PhysicalWebDataSource to look something like
this:

// Starts discovering and resolving URLs, calling |on_obtained_results| when
finished. Will reuse any results that have been obtained by a previous call to
StartDiscovery() without an intervening call to StopDiscovery().
StartDiscovery(base::Callback on_obtained_results);

// Stops discovery and discards any cached results. The next call to
StartDiscovery() will obtain a fresh set of results.
StopDiscovery();

[mattreynolds, 2016-08-01 23:20:39]

On 2016/07/29 12:10:22, blundell (OOO until August 16) wrote:
> I'm OOO for the next two weeks. Sylvain is both a //components OWNER and an
> //ios OWNER; I recommend that he take over the review to avoid delaying you. I
> talked to him about this CL yesterday so he should be basically up-to-date.
> 
> If you're tied to using a singleton, I'd prefer having the singleton be
> embedder-level instead of components-level.
> That is:
> 
> - Have physical_web_data_source.h just be a pure virtual interface that is
> injected into the //components-level code that will need it.
> 
> - In //ios/chrome, have ApplicationContext own an instance of
> IOSChromePhysicalDataSource the way that it does many other objects, and when
> you need to invoke relevant //components-level code from //ios/chrome, access
> the instance via GetApplicationContext()->GetPhysicalWebDataSource().
> 
> This way we wouldn't have global state leaking down into the //components
level
> where it doesn't really belong (of course, I'm sure there are existing
examples,
> some of which I may have even created :P). In addition, we're making use of
the
> existing ApplicationContext abstraction in //ios/chrome, which exists
> essentially exactly for this purpose (and similarly in //chrome there's
> BrowserProcess).
> 
> That said, I don't really understand the API of PhysicalWebDataSource that
well
> wrt the caching issue. Given that obtaining physical URLs and resolving them
is
> asynchronous, how does the client know when to call GetMetadata() after
calling
> StartDiscovery()? If there are multiple clients of a single
> PhysicalWebDataSource instance, how does client 2 know whether it needs to
call
> StartDiscovery() or whether client 1 has already done it and client 2 should
> just immediately be calling GetMetadata()?
> 
> I would expect the API surface of PhysicalWebDataSource to look something like
> this:
> 
> // Starts discovering and resolving URLs, calling |on_obtained_results| when
> finished. Will reuse any results that have been obtained by a previous call to
> StartDiscovery() without an intervening call to StopDiscovery().
> StartDiscovery(base::Callback on_obtained_results);
> 
> // Stops discovery and discards any cached results. The next call to
> StartDiscovery() will obtain a fresh set of results.
> StopDiscovery();

message: On 2016/07/29 12:10:22, blundell (OOO until August 16) wrote:
> I'm OOO for the next two weeks. Sylvain is both a //components OWNER and an
> //ios OWNER; I recommend that he take over the review to avoid delaying you. I
> talked to him about this CL yesterday so he should be basically up-to-date.

SGTM, thanks for the comments and enjoy your break!

> If you're tied to using a singleton, I'd prefer having the singleton be
> embedder-level instead of components-level.
> That is:
> 
> - Have physical_web_data_source.h just be a pure virtual interface that is
> injected into the //components-level code that will need it.
> 
> - In //ios/chrome, have ApplicationContext own an instance of
> IOSChromePhysicalDataSource the way that it does many other objects, and when
> you need to invoke relevant //components-level code from //ios/chrome, access
> the instance via GetApplicationContext()->GetPhysicalWebDataSource().
> 
> This way we wouldn't have global state leaking down into the //components
level
> where it doesn't really belong (of course, I'm sure there are existing
examples,
> some of which I may have even created :P). In addition, we're making use of
the
> existing ApplicationContext abstraction in //ios/chrome, which exists
> essentially exactly for this purpose (and similarly in //chrome there's
> BrowserProcess).

ApplicationContext won't work because it needs to be accessible from
//components/omnibox/browser which should not have any //ios/chrome
dependencies.  BrowserProcess looks good though, I'll try attaching it there.

> That said, I don't really understand the API of PhysicalWebDataSource that
well
> wrt the caching issue. Given that obtaining physical URLs and resolving them
is
> asynchronous, how does the client know when to call GetMetadata() after
calling
> StartDiscovery()? If there are multiple clients of a single
> PhysicalWebDataSource instance, how does client 2 know whether it needs to
call
> StartDiscovery() or whether client 1 has already done it and client 2 should
> just immediately be calling GetMetadata()?
> 
> I would expect the API surface of PhysicalWebDataSource to look something like
> this:
> 
> // Starts discovering and resolving URLs, calling |on_obtained_results| when
> finished. Will reuse any results that have been obtained by a previous call to
> StartDiscovery() without an intervening call to StopDiscovery().
> StartDiscovery(base::Callback on_obtained_results);
> 
> // Stops discovery and discards any cached results. The next call to
> StartDiscovery() will obtain a fresh set of results.
> StopDiscovery();

I'll work on fleshing out the documentation in physical_web_data_source.h so
it's clearer how the API is intended to be used.  Here's a first pass to keep
the conversation going:

StartDiscovery starts a discovery service that listens for Bluetooth devices
advertising Physical Web URLs.  The service will scan continuously whenever the
app is in the foreground.  When a URL is discovered, it's added to an internal
list of nearby URLs.  If StartDiscovery is called with
network_request_enabled=true, the URL is additionally sent to a Google service
to verify that the URL is resolvable and to fetch metadata related to the page
(eg: title, description, favicon).  This metadata is cached along with the list
of nearby URLs.  Calling StopDiscovery stops the discovery service and clears
cached URLs and metadata.

When the discovery service is active and network requests are enabled, calling
GetMetadata returns the current list of nearby URLs for which we have metadata. 
Nearby URLs that are not resolvable or for which we could not fetch metadata are
not included.  When the discovery service is not active, or when network
requests are disabled, GetMetadata will always return an empty list.

To enable onboarding scenarios in which the discovery service is active but
network requests are disabled for privacy reasons, the data source exposes a
boolean value HasUnresolvedDiscoveries.  When true, it indicates that one or
more nearby URLs were discovered but additional permission is required before
the metadata can be fetched.

> Given that obtaining physical URLs and resolving them is
asynchronous, how does the client know when to call GetMetadata() after calling
StartDiscovery()?

We'll start the service when Chrome starts (if the preference is enabled) or
when the preference switch is switched on.  Currently, all of the clients that
use the metadata are triggered by user actions (tapping in the omnibox, opening
the WebUI) so it's not necessary to notify the client on metadata updates.  It
should be fine to call GetMetadata at any time, even when the discovery service
is inactive.

In an earlier version I did have a way of registering metadata listeners, but
for the initial implementation this turned out to be unnecessary.  We currently
only access the stored metadata in two places: in the autocomplete provider to
determine how many URLs are nearby (it displays a message like "X web pages
nearby") and in the WebUI where the URLs are displayed along with the metadata. 
Notifying the autocomplete provider would possibly let us update the nearby URL
count if new URLs are discovered while the omnibox suggestions list is visible. 
Notifying the WebUI would let us append new discoveries to the bottom of the
list.  Neither of these are required for our MVP so I removed the notification
logic for this CL.

[mattreynolds, 2016-08-02 00:41:39]

If I add the PhysicalWebDataSource instance to BrowserProcess is it still
appropriate to create the instance and start the discovery service in
ios_chrome_main_parts.mm?  My guess is no since there are currently no other
references to BrowserProcess in that file.  That means I'll need to find an
appropriate place in the BrowserMain initialization sequence.

Previously I was initializing in IOSChromeMainParts::PreMainMessageLoopRun.  In
chrome_browser_main.cc there's ChromeBrowserMainParts::PreMainMessageLoopRun
which calls ChromeBrowserMainParts::PreMainMessageLoopRunImpl.  That seems like
a good place to put Physical Web initialization since it runs very late in the
init sequence.  WDYT?

[sdefresne, 2016-08-02 22:23:37]

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:21: static
PhysicalWebDataSource* GetInstance();
I agree with blundell, and I think we should avoid having a singleton in the
component.

From what I see in the code, the fact that this object is a singleton is an
embedder details (the goal is to avoid duplicating network/bluetooth requests).
So what I would do is keep this interface but remove the
GetInstance/SetInstance. Instead, I would change the client code that expect to
access this object to take the instance to use as a parameter (instead of
accessing the singleton) and leave how the instance is fetched to the embedder
code.

For src/ios/chrome/*_extension it can be a field of the main application
delegate, for src/ios/chrome/browser it can be attached to the
ApplicationContext, for src/chrome/browser it could be tied to BrowserProcess
(it is a //chrome level object), ...

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:35: virtual
std::unique_ptr<base::ListValue> GetMetadata() = 0;
When should this method be called? Should client code poll and call it
periodically?

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:39: virtual bool
HasUnresolvedDiscoveries() = 0;
When should this method be called? Should client code poll and call it
periodically?

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/create_physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/create_physical_web_data_source.h:15: // This
helper function allows the construction of
I would remove this paragraph as it is an implementation details that may become
obsolete someday.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/create_physical_web_data_source.mm (right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/create_physical_web_data_source.mm:11: return
base::WrapUnique(new IOSChromePhysicalWebDataSource());
return base::MakeUnique<IOSChromePhysicalWebDataSource>();

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm
(right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:20:
setNetworkRequestEnabled:static_cast<BOOL>(network_request_enabled)];
There is no need to cast a bool to a BOOL. I think passing the value as is
should work. If this is not the case, then I would recommend to use
initialisation syntax instead, so use one of the two following syntaxes:

[scanner_ setNetworkRequestEnabled:network_request_enabled];
[scanner_ setNetworkRequestEnabled:BOOL{network_request_enabled}];

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:31: if
(metadata.get() == NULL) {
if (!metadata)
  metadata = base::MakeUnique<base::ListValue>();

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:41:
IOSChromePhysicalWebDataSource::IOSChromePhysicalWebDataSource() {}
Please keep method in the same order as in the header file.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:157: const char*
scannedUrl = [[[device requestURL] absoluteString] UTF8String];
Use base::SysNSStringToUTF8 (it returns std::string, but since
base::DictionaryValue::SetString takes its second parameter as a std::string,
there is no point avoiding the std::string here). It deal correctly with nil or
empty NSString.

Here and for the other values too.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:163: // Provide defaults
for optional fields.
You can remove this if using base::SysNSStringToUTF8.

[mattreynolds, 2016-08-04 00:56:13]

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:21: static
PhysicalWebDataSource* GetInstance();
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> I agree with blundell, and I think we should avoid having a singleton in the
> component.
> 
> From what I see in the code, the fact that this object is a singleton is an
> embedder details (the goal is to avoid duplicating network/bluetooth
requests).
> So what I would do is keep this interface but remove the
> GetInstance/SetInstance. Instead, I would change the client code that expect
to
> access this object to take the instance to use as a parameter (instead of
> accessing the singleton) and leave how the instance is fetched to the embedder
> code.
> 
> For src/ios/chrome/*_extension it can be a field of the main application
> delegate, for src/ios/chrome/browser it can be attached to the
> ApplicationContext, for src/chrome/browser it could be tied to BrowserProcess
> (it is a //chrome level object), ...

Okay, I think I understand now.  BrowserProcess is used in all versions of
Chrome except Bling, and on Bling the equivalent is the ApplicationContext.  So
I'll add a GetPhysicalWebDataSource() to ApplicationContext and access the data
source instance from there when embedding in the autocomplete component.  When
we bring the feature to another platform, we'll need to add an equivalent method
to the BrowserProcess.

The embedder will need to wrap the ApplicationContext call in #ifdef (OS_IOS)
and return null (or maybe a NullPhysicalWebDataSource) in the #else case.

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:35: virtual
std::unique_ptr<base::ListValue> GetMetadata() = 0;
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> When should this method be called? Should client code poll and call it
> periodically?

Updated the comment.

In the current design there is no need for the client to poll since we only get
the metadata once. If it becomes necessary for any clients to receive
synchronous notifications for metadata updates, we'll add a mechanism to
register for metadata changes.

https://codereview.chromium.org/2113473002/diff/120001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:39: virtual bool
HasUnresolvedDiscoveries() = 0;
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> When should this method be called? Should client code poll and call it
> periodically?

Updated the comment, and as with GetMetadata there's currently no need to poll.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/create_physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/create_physical_web_data_source.h:15: // This
helper function allows the construction of
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> I would remove this paragraph as it is an implementation details that may
become
> obsolete someday.

Done.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/create_physical_web_data_source.mm (right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/create_physical_web_data_source.mm:11: return
base::WrapUnique(new IOSChromePhysicalWebDataSource());
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> return base::MakeUnique<IOSChromePhysicalWebDataSource>();

Done.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm
(right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:20:
setNetworkRequestEnabled:static_cast<BOOL>(network_request_enabled)];
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> There is no need to cast a bool to a BOOL. I think passing the value as is
> should work. If this is not the case, then I would recommend to use
> initialisation syntax instead, so use one of the two following syntaxes:
> 
> [scanner_ setNetworkRequestEnabled:network_request_enabled];
> [scanner_ setNetworkRequestEnabled:BOOL{network_request_enabled}];

Done.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:31: if
(metadata.get() == NULL) {
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> if (!metadata)
>   metadata = base::MakeUnique<base::ListValue>();

Done.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:41:
IOSChromePhysicalWebDataSource::IOSChromePhysicalWebDataSource() {}
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> Please keep method in the same order as in the header file.

Done.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:157: const char*
scannedUrl = [[[device requestURL] absoluteString] UTF8String];
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> Use base::SysNSStringToUTF8 (it returns std::string, but since
> base::DictionaryValue::SetString takes its second parameter as a std::string,
> there is no point avoiding the std::string here). It deal correctly with nil
or
> empty NSString.
> 
> Here and for the other values too.

Done.

https://codereview.chromium.org/2113473002/diff/120001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:163: // Provide defaults
for optional fields.
On 2016/08/02 22:23:36, sdefresne (travelling - slow) wrote:
> You can remove this if using base::SysNSStringToUTF8.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:328: return
[[PhysicalWebDevice alloc] initWithURL:url
Bug fix to prevent us from crashing when beacons broadcast invalid URLs

[sdefresne, 2016-08-04 19:23:48]

This LG. I've added some comments on local changes I think are worth doing.

https://codereview.chromium.org/2113473002/diff/140001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/140001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:10: #include
"base/values.h"
nit: I think you can remove this include and instead forward-declare
base::ListValue.

https://codereview.chromium.org/2113473002/diff/140001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:15: virtual
~PhysicalWebDataSource() {};
Remove the trailing semi-colon here.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/app...
File ios/chrome/browser/application_context_impl.cc (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/app...
ios/chrome/browser/application_context_impl.cc:307:
DCHECK(physical_web_data_source_);
Move this DCHECK into the if block.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_main_parts.mm (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_main_parts.mm:37: #import
"ios/chrome/browser/experimental_flags.h"
Is this needed?

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:9: #include
"base/values.h"
nit: I think you can remove this include and instead forward-declare
base::ListValue.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:12: #import
"ios/chrome/common/physical_web/physical_web_scanner.h"
nit: I think you can remove this include and instead forward-declare @class
PhysicalWebScanner.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm
(right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:37:
std::unique_ptr<base::ListValue> metadata = [scanner_ metadata];
It look like the current implementation of -[PhysicalWebScanner metadata] always
returns a non-nil object, so the check below is redundant. Can we instead change
it to DCHECK(metadata)? This would require including base/logging.h.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.h (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:12: #include
"base/values.h"
nit: I think we can forward-declare base::ListValue here and remove this
include.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:48: // Returns the
metadata for all resolved physical web URLs.
Can you say that the method always returns a non null value?

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:152:
std::unique_ptr<base::ListValue> metadataList(new base::ListValue());
auto metadataList = base::MakeUniquePtr<base::ListValue>();

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:165:
base::DictionaryValue* metadataItem = new base::DictionaryValue();
auto metadataItem = base::MakeUnique<base::DictionaryValue>();
metadataItem->SetString(...
metadataList->Append(std::move(metadataItem));

[mattreynolds, 2016-08-04 21:14:35]

https://codereview.chromium.org/2113473002/diff/140001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2113473002/diff/140001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:10: #include
"base/values.h"
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> nit: I think you can remove this include and instead forward-declare
> base::ListValue.

Done.

https://codereview.chromium.org/2113473002/diff/140001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:15: virtual
~PhysicalWebDataSource() {};
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> Remove the trailing semi-colon here.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/app...
File ios/chrome/browser/application_context_impl.cc (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/app...
ios/chrome/browser/application_context_impl.cc:307:
DCHECK(physical_web_data_source_);
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> Move this DCHECK into the if block.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_main_parts.mm (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_main_parts.mm:37: #import
"ios/chrome/browser/experimental_flags.h"
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> Is this needed?

Oops, removed.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:9: #include
"base/values.h"
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> nit: I think you can remove this include and instead forward-declare
> base::ListValue.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:12: #import
"ios/chrome/common/physical_web/physical_web_scanner.h"
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> nit: I think you can remove this include and instead forward-declare @class
> PhysicalWebScanner.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm
(right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm:37:
std::unique_ptr<base::ListValue> metadata = [scanner_ metadata];
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> It look like the current implementation of -[PhysicalWebScanner metadata]
always
> returns a non-nil object, so the check below is redundant. Can we instead
change
> it to DCHECK(metadata)? This would require including base/logging.h.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.h (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:12: #include
"base/values.h"
On 2016/08/04 19:23:48, sdefresne (travelling - slow) wrote:
> nit: I think we can forward-declare base::ListValue here and remove this
> include.

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:48: // Returns the
metadata for all resolved physical web URLs.
On 2016/08/04 19:23:47, sdefresne (travelling - slow) wrote:
> Can you say that the method always returns a non null value?

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:152:
std::unique_ptr<base::ListValue> metadataList(new base::ListValue());
On 2016/08/04 19:23:48, sdefresne (travelling - slow) wrote:
> auto metadataList = base::MakeUniquePtr<base::ListValue>();

Done.

https://codereview.chromium.org/2113473002/diff/140001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:165:
base::DictionaryValue* metadataItem = new base::DictionaryValue();
On 2016/08/04 19:23:48, sdefresne (travelling - slow) wrote:
> auto metadataItem = base::MakeUnique<base::DictionaryValue>();
> metadataItem->SetString(...
> metadataList->Append(std::move(metadataItem));

Done.

[sdefresne, 2016-08-04 21:26:00]

lgtm

https://codereview.chromium.org/2113473002/diff/160001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2113473002/diff/160001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:157: for (int i = 0; i <
deviceCount; ++i) {
nit: you could use Objective-C for loop syntax here I guess

for (PhysicalWebDevice* device in [self devices]) {
  ...
}

[mattreynolds, 2016-08-08 21:58:12]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-08-08 21:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@google.com, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2113473002/#ps180001
(title: "obj-c loop syntax")

[commit-bot: I haz the power, 2016-08-08 21:58:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 22:02:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 22:02:38]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattreynolds, 2016-08-08 22:18:26]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-08-08 22:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, olivierrobin@google.com
Link to the patchset: https://codereview.chromium.org/2113473002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-08 22:19:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 22:21:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 22:21:41]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mattreynolds, 2016-08-09 17:50:54]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-08-09 17:50:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, olivierrobin@google.com
Link to the patchset: https://codereview.chromium.org/2113473002/#ps220001
(title: "rebase harder")

[commit-bot: I haz the power, 2016-08-09 17:51:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 18:03:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 18:03:03]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mattreynolds, 2016-08-09 18:49:16]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-08-09 18:49:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, olivierrobin@google.com
Link to the patchset: https://codereview.chromium.org/2113473002/#ps240001
(title: "fix TestingApplicationContext")

[commit-bot: I haz the power, 2016-08-09 18:49:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 19:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 19:00:11]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[mattreynolds, 2016-08-09 19:10:56]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-08-09 19:10:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, olivierrobin@google.com
Link to the patchset: https://codereview.chromium.org/2113473002/#ps260001
(title: "typo")

[commit-bot: I haz the power, 2016-08-09 19:11:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 21:31:22]

Description was changed from

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=630769
==========

to

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=630769
==========

[commit-bot: I haz the power, 2016-08-09 21:31:23]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-09 21:33:34]

Description was changed from

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=630769
==========

to

==========
Add a Physical Web data source

Add a helper class for accessing nearby URL metadata and controlling
Physical Web scanning and URL resolution from C++. This metadata will be used
by an omnibox autocomplete provider which should not depend on ios/chrome.

BUG=630769

Committed: https://crrev.com/c565df2c11f431f8711db353e7ec00d48ab9901c
Cr-Commit-Position: refs/heads/master@{#410829}
==========

[commit-bot: I haz the power, 2016-08-09 21:33:35]

Patchset 14 (id:??) landed as
https://crrev.com/c565df2c11f431f8711db353e7ec00d48ab9901c
Cr-Commit-Position: refs/heads/master@{#410829}