Make GCMDriver::GetInstanceIDHandler protected, to avoid misuse

Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
Committed: https://crrev.com/54a3e1481e43a7e4debb0ec204a078d1e78d25c6
Cr-Commit-Position: refs/heads/master@{#380692}

[johnme, 2016-03-10 20:20:30]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[johnme, 2016-03-10 21:08:27]

Description was changed from

==========
Make GCMDriver::GetInstanceIDHandler protected, to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are currently public.

This breaks the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

To fix this, make GCMDriver::GetInstanceIDHandler protected, and grant
access only to InstanceID and tests via a friend class declaration.

BUG=589461
==========

to

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods were public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
==========

[Peter Beverloo, 2016-03-10 21:11:24]

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:215: // Supports InstanceID handling. Must
only be used by instance_id::InstanceID.
  // Supports InstanceID handling. Must only be used by the InstanceID system.

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop_unittest.cc:1260: void
GCMDriverInstanceIDTest::RemoveInstanceIDData(const std::string& app_id) {
newline!!``oneone

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:110: InstanceID(const
std::string& app_id, gcm::GCMDriver* gcm_driver);
As discussed, let's limit knowledge and have this be the gcm::InstanceIDHandler.

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:114: gcm::InstanceIDHandler*
handler_;  // Not owned.
Per the styleguide:

""Limit the use of protected to those member functions that might need to be
accessed from subclasses. Note that data members should be private.""

[johnme, 2016-03-11 15:56:04]

Addressed review comments

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:215: // Supports InstanceID handling. Must
only be used by instance_id::InstanceID.
On 2016/03/10 21:11:23, Peter Beverloo wrote:
>   // Supports InstanceID handling. Must only be used by the InstanceID system.

Done.

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop_unittest.cc:1260: void
GCMDriverInstanceIDTest::RemoveInstanceIDData(const std::string& app_id) {
On 2016/03/10 21:11:23, Peter Beverloo wrote:
> newline!!``oneone

Done.

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:110: InstanceID(const
std::string& app_id, gcm::GCMDriver* gcm_driver);
On 2016/03/10 21:11:23, Peter Beverloo wrote:
> As discussed, let's limit knowledge and have this be the
gcm::InstanceIDHandler.

Done.

https://codereview.chromium.org/1785613006/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:114: gcm::InstanceIDHandler*
handler_;  // Not owned.
On 2016/03/10 21:11:23, Peter Beverloo wrote:
> Per the styleguide:
> 
> ""Limit the use of protected to those member functions that might need to be
> accessed from subclasses. Note that data members should be private.""

Done.

[Peter Beverloo, 2016-03-11 16:10:20]

lgtm % comments

Also please s/were public/are public/ in the description.

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:59: // |gcm_driver|: driver to
access the GCM functionalities needed to support
Please update this comment.

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:113: gcm::InstanceIDHandler*
handler() { return handler_; }
Can this be const? (Like the method you're replacing.)

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:116: gcm::InstanceIDHandler*
handler_;  // Not owned.
Personally I would prefer documenting *why* it's safe to store it here. The fact
that it's not owned is implied by it not being a scoped_(ref)ptr.

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.h (left):

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.h:22: class
InstanceIDHandler;
We should continue forward-declaring InstanceIDHandler (it's used in the
constructor).

[johnme, 2016-03-11 17:30:27]

Description was changed from

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods were public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
==========

to

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
==========

[johnme, 2016-03-11 17:53:23]

Addressed nits - thanks :)

> Also please s/were public/are public/ in the description.

Done.

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:59: // |gcm_driver|: driver to
access the GCM functionalities needed to support
On 2016/03/11 16:10:19, Peter Beverloo wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:113: gcm::InstanceIDHandler*
handler() { return handler_; }
On 2016/03/11 16:10:19, Peter Beverloo wrote:
> Can this be const? (Like the method you're replacing.)

Done.

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:116: gcm::InstanceIDHandler*
handler_;  // Not owned.
On 2016/03/11 16:10:19, Peter Beverloo wrote:
> Personally I would prefer documenting *why* it's safe to store it here. The
fact
> that it's not owned is implied by it not being a scoped_(ref)ptr.

Done: "
// Owned by GCMProfileServiceFactory, which is a dependency of
// InstanceIDProfileServiceFactory, which owns this."

ditto in instance_id_driver.h

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.h (left):

https://codereview.chromium.org/1785613006/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.h:22: class
InstanceIDHandler;
On 2016/03/11 16:10:19, Peter Beverloo wrote:
> We should continue forward-declaring InstanceIDHandler (it's used in the
> constructor).

Done.

[johnme, 2016-03-11 17:53:28]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-03-11 17:53:28]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1785613006/#ps60001
(title: "Address peter's nits")

[commit-bot: I haz the power, 2016-03-11 17:54:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785613006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785613006/60001

[commit-bot: I haz the power, 2016-03-11 19:13:29]

Description was changed from

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
==========

to

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
==========

[commit-bot: I haz the power, 2016-03-11 19:13:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-11 19:14:33]

Description was changed from

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461
==========

to

==========
Rename GCMDriver::GetInstanceIDHandler to avoid misuse

GCMDriver::GetInstanceIDHandler and the GCMDriverDesktop's
implementations of the InstanceIDHandler methods are public.

This broke the encapsulation model of instance_id::InstanceID, which
assumes it can safely cache the results of InstanceIDHandler methods,
as it is the only class that can modify Instance IDs.

This patch renames GCMDriver::GetInstanceIDHandler to
GCMDriver::GetInstanceIDHandlerInternal as a warning, and adds a
comment that it should only be used by InstanceID (and tests).

BUG=589461

Committed: https://crrev.com/54a3e1481e43a7e4debb0ec204a078d1e78d25c6
Cr-Commit-Position: refs/heads/master@{#380692}
==========

[commit-bot: I haz the power, 2016-03-11 19:14:33]

Patchset 4 (id:??) landed as
https://crrev.com/54a3e1481e43a7e4debb0ec204a078d1e78d25c6
Cr-Commit-Position: refs/heads/master@{#380692}