Implement Physical Web listener registration

Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490
Committed: https://crrev.com/7b30c99f5ea9b24f352c720280ee78edebca37cf
Cr-Commit-Position: refs/heads/master@{#420682}

[hayesjordan, 2016-09-20 19:22:54]

Description was changed from

==========
Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490
==========

to

==========
Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490
==========

[hayesjordan, 2016-09-20 19:22:54]

hayesjordan@google.com changed reviewers:
+ cco3@chromium.org, mattreynolds@chromium.org

[cco3, 2016-09-20 22:11:34]

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source_impl.cc
(right):

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl.cc:11: //
observer_list_* = new ObserverList<PhysicalWebListener>();
Is this intentional?

https://codereview.chromium.org/2349483002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2349483002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:21: :
public PhysicalWebDataSourceWithListener {
Update

[hayesjordan, 2016-09-20 22:21:47]

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source_impl.cc
(right):

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl.cc:11: //
observer_list_* = new ObserverList<PhysicalWebListener>();
On 2016/09/20 22:11:33, cco3 wrote:
> Is this intentional?

No, I forgot to delete it.

https://codereview.chromium.org/2349483002/diff/20001/ios/chrome/common/physi...
File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2349483002/diff/20001/ios/chrome/common/physi...
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:21: :
public PhysicalWebDataSourceWithListener {
On 2016/09/20 22:11:33, cco3 wrote:
> Update

Done.

[cco3, 2016-09-20 22:22:45]

On 2016/09/20 22:21:47, hayesjordan wrote:
>
https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
> File components/physical_web/data_source/physical_web_data_source_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
> components/physical_web/data_source/physical_web_data_source_impl.cc:11: //
> observer_list_* = new ObserverList<PhysicalWebListener>();
> On 2016/09/20 22:11:33, cco3 wrote:
> > Is this intentional?
> 
> No, I forgot to delete it.
> 
>
https://codereview.chromium.org/2349483002/diff/20001/ios/chrome/common/physi...
> File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h
> (right):
> 
>
https://codereview.chromium.org/2349483002/diff/20001/ios/chrome/common/physi...
> ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:21: :
> public PhysicalWebDataSourceWithListener {
> On 2016/09/20 22:11:33, cco3 wrote:
> > Update
> 
> Done.

LGTM, but Matt should look at it.

[mattreynolds, 2016-09-20 22:49:34]

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source_impl.h
(right):

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl.h:23: //
Notify all register listeners, that a URL has been found.
register -> registered

Also, the comma is unnecessary

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
File
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc
(right):

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:68:
void TestPhysicalWebListener::OnFound(const std::string& url) {
These methods are pretty short, I think it makes sense to move them into the
class definition

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:84:
void TestPhysicalWebListener::ClearNotified() {
Why not initialize these in the TestPhysicalWebListener constructor?

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:88:
last_event_url_ = "";
last_event_url_.clear();

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:119:
std::string url = "https://www.google.com";
Let's pull out this constant and share it between tests.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:152:
EXPECT_EQ("", listener_.LastEventUrl());
Consider changing to EXPECT_TRUE(listener_.LastEventUrl().empty())

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:165:
TEST_F(PhysicalWebDataSourceImplTest, OnDistanceChangedNotregistered) {
OnDistanceChangedNotregistered -> OnDistanceChangedNotRegistered

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:174:
}
Add a comment for the end of the namespace, eg:

}  // namespace physical_web

[hayesjordan, 2016-09-21 20:47:44]

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source_impl.h
(right):

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl.h:23: //
Notify all register listeners, that a URL has been found.
On 2016/09/20 22:49:33, mattreynolds wrote:
> register -> registered
> 
> Also, the comma is unnecessary

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
File
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc
(right):

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:68:
void TestPhysicalWebListener::OnFound(const std::string& url) {
On 2016/09/20 22:49:33, mattreynolds wrote:
> These methods are pretty short, I think it makes sense to move them into the
> class definition

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:84:
void TestPhysicalWebListener::ClearNotified() {
On 2016/09/20 22:49:33, mattreynolds wrote:
> Why not initialize these in the TestPhysicalWebListener constructor?

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:88:
last_event_url_ = "";
On 2016/09/20 22:49:34, mattreynolds wrote:
> last_event_url_.clear();

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:119:
std::string url = "https://www.google.com";
On 2016/09/20 22:49:33, mattreynolds wrote:
> Let's pull out this constant and share it between tests.

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:152:
EXPECT_EQ("", listener_.LastEventUrl());
On 2016/09/20 22:49:33, mattreynolds wrote:
> Consider changing to EXPECT_TRUE(listener_.LastEventUrl().empty())

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:165:
TEST_F(PhysicalWebDataSourceImplTest, OnDistanceChangedNotregistered) {
On 2016/09/20 22:49:33, mattreynolds wrote:
> OnDistanceChangedNotregistered -> OnDistanceChangedNotRegistered

Done.

https://codereview.chromium.org/2349483002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:174:
}
On 2016/09/20 22:49:33, mattreynolds wrote:
> Add a comment for the end of the namespace, eg:
> 
> }  // namespace physical_web

Done.

[mattreynolds, 2016-09-21 22:01:34]

https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
File
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc
(right):

https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:14:
std::string url = "https://www.google.com";
Declare this as a char array, make it const, and rename it so it starts with a
"k" to reflect the constness. eg:

const char kUrl[] = "https://www.google.com";

(It should be fine to pass a const char[] to functions that expect a const
std::string&)

https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:48:
last_event_url_.clear();
You can use member initialization here

TestPhysicalWebListener()
  : on_found_notified_(false),
    on_lost_notified_(false), ...

Also, you shouldn't need to initialize last_event_url_ since std::string
defaults to the empty string

[hayesjordan, 2016-09-21 22:25:07]

https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
File
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc
(right):

https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:14:
std::string url = "https://www.google.com";
On 2016/09/21 22:01:34, mattreynolds wrote:
> Declare this as a char array, make it const, and rename it so it starts with a
> "k" to reflect the constness. eg:
> 
> const char kUrl[] = "https://www.google.com";
> 
> (It should be fine to pass a const char[] to functions that expect a const
> std::string&)

Done.

https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:48:
last_event_url_.clear();
On 2016/09/21 22:01:34, mattreynolds wrote:
> You can use member initialization here
> 
> TestPhysicalWebListener()
>   : on_found_notified_(false),
>     on_lost_notified_(false), ...
> 
> Also, you shouldn't need to initialize last_event_url_ since std::string
> defaults to the empty string

Done.

[mattreynolds, 2016-09-22 00:50:48]

On 2016/09/21 22:25:07, hayesjordan wrote:
>
https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
> File
> components/physical_web/data_source/physical_web_data_source_impl_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
>
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:14:
> std::string url = "https://www.google.com";
> On 2016/09/21 22:01:34, mattreynolds wrote:
> > Declare this as a char array, make it const, and rename it so it starts with
a
> > "k" to reflect the constness. eg:
> > 
> > const char kUrl[] = "https://www.google.com";
> > 
> > (It should be fine to pass a const char[] to functions that expect a const
> > std::string&)
> 
> Done.
> 
>
https://codereview.chromium.org/2349483002/diff/60001/components/physical_web...
>
components/physical_web/data_source/physical_web_data_source_impl_unittest.cc:48:
> last_event_url_.clear();
> On 2016/09/21 22:01:34, mattreynolds wrote:
> > You can use member initialization here
> > 
> > TestPhysicalWebListener()
> >   : on_found_notified_(false),
> >     on_lost_notified_(false), ...
> > 
> > Also, you shouldn't need to initialize last_event_url_ since std::string
> > defaults to the empty string
> 
> Done.

lgtm

[hayesjordan, 2016-09-22 18:29:55]

hayesjordan@google.com changed reviewers:
+ blundell@chromium.org, olivierrobin@chromium.org

[hayesjordan, 2016-09-22 18:29:56]

blundell@chromium.org: Please review changes in components/BUILD.gn

olivierrobin@chromium.org: Please review changes in ios/
chrome/common/physical_web/ios_chrome_physical_web_data_source.h and
ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.mm

[blundell, 2016-09-23 07:19:50]

//components/BUILD.gn LGTM

[Olivier, 2016-09-23 08:16:37]

lgtm

[hayesjordan, 2016-09-23 16:35:19]

The CQ bit was checked by hayesjordan@google.com

[hayesjordan, 2016-09-23 16:35:19]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org
Link to the patchset: https://codereview.chromium.org/2349483002/#ps80001
(title: "Address mattreynolds comments")

[commit-bot: I haz the power, 2016-09-23 16:35:41]

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-09-23 16:53:56]

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

[commit-bot: I haz the power, 2016-09-23 16:53:57]

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...)

[hayesjordan, 2016-09-23 17:45:07]

The CQ bit was checked by hayesjordan@google.com

[hayesjordan, 2016-09-23 17:45:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattreynolds@chromium.org, cco3@chromium.org, olivierrobin@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2349483002/#ps100001
(title: "Fix build error")

[commit-bot: I haz the power, 2016-09-23 17:46: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-09-23 18:59:01]

Description was changed from

==========
Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490
==========

to

==========
Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490
==========

[commit-bot: I haz the power, 2016-09-23 18:59:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-23 19:02:30]

Description was changed from

==========
Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490
==========

to

==========
Implement Physical Web listener registration

Implement the RegisterListener, UnregisterListener, and notify methods
in an abstract class so that all Physical Web data sources can have an
the ability to push data to subscribers.

BUG=636490

Committed: https://crrev.com/7b30c99f5ea9b24f352c720280ee78edebca37cf
Cr-Commit-Position: refs/heads/master@{#420682}
==========

[commit-bot: I haz the power, 2016-09-23 19:02:31]

Patchset 6 (id:??) landed as
https://crrev.com/7b30c99f5ea9b24f352c720280ee78edebca37cf
Cr-Commit-Position: refs/heads/master@{#420682}