Cleanup PushMessagingAppIdentifier

Cleanup PushMessagingAppIdentifier

In particular, the IsValid method had a confusing dual purpose - it was
used both to sanity check the inputs, and to check whether the instances
returned by the Get methods were null (i.e. Get failed). It has been
split into is_null() and DCheckValid(). Note that having DCheckValid as
a method also gives better stacktraces than the old DCHECK(IsValid())
style, since stacktraces will indicate which check failed.

BUG=458592
Committed: https://crrev.com/db9cb0e5848645420e439277974b5aa379d83553
Cr-Commit-Position: refs/heads/master@{#329406}

[johnme, 2015-05-08 17:02:42]

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

[Peter Beverloo, 2015-05-11 16:54:08]

lgtm % comments

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_app_identifier.h (right):

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:24: // Type used
to identify a web app (specifically, a Service Worker registration)
+1 to the addition, but I would prefer to remove the "web app" terminology
completely if we can. Michael's been working to do the same with the W3C API.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:25: // from a Push
API perspective. These can be persisted to disk, in a 1:1 mapping
"to disk" -> "to prefs" to match the other changes.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:37: // Looks up an
app identifier by app_id. Will be null if not found.
What do you think about changing null -> is_null() in comments? If I read it,
I'd expect it to return a nullptr, while it returns an object.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:59: // Returns
true if this object has not been initialized.
The object *will* have been initialized. What about:

// Returns true if this identifier does not represent an app.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:62: // Perform a
DCHECK that the origin, app_id and service_worker_registration_id
nit: Make this private (per the changes in PushMessagingServiceImpl). I would
personally not mention the fields it checks, but rather just say it validates
that everything about this app identifier has valid values.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:307:
app_identifier.DCheckValid();
Remove this line, PushMessagingAppIdentifier::Generate() already does this
check.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:365:
app_identifier.DCheckValid();
dito

[johnme, 2015-05-12 13:21:51]

Addressed peter's comments (thanks!), and also added DCHECKS to getters.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_app_identifier.h (right):

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:24: // Type used
to identify a web app (specifically, a Service Worker registration)
On 2015/05/11 16:54:08, Peter Beverloo wrote:
> +1 to the addition, but I would prefer to remove the "web app" terminology
> completely if we can. Michael's been working to do the same with the W3C API.

Done.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:25: // from a Push
API perspective. These can be persisted to disk, in a 1:1 mapping
On 2015/05/11 16:54:07, Peter Beverloo wrote:
> "to disk" -> "to prefs" to match the other changes.

Done.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:37: // Looks up an
app identifier by app_id. Will be null if not found.
On 2015/05/11 16:54:08, Peter Beverloo wrote:
> What do you think about changing null -> is_null() in comments? If I read it,
> I'd expect it to return a nullptr, while it returns an object.

Done.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:59: // Returns
true if this object has not been initialized.
On 2015/05/11 16:54:07, Peter Beverloo wrote:
> The object *will* have been initialized. What about:
> 
> // Returns true if this identifier does not represent an app.

Done ("Returns true if this identifier does not represent an app (i.e. this was
returned by a failed call to Get).")

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:62: // Perform a
DCHECK that the origin, app_id and service_worker_registration_id
On 2015/05/11 16:54:08, Peter Beverloo wrote:
> nit: Make this private (per the changes in PushMessagingServiceImpl). I would
> personally not mention the fields it checks, but rather just say it validates
> that everything about this app identifier has valid values.

Done.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:307:
app_identifier.DCheckValid();
On 2015/05/11 16:54:08, Peter Beverloo wrote:
> Remove this line, PushMessagingAppIdentifier::Generate() already does this
> check.

Done.

https://codereview.chromium.org/1131303002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:365:
app_identifier.DCheckValid();
On 2015/05/11 16:54:08, Peter Beverloo wrote:
> dito

Done.

[johnme, 2015-05-12 13:25:03]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-05-12 13:25:03]

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/1131303002/#ps60001
(title: "Undo accidental test change")

[commit-bot: I haz the power, 2015-05-12 13:25:43]

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

[commit-bot: I haz the power, 2015-05-12 14:55:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-12 14:56:43]

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