Use URL without fragment when query recent redirect list for favicons.

Use URL without fragment when query recent redirect list for favicons.

The hash fragment is often set with Javascript when a page loads, which causes the favicon mechanism to use the URL with fragment to query the recent redirects. However, the recent redirects are typically keyed to the URL without the hash.

We need to keep to lookup for the URL with an hash, though, in case the redirect is keyed to it, which happens if the user navigated to a URL with a hash fragment but was redirected.

BUG=498618
Committed: https://crrev.com/6bef27e9bb046778a13b5cd7025208218c098e80
Cr-Commit-Position: refs/heads/master@{#335268}

[beaudoin, 2015-06-16 19:14:17]

beaudoin@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-06-16 22:06:24]

How about some test coverage?

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:2087:
GetCachedRecentRedirects(page_url, &redirects);
If the ref isn't useful when querying for redirects than shouldn't we only do
this call if there is no ref?

[chromium-reviews, 2015-06-16 22:46:54]

 On Tue, Jun 16, 2015, 6:06 PM  <sky@chromium.org> wrote:

How about some test coverage?

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
<https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...>
components/history/core/browser/history_backend.cc:2087
<https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...>
:
GetCachedRecentRedirects(page_url, &redirects);
If the ref isn't useful when querying for redirects than shouldn't we
only do this call if there is no ref?

https <https://codereview.chromium.org/1180403005/>://
<https://codereview.chromium.org/1180403005/>codereview.chromium.org
<https://codereview.chromium.org/1180403005/>/1180403005/
<https://codereview.chromium.org/1180403005/>

 That's what I tried to explain in the bug and cl description: the redirect
