Issue 1929673004: Handle intent-based navigations from gws.

Bryan McQuade

Description was changed from ========== Handle intent-based navigations from gws. BUG=607330 ========== to ========== Handle ...

4 years, 7 months ago (2016-04-28 01:51:07 UTC) #1

Bryan McQuade

bmcquade@chromium.org changed reviewers: + csharrison@chromium.org

4 years, 7 months ago (2016-04-28 02:12:03 UTC) #2

Charlie Harrison

Looks good. I just have some nits. https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc#newcode249 chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:249: // Google ...

4 years, 7 months ago (2016-04-28 19:05:32 UTC) #4

Bryan McQuade

Thanks! Addressed comments. PTAL. https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc#newcode249 chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:249: // Google search result redirects ...

4 years, 7 months ago (2016-04-29 14:09:56 UTC) #5

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:249:
// Google search result redirects are differentiated from other general
On 2016/04/28 at 19:05:32, csharrison wrote:
> Move these comments above the conditional.

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:250:
// google redirects by 'source=web' inthe query string.
On 2016/04/28 at 19:05:31, csharrison wrote:
> Add a space between "in" and "the".

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:252:
return true;
On 2016/04/28 at 19:05:32, csharrison wrote:
> Can you add braces here? It's hard to read multi-line conditionals with no
braces.

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:256:
if (url.path_piece() == "/searchurl/r.html" && url.has_ref())
On 2016/04/28 at 19:05:32, csharrison wrote:
> Can you comment why you need "has_ref" and why it's sufficient to just query
existence?

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:258:
return false;
On 2016/04/28 at 19:05:31, csharrison wrote:
> Suggestion: "return url.path_piece() == "/searchurl/r.html" && url.has_ref();"

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:361:
// navigation.
On 2016/04/28 at 19:05:32, csharrison wrote:
> Good catch. Do you mind pulling the comments out of the function args?

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:411:
// provisional url if the navigation didn't commit.
On 2016/04/28 at 19:05:32, csharrison wrote:
> Sorry I know I've been bad about this myself but could you try to remove the
"we"s in the comments? You don't need to go crazy fixing the entire file, but
let's try to be better about this style.

Is this expectation stated as expected style somewhere with an explanation for
why we don't like to use we? 'we' is used in comments all over the codebase.
I've had another reviewer ask that we not use 'we' in comments but it makes no
sense to me why that's the case. IMO comments should be written to clearly
convey information. Choice of words, in particular use of we, seems like an
arbitrary thing to discourage. Happy to remove it if there's a good reason but
if this is just one or two peoples' personal preference that's creeping into
other reviewers requests, I'm disinclined to make the change just to satisfy
that person's personal preference.

I don't see anything about this in the style guide but I do see lots of example
comments in the style guide that include the word 'we' so this seems to be
implicitly supported as far as I can tell. See e.g.
https://google.github.io/styleguide/cppguide.html#Variable_Comments

Sorry for the lengthy reply but I want to either understand why not using 'we'
is important else discourage us from requesting this as part of code reviews.

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:413:
if (provisional_url_has_search_hostname_)
On 2016/04/28 at 19:05:32, csharrison wrote:
> Can you combine these two conditions?

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:420:
if (IsGoogleSearchHostname(committed_url.host_piece()))
On 2016/04/28 at 19:05:32, csharrison wrote:
> ditto.

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:429:
// the navigation came from a redirect url.  TODO(csharrison): Use this signal
On 2016/04/28 at 19:05:32, csharrison wrote:
> Remove the extra space here (or use a newline).

Done

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:72:
bool provisional_url_is_non_http_or_https_ = false;
On 2016/04/28 at 19:05:32, csharrison wrote:
> Can store this bool in the positive form (i.e. is_http_or_https)?

My motivation for non is that I really want to make sure we explicitly changed
it from its default value as part of testing it. I wanted to avoid any risk of
returning false in ShouldLogMetrics because the value never got set and was just
its default. I know that should never happen since it's set in OnStart which is
called unconditionally but I still felt a bit uncomfortable about taking action
based on the value of a default that didn't get set to some expected value.

