GeolocationPermissionContext: only use origin from embedder url.

GeolocationPermissionContext: only use origin from embedder url.

Specifically for the bug being fixed, if the hash is not stripped, and it changes while the info bar is displayed, then an equality comparison will fail needlessly later.

Also, use WebContents::GetLastCommittedURL instead of the deprecated GetURL.

BUG=286667
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223078

[Michael van Ouwerkerk, 2013-09-12 16:21:15]

Kind of a hacky solution to get it working. Is that ok or would you like to see
a more elegant solution?

[bulach, 2013-09-12 16:28:41]

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:98: GURL
embedder = web_contents->GetLastCommittedURL();
.GetOrigin() ?

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:113: }
see above, I think we should just use .GetOrigin() (I think we do that for the
requesting_frame at some level, right?)

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:78:
controller_->OnPermissionSet(id_, requesting_frame_, embedder,
probably best to have just the controller and context sanitizing their inputs by
using GetOrigin(), wdyt?

[bulach, 2013-09-12 16:29:05]

oh, and it'd be great to have some tests too :)

[joth, 2013-09-12 16:43:13]

writing custom code to parse url parts is generally considered an antipattern in
chrome

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:75: embedder =
GURL(parts[0]);
rather than grepping for # I think you can use this to remove one component:
  embedder = embedder.ReplaceComponents(...)

Although, geolocation is keyed entirely off the origin -- path & query
components are also irrelevant. So you should be just using .GetOrigin() here or
wherever the comparisons occur.

[Michael van Ouwerkerk, 2013-09-12 16:50:40]

Much better, thanks! I'll look into tests now.

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:98: GURL
embedder = web_contents->GetLastCommittedURL();
On 2013/09/12 16:28:41, bulach wrote:
> .GetOrigin() ?

Much nicer. Done.

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:113: }
On 2013/09/12 16:28:41, bulach wrote:
> see above, I think we should just use .GetOrigin() (I think we do that for the
> requesting_frame at some level, right?)

Done.

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:75: embedder =
GURL(parts[0]);
On 2013/09/12 16:43:13, joth wrote:
> rather than grepping for # I think you can use this to remove one component:
>   embedder = embedder.ReplaceComponents(...)
> 
> Although, geolocation is keyed entirely off the origin -- path & query
> components are also irrelevant. So you should be just using .GetOrigin() here
or
> wherever the comparisons occur.

Done.

https://codereview.chromium.org/23440030/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:78:
controller_->OnPermissionSet(id_, requesting_frame_, embedder,
On 2013/09/12 16:28:41, bulach wrote:
> probably best to have just the controller and context sanitizing their inputs
by
> using GetOrigin(), wdyt?

Done.

[joth, 2013-09-12 17:12:34]

lgtm

https://codereview.chromium.org/23440030/diff/14001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23440030/diff/14001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:52:
requesting_frame_(requesting_frame),
(OT for this patch)

should this be requesting_frame_(requesting_frame.GetOrigin()) and then remove
the GetOrigin() on line 99?

[bulach, 2013-09-13 09:07:23]

lgtm perhaps we could make the chrome_geolocation_permission_context sanitize
all its inputs to be Origin based, then 
chrome_geolocation_permission_context_unittest.cc would be sufficient?

[Michael van Ouwerkerk, 2013-09-13 13:41:25]

Thanks for the quick reviews!

https://codereview.chromium.org/23440030/diff/14001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23440030/diff/14001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:52:
requesting_frame_(requesting_frame),
On 2013/09/12 17:12:34, joth wrote:
> (OT for this patch)
> 
> should this be requesting_frame_(requesting_frame.GetOrigin()) and then remove
> the GetOrigin() on line 99?

Done.

[commit-bot: I haz the power, 2013-09-13 13:41:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/23440030/24001

[commit-bot: I haz the power, 2013-09-13 17:50:31]

Change committed as 223078