favicon: Replace usage of GetActiveEntry by GetLastCommittedEntry.

favicon: Replace usage of GetActiveEntry by GetLastCommittedEntry.

According to the bug crbug.com/273710 it can lead to security issues.

BUG=273710
TEST=components_unittests
R=nasko@chromium.org,sky@chromium.org

Committed: https://crrev.com/a04f01752ea1d092e823354f58d8926a346119ea
Cr-Commit-Position: refs/heads/master@{#337054}

[tfarina, 2015-06-30 21:22:56]

PTAL!

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver.cc:85: GURL
ContentFaviconDriver::GetActiveURL() {
Nasko, I guess you want me to rename this and GetActiveTitle()?

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver.cc:85: GURL
ContentFaviconDriver::GetActiveURL() {
Should I rename it to GetLastCommittedURL()?

[nasko, 2015-07-01 05:56:30]

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver.cc:85: GURL
ContentFaviconDriver::GetActiveURL() {
On 2015/06/30 21:22:55, tfarina wrote:
> Should I rename it to GetLastCommittedURL()?

Ideally, this should be renamed, but there are many more methods named Active.
I'm not sure if renaming all methods to be consistent is something we should do.
I'll leave it up to the component owner to decide.

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver.cc:87:
web_contents()->GetController().GetLastCommittedEntry();
Let's watch for failures/bugs from this CL once it lands. It is possible that
favicon and title are expected to be updated on back/forward navigation prior to
the commit of it. If that is the case, we will have to use GetVisibleEntry.

[nasko, 2015-07-01 05:56:47]

Ah, forgot to add LGTM :)

[tfarina, 2015-07-01 14:41:13]

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

[tfarina, 2015-07-01 14:41:14]

Thanks nasko.

+sky for OWNERS approval.

[sky, 2015-07-01 16:41:53]

LGTM

[tfarina, 2015-07-01 17:03:43]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2015-07-01 17:03:48]

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

[commit-bot: I haz the power, 2015-07-01 17:30:10]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-07-01 17:31:09]

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

[sdefresne, 2015-08-13 09:12:26]

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/1211993005/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver.cc:87:
web_contents()->GetController().GetLastCommittedEntry();
On 2015/07/01 at 05:56:30, nasko (very slow) wrote:
> Let's watch for failures/bugs from this CL once it lands. It is possible that
favicon and title are expected to be updated on back/forward navigation prior to
the commit of it. If that is the case, we will have to use GetVisibleEntry.

drive-by: on iOS we are using GetVisibleItem() everywhere (the iOS equivalent of
GetVisibleEntry). IIRC, when implementing WebFaviconDriver I dug into the
favicon code to know why GetActiveEntry was used and it is just because the code
pre-dated GetVisibleEntry/GetLastCommittedEntry. After testing we decided to use
GetVisibleItem().