Push API: Include origin in generated app_ids

Push API: Include origin in generated app_ids

PushMessagingAppIdentifier currently generates app_id strings of the form:
"wp:9CC55CCE-B8F9-4092-A364-3B0F73A3AB5F".

This patch changes it to generate app_id strings of the form:
"wp:https://foo.example.com:8443/#9CC55CCE-B8F9-4092-A364-3B0F73A3AB5F"

BUG=458592, 461867
Committed: https://crrev.com/14defc6866693aa3dfceed796cd94d788e9f3122
Cr-Commit-Position: refs/heads/master@{#329851}

[johnme, 2015-05-13 11:38:40]

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

[Peter Beverloo, 2015-05-13 13:04:14]

Thanks for the patch. Hereby a few comments, and some questions.

  (1) What's the plan for stopping to support legacy app ids? Is this code going
to be here indefinitely?

  (2) Is there some other serialization mechanism we could be using? All the
string math has a very significant (negative) impact on the complexity of this
class.

This CL should not land without proper unit tests for both new and old app_ids.

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

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:27: return
app_id.size() == strlen(kPushMessagingAppIdentifierPrefix)
I'd slightly prefer to use IsValidGUID for this.

bool IsLegacyAppId(const std::string& app_id) {
  return base::IsValidGUID(
      app_id.substr(strlen(kPushMessagingAppIdentifierPrefix)));
}

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:31: GURL
GetOriginFromModernAppId(const std::string& app_id) {
s/Modern//. The omission from "Legacy" in the name implies that it's current.

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:39: bool
GetOriginAndSWRFromLegacyMapValue(
nit: While the variable in PushMessagingAppIdentifier::Get() is called
map_value, that makes no sense in scope of this function. Could we call it
PrefKey or similar?

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:72: std::string
app_id = kPushMessagingAppIdentifierPrefix + origin.spec()
You could consider using base::StringPrintF here, which is how several other
features handle generation of such strings.

  std::string app_id = base::StringPrintf(
      "%s%s%c%s",
      kPushMessagingAppIdentifierPrefix,
      origin.spec(),
      kSeparator,
      guid);

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:72: std::string
app_id = kPushMessagingAppIdentifierPrefix + origin.spec()
How certain are we that |origin|==|origin.GetOrigin()| here?

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:210:
std::string::npos)));
std::string::npos is the default argument value, you can omit it.

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

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:42: static
PushMessagingAppIdentifier Get(Profile* profile,
nit: It's not obvious what ::Get() means when reading the code.

Have you considered renaming this one (:42) to FindByAppId(), and renaming (:47)
to FindByAppInfo() (or similar)?

[johnme, 2015-05-13 16:18:09]

Addressed peter's inline review comments, but haven't yet addressed the bigger
questions asked.

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

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:27: return
app_id.size() == strlen(kPushMessagingAppIdentifierPrefix)
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> I'd slightly prefer to use IsValidGUID for this.
> 
> bool IsLegacyAppId(const std::string& app_id) {
>   return base::IsValidGUID(
>       app_id.substr(strlen(kPushMessagingAppIdentifierPrefix)));
> }

How about:

  if (app_id.size() == strlen(kPushMessagingAppIdentifierPrefix)
                     + kGuidLength) {
    DCHECK(base::IsValidGUID(
        app_id.substr(strlen(kPushMessagingAppIdentifierPrefix))));
    return true;
  }
  return false;

? Happy to go with yours if you'd rather.

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:31: GURL
GetOriginFromModernAppId(const std::string& app_id) {
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> s/Modern//. The omission from "Legacy" in the name implies that it's current.

Done.

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:39: bool
GetOriginAndSWRFromLegacyMapValue(
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> nit: While the variable in PushMessagingAppIdentifier::Get() is called
> map_value, that makes no sense in scope of this function. Could we call it
> PrefKey or similar?

Done.

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:72: std::string
app_id = kPushMessagingAppIdentifierPrefix + origin.spec()
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> You could consider using base::StringPrintF here, which is how several other
> features handle generation of such strings.

Seems more legible to use + (even if minorly less efficient).

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:72: std::string
app_id = kPushMessagingAppIdentifierPrefix + origin.spec()
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> How certain are we that |origin|==|origin.GetOrigin()| here?

DCheckValid enforces this if DCHECKs are on. Happy to always use
origin.GetOrigin() though if you'd rather I take no changes when DCHECKs are
off? WDYT?

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:210:
std::string::npos)));
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> std::string::npos is the default argument value, you can omit it.

