Allows URL queries to be passed to contet handlers.

Allows URL queries to be passed to contet handlers.

R=ppi@chromium.org
BUG=Fixes https://github.com/domokit/mojo/issues/559

Committed: https://chromium.googlesource.com/external/mojo/+/e58454c864bba30494b8d53188936d88448fe545

[ppi, 2015-11-24 10:47:21]

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/a...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/a...
shell/application_manager/application_manager.cc:4: 
Is it possible to have an application_manager_unittest for this change?

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/a...
shell/application_manager/application_manager.cc:265: bool with_query) {
Why parametrize if we don't call it with |with_query| == false?

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
File shell/application_manager/identity.h (right):

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
shell/application_manager/identity.h:17: * with different query parameters will
not have the same identity.
Can you describe the cases when we want to be "query sensitive" and when we
don't?

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
shell/application_manager/identity.h:22: bool with_query = false);
"bool query_sensitive" would probably be clearer - "with_query" suggests that
the flag is somehow about presence of the query parameters in the url.

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
shell/application_manager/identity.h:22: bool with_query = false);
Passing "with_query" in a constructor results in weird semantics. If I create
two Identities, identity A("abc:abc?query", true) and B("abc:abc", false), then
from perspective of B, B is equal to A, but not the other way round.

being query sensitive ideally would be a parameter of some comparison function,
not of an "Identity".

[qsr, 2015-11-24 12:24:32]

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/a...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/a...
shell/application_manager/application_manager.cc:265: bool with_query) {
On 2015/11/24 10:47:21, ppi wrote:
> Why parametrize if we don't call it with |with_query| == false?

We do, we have a default value for the parameter.

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
File shell/application_manager/identity.h (right):

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
shell/application_manager/identity.h:17: * with different query parameters will
not have the same identity.
On 2015/11/24 10:47:21, ppi wrote:
> Can you describe the cases when we want to be "query sensitive" and when we
> don't?

Changed identity to not care about query at all.

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
shell/application_manager/identity.h:22: bool with_query = false);
On 2015/11/24 10:47:21, ppi wrote:
> Passing "with_query" in a constructor results in weird semantics. If I create
> two Identities, identity A("abc:abc?query", true) and B("abc:abc", false),
then
> from perspective of B, B is equal to A, but not the other way round.
> 
> being query sensitive ideally would be a parameter of some comparison
function,
> not of an "Identity".

Acknowledged.

https://codereview.chromium.org/1472923002/diff/1/shell/application_manager/i...
shell/application_manager/identity.h:22: bool with_query = false);
On 2015/11/24 10:47:21, ppi wrote:
> "bool query_sensitive" would probably be clearer - "with_query" suggests that
> the flag is somehow about presence of the query parameters in the url.

Acknowledged.

[ppi, 2015-11-24 12:39:02]

https://codereview.chromium.org/1472923002/diff/20001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1472923002/diff/20001/shell/application_manag...
shell/application_manager/application_manager.cc:448:
MakeApplicationIdentity(content_handler_url, kDoNotStripQuery);
Add a comment somewhere

// If two content handler urls differ by query parameter, we want to create a
separate connection for each. See https://github.com/domokit/mojo/issues/559 .

https://codereview.chromium.org/1472923002/diff/20001/shell/application_manag...
shell/application_manager/application_manager.cc:449: auto it =
identity_to_content_handler_.find(content_handler_id);
do we need to update the map name now? E.g. identity_to_connection_.

[qsr, 2015-11-24 13:11:25]

https://codereview.chromium.org/1472923002/diff/20001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1472923002/diff/20001/shell/application_manag...
shell/application_manager/application_manager.cc:448:
MakeApplicationIdentity(content_handler_url, kDoNotStripQuery);
On 2015/11/24 12:39:02, ppi wrote:
> Add a comment somewhere
> 
> // If two content handler urls differ by query parameter, we want to create a
> separate connection for each. See https://github.com/domokit/mojo/issues/559 .

Done.

https://codereview.chromium.org/1472923002/diff/20001/shell/application_manag...
shell/application_manager/application_manager.cc:449: auto it =
identity_to_content_handler_.find(content_handler_id);
On 2015/11/24 12:39:02, ppi wrote:
> do we need to update the map name now? E.g. identity_to_connection_.

Acknowledged.

[ppi, 2015-11-24 13:15:59]

lgtm

[qsr, 2015-11-24 13:59:08]

Committed patchset #3 (id:40001) manually as
e58454c864bba30494b8d53188936d88448fe545 (presubmit successful).