3 ways I can see of addressing this:
1. leave it as is
2. switch to positive form wrapped in Optional (base::Optional<bool>)
3. store the full provisional URL rather than booleans

I'll leave it as is for now. Let me know which you prefer.

Charlie Harrison

LGTM https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc#newcode411 chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:411: // provisional url if the navigation didn't commit. ...

4 years, 7 months ago (2016-04-29 14:36:55 UTC) #6

LGTM

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:411:
// provisional url if the navigation didn't commit.
On 2016/04/29 14:09:55, Bryan McQuade wrote:
> On 2016/04/28 at 19:05:32, csharrison wrote:
> > Sorry I know I've been bad about this myself but could you try to remove the
> "we"s in the comments? You don't need to go crazy fixing the entire file, but
> let's try to be better about this style.
> 
> Is this expectation stated as expected style somewhere with an explanation for
> why we don't like to use we? 'we' is used in comments all over the codebase.
> I've had another reviewer ask that we not use 'we' in comments but it makes no
> sense to me why that's the case. IMO comments should be written to clearly
> convey information. Choice of words, in particular use of we, seems like an
> arbitrary thing to discourage. Happy to remove it if there's a good reason but
> if this is just one or two peoples' personal preference that's creeping into
> other reviewers requests, I'm disinclined to make the change just to satisfy
> that person's personal preference.
> 
> I don't see anything about this in the style guide but I do see lots of
example
> comments in the style guide that include the word 'we' so this seems to be
> implicitly supported as far as I can tell. See e.g.
> https://google.github.io/styleguide/cppguide.html#Variable_Comments
> 
> Sorry for the lengthy reply but I want to either understand why not using 'we'
> is important else discourage us from requesting this as part of code reviews.

Discussed this offline, and I couldn't find any rules either so my take is this
is OWNERS style preference. Feel free to leave unchanged, the comment is clear
enough to me.

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/1929673004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:72:
bool provisional_url_is_non_http_or_https_ = false;
On 2016/04/29 14:09:56, Bryan McQuade wrote:
> On 2016/04/28 at 19:05:32, csharrison wrote:
> > Can store this bool in the positive form (i.e. is_http_or_https)?
> 
> My motivation for non is that I really want to make sure we explicitly changed
> it from its default value as part of testing it. I wanted to avoid any risk of
> returning false in ShouldLogMetrics because the value never got set and was
just
> its default. I know that should never happen since it's set in OnStart which
is
> called unconditionally but I still felt a bit uncomfortable about taking
action
> based on the value of a default that didn't get set to some expected value.
> 
> 3 ways I can see of addressing this:
> 1. leave it as is
> 2. switch to positive form wrapped in Optional (base::Optional<bool>)
> 3. store the full provisional URL rather than booleans
> 
> I'll leave it as is for now. Let me know which you prefer.

You could also set the default to true if you wanted to. Up to you, your
reasoning makes sense.

commit-bot: I haz the power

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

4 years, 7 months ago (2016-04-29 15:06:48 UTC) #8

commit-bot: I haz the power

4 years, 7 months ago (2016-04-30 17:25:56 UTC) #10

Message was sent while issue was closed.

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

commit-bot: I haz the power

4 years, 7 months ago (2016-04-30 18:00:42 UTC) #11

Message was sent while issue was closed.

Description was changed from

==========
Handle intent-based navigations from gws.

We now explicitly ignore provisional navigations for
non HTTP/HTTPS schemes.

We also consider navigations from the intent redirector
to be from search.

BUG=607330
==========

to

==========
Handle intent-based navigations from gws.

We now explicitly ignore provisional navigations for
non HTTP/HTTPS schemes.

We also consider navigations from the intent redirector
to be from search.

BUG=607330

Committed: https://crrev.com/f91fa02d7d146fb0dc4d17b25aef3128f644a439
Cr-Commit-Position: refs/heads/master@{#390652}
==========

Issue 1929673004: Handle intent-based navigations from gws. (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : address comments #

Messages