Done.

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

https://codereview.chromium.org/1141613003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_app_identifier.h:42: static
PushMessagingAppIdentifier Get(Profile* profile,
On 2015/05/13 13:04:14, Peter Beverloo wrote:
> nit: It's not obvious what ::Get() means when reading the code.
> 
> Have you considered renaming this one (:42) to FindByAppId(), and renaming
(:47)
> to FindByAppInfo() (or similar)?

Done (but FindByServiceWorker).

[johnme, 2015-05-13 17:25:58]

> (1) What's the plan for stopping to support legacy app
> ids? Is this code going to be here indefinitely?

We don't want to break existing subscriptions, so we'll probably need this for a
year or more before it'll be safe to remove.

You suggested offline that I redundantly store the origin as part of the prefs
value to simplify parsing logic. The most recent patch I've uploaded does that;
I'm not sure if the difference in code complexity is worth the duplicate storage
of origins. Thoughts?

> (2) Is there some other serialization mechanism we could be
> using? All the string math has a very significant
> (negative) impact on the complexity of this class.

Sadly no - ordinarily I could use a ListValue or similar to store multiple
values in preferences, but the main complexity here is in generating and
validating the app_id string, which must be a string.

> This CL should not land without proper unit tests for both
> new and old app_ids.

New app_ids are inherently covered. I'll add tests for old ones in the next
patch.

[Peter Beverloo, 2015-05-14 11:26:29]

lgtm

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (left):

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:48: // Workaround
crbug.com/461867 in GCM where it converts subtypes to lowercase.
Please add 461867 to the BUG= line. Should we DCHECK on casing to make sure that
we get *some* information if this goes wrong again, given that it is external
input?

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:7: #include
<string.h>
<vector>?

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:43: *origin =
GURL(parts[0]);
Do we need to validate here that the origin is valid and return FALSE if it
isn't?

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc
(right):

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:114:
original_.app_id_ = legacy_app_id;
Eeuw!

[johnme, 2015-05-14 13:18:20]

Addressed peter's nits - thanks :)

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (left):

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:48: // Workaround
crbug.com/461867 in GCM where it converts subtypes to lowercase.
On 2015/05/14 11:26:29, Peter Beverloo wrote:
> Please add 461867 to the BUG= line. Should we DCHECK on casing to make sure
that
> we get *some* information if this goes wrong again, given that it is external
> input?

Done. I factored out a DCheckAppIdValid method from DCheckValid, incorporated a
case check into that, and call it from here.

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:7: #include
<string.h>
On 2015/05/14 11:26:29, Peter Beverloo wrote:
> <vector>?

push_messaging_app_identifier.h already includes <vector>.

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:43: *origin =
GURL(parts[0]);
On 2015/05/14 11:26:29, Peter Beverloo wrote:
> Do we need to validate here that the origin is valid and return FALSE if it
> isn't?

Done.

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc
(right):

https://codereview.chromium.org/1141613003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:114:
original_.app_id_ = legacy_app_id;
On 2015/05/14 11:26:29, Peter Beverloo wrote:
> Eeuw!

Acknowledged.

[johnme, 2015-05-14 14:07:27]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-05-14 14:07:27]

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/1141613003/#ps120001
(title: "Simplify DCHECKs")

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

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

[commit-bot: I haz the power, 2015-05-14 15:04:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-14 15:05:46]

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