list could be keyed on the URL with the hash is the user navigated with a
hash and for redirected. The way this works is that the browser "remembers"
the hash (since it's not sent to the server) and reapplies it after the 3xx
redirection. The more typical case, though, is a location.setHash() when
the page loads, in which case we wrongfully get an empty redirect list.

Coverage is already very light for this function (iirc; I'm on a plane ;))
Would you be OK with tests in a follow up CL?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-06-16 23:41:08]

Is there a rush for no test coverage?

Seems to me you don't need extra_redirects. append the new items to
redirects (you'll likely need extra_redirects in the if statement of
the has_ref block.

On Tue, Jun 16, 2015 at 3:46 PM, Philippe Beaudoin <beaudoin@google.com> wrote:
> On Tue, Jun 16, 2015, 6:06 PM  <sky@chromium.org> wrote:
>
> How about some test coverage?
>
>
https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
> File components/history/core/browser/history_backend.cc (right):
>
>
https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
> components/history/core/browser/history_backend.cc:2087:
> GetCachedRecentRedirects(page_url, &redirects);
> If the ref isn't useful when querying for redirects than shouldn't we
> only do this call if there is no ref?
>
> https://codereview.chromium.org/1180403005/
>
> That's what I tried to explain in the bug and cl description: the redirect
> list could be keyed on the URL with the hash is the user navigated with a
> hash and for redirected. The way this works is that the browser "remembers"
> the hash (since it's not sent to the server) and reapplies it after the 3xx
> redirection. The more typical case, though, is a location.setHash() when the
> page loads, in which case we wrongfully get an empty redirect list.
>
> Coverage is already very light for this function (iirc; I'm on a plane ;))
> Would you be OK with tests in a follow up CL?
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-06-17 16:07:30]

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:2104: for
(RedirectList::const_iterator i(extra_redirects.begin());
This is the part I dislike about the current patch and is why I prefer copying
to extra_redirects. That said, I'm ok with what you have if you move 2100-2103
into a common function that both places call.

[beaudoin, 2015-06-17 16:51:27]

PTAL.

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:2087:
GetCachedRecentRedirects(page_url, &redirects);
On 2015/06/16 22:06:23, sky wrote:
> If the ref isn't useful when querying for redirects than shouldn't we only do
> this call if there is no ref?

Acknowledged.

https://codereview.chromium.org/1180403005/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:2104: for
(RedirectList::const_iterator i(extra_redirects.begin());
On 2015/06/17 16:07:30, sky wrote:
> This is the part I dislike about the current patch and is why I prefer copying
> to extra_redirects. That said, I'm ok with what you have if you move 2100-2103
> into a common function that both places call.

Done.

[sky, 2015-06-17 17:39:27]

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:2096: mappings_changed =
mappings_changed ||
Don't you need the |= here, otherwise if mappings_changed is already true this
won't get executed, right?

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:2108: for
(std::vector<GURL>::const_iterator i(page_urls.begin());
I would use auto here, but this is ok.

[beaudoin, 2015-06-17 18:06:39]

PTAL

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:2096: mappings_changed =
mappings_changed ||
On 2015/06/17 17:39:27, sky wrote:
> Don't you need the |= here, otherwise if mappings_changed is already true this
> won't get executed, right?

That's what I do but since ||= doesnt exist I do m_c = m_c || ...

Is it OK to use bitwise |= on booleans?

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:2108: for
(std::vector<GURL>::const_iterator i(page_urls.begin());
On 2015/06/17 17:39:27, sky wrote:
> I would use auto here, but this is ok.

Done.

[sky, 2015-06-17 21:20:45]

And given this code would have broke, we need test coverage.

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:2096: mappings_changed =
mappings_changed ||
On 2015/06/17 18:06:39, beaudoin wrote:
> On 2015/06/17 17:39:27, sky wrote:
> > Don't you need the |= here, otherwise if mappings_changed is already true
this
> > won't get executed, right?
> 
> That's what I do but since ||= doesnt exist I do m_c = m_c || ...

The way you have the code here SetFavicon... is not called if mapping_changed is
true (try it on windows). If you use a bitwire operator it will always be
called. This is why the code on 2110 uses a bitwire operator.

> Is it OK to use bitwise |= on booleans?

[beaudoin, 2015-06-17 22:50:29]

Will try to write a test before going away and ping the thread again then.

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1180403005/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:2096: mappings_changed =
mappings_changed ||
On 2015/06/17 21:20:45, sky wrote:
> On 2015/06/17 18:06:39, beaudoin wrote:
> > On 2015/06/17 17:39:27, sky wrote:
> > > Don't you need the |= here, otherwise if mappings_changed is already true
> this
> > > won't get executed, right?
> > 
> > That's what I do but since ||= doesnt exist I do m_c = m_c || ...
> 
> The way you have the code here SetFavicon... is not called if mapping_changed
is
> true (try it on windows). If you use a bitwire operator it will always be
> called. This is why the code on 2110 uses a bitwire operator.
> 
> > Is it OK to use bitwise |= on booleans?
> 

Ack.

[beaudoin, 2015-06-18 23:45:08]

Added a test (And found an error doing it! The shame!).

PTAL.

[sky, 2015-06-19 14:49:53]

LGTM

https://codereview.chromium.org/1180403005/diff/120001/components/history/cor...
File components/history/core/browser/history_backend.h (right):

https://codereview.chromium.org/1180403005/diff/120001/components/history/cor...
components/history/core/browser/history_backend.h:699: // Returns true if the
function changed some of the |page_urls| mappings.
some->at least one

[beaudoin, 2015-06-19 15:30:24]

The CQ bit was checked by beaudoin@chromium.org

[beaudoin, 2015-06-19 15:30:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1180403005/#ps140001
(title: "Rebased and answered nit.")

[commit-bot: I haz the power, 2015-06-19 15:30:44]

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

[commit-bot: I haz the power, 2015-06-19 16:28:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-19 16:29:20]

Patchset 8 (id:??) landed as
https://crrev.com/6bef27e9bb046778a13b5cd7025208218c098e80
Cr-Commit-Position: refs/heads/master@{#335268}

[beaudoin, 2015-07-06 18:27:24]

https://codereview.chromium.org/1180403005/diff/120001/components/history/cor...
File components/history/core/browser/history_backend.h (right):

https://codereview.chromium.org/1180403005/diff/120001/components/history/cor...
components/history/core/browser/history_backend.h:699: // Returns true if the
function changed some of the |page_urls| mappings.
On 2015/06/19 14:49:53, sky wrote:
> some->at least one

Done.