Remove useless FaviconHandler::PageChangedSinceFaviconWasRequested()

Remove useless FaviconHandler::PageChangedSinceFaviconWasRequested()

Remove FaviconHandler::PageChangedSinceFaviconWasRequested() since both
- FaviconHandler::url_ changes (FaviconHandler::FetchFavicon())
- The NavigationController's last committed URL changes
  (NavigationControllerImpl::RendererDidNavigate())
only as a result of NavigatorImpl::DidNavigate()

BUG=517089
TEST=None

Committed: https://crrev.com/38c2b85c4d3f1426e6abab4990779d10edb0e550
Cr-Commit-Position: refs/heads/master@{#351113}

[pkotwicz, 2015-08-05 22:11:09]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-08-05 22:29:53]

pkotwicz@chromium.org changed reviewers:
+ stevenjb@chromium.org

[pkotwicz, 2015-08-05 22:29:54]

Steven can you please take a look?

I am writing this CL because the FaviconHandler code was been rotting away and
people have been duplicating its functionality

I do not understand the fix to http://crbug.com/119437. Do you remember why the
fix worked? I have confirmed that the bug does not reproduce with my change

FaviconHandler::PageChangedSinceFaviconWasRequested() is no longer needed as a
result of using NavigationController::GetLastCommittedEntry() instead of
NavigationController::GetActiveEntry() in FaviconHandler. This change was made
in https://codereview.chromium.org/1211993005

A couple of things I checked:
- Subframe navigations can occur without FaviconHandler knowing about them.
NavigationEntry::GetURL() for subframe navigations is the URL of the main page
- The history.pushState() JS call causes FaviconHandler::OnUpdateFaviconURL() to
be called

[stevenjb, 2015-08-06 15:14:05]

On 2015/08/05 22:29:54, pkotwicz wrote:
> Steven can you please take a look?
> 
> I am writing this CL because the FaviconHandler code was been rotting away and
> people have been duplicating its functionality
> 
> I do not understand the fix to http://crbug.com/119437. Do you remember why
the
> fix worked? I have confirmed that the bug does not reproduce with my change
> 
> FaviconHandler::PageChangedSinceFaviconWasRequested() is no longer needed as a
> result of using NavigationController::GetLastCommittedEntry() instead of
> NavigationController::GetActiveEntry() in FaviconHandler. This change was made
> in https://codereview.chromium.org/1211993005
> 
> A couple of things I checked:
> - Subframe navigations can occur without FaviconHandler knowing about them.
> NavigationEntry::GetURL() for subframe navigations is the URL of the main page
> - The history.pushState() JS call causes FaviconHandler::OnUpdateFaviconURL()
to
> be called

It's been a long time since I looked at the favicon code, but looking at that CL
(which no longer appears to resemble the current code) it looks like there were
two bugs addressed there:
1) If there was no current candidate we weren't returning true when there was an
exact match, potentially causing an incorrect additional request.
2) We weren't correctly identifying http://foo?bar=1 is the same page as
http://foo?bar=2

PageChangedSinceFaviconWasRequested() is new to me. It appears to have been
added in https://codereview.chromium.org/261403003. You should probably get jif@
or blundell@ to review this.

[pkotwicz, 2015-08-06 15:27:23]

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

[pkotwicz, 2015-08-06 15:27:38]

Scott, can you please take a look?

[pkotwicz, 2015-08-06 16:25:17]

Update: It looks like changing window.location.hash does not cause
FaviconHandler::OnUpdateFaviconURL() to get called.
I still believe that this CL is correct
I now better understand the fix to http://crbug.com/119437

[sky, 2015-08-06 18:14:08]

Where does the code now handle the case of the url changing after we start
download/query history.

[pkotwicz, 2015-08-06 21:49:57]

Scott, I do not understand comment #8.

I have tried changing the if statement in UrlMatches() in favicon_handler.cc to
a CHECK() and downloading a couple of files and did not get a crash. I am unsure
what you mean by "query history". Do you mean navigating history via Javascript?

[sky, 2015-08-10 17:23:36]

What I meant was we start the query for history and then the page url
changes. Say a new navigation happens. At one point the code dealt
with trying to save the favicon, but that was disabled. Where do we
handle the case of the url changing now?

  -Scott

On Thu, Aug 6, 2015 at 2:49 PM,  <pkotwicz@chromium.org> wrote:
> Scott, I do not understand comment #8.
>
> I have tried changing the if statement in UrlMatches() in favicon_handler.cc
> to
> a CHECK() and downloading a couple of files and did not get a crash. I am
> unsure
> what you mean by "query history". Do you mean navigating history via
> Javascript?
>
> https://codereview.chromium.org/1272413002/

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

[sky, 2015-08-10 19:22:39]

LGTM

https://codereview.chromium.org/1272413002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler.h (right):

https://codereview.chromium.org/1272413002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.h:237: bool
PageChangedSinceFaviconWasRequested();
remove this.

[pkotwicz, 2015-08-10 20:22:56]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2015-08-10 20:25:56]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-08-10 20:25:56]

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/1272413002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-08-10 20:26:39]

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

[commit-bot: I haz the power, 2015-08-10 21:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-10 21:10:22]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2015-08-13 02:26:34]

pkotwicz@chromium.org changed reviewers:
+ sdefresne@chromium.org

[pkotwicz, 2015-08-13 02:26:35]

sdefresne@ can you please take a look at whether this CL is ok w.r.t to iOS? I
know that the CL is ok on desktop Chrome

[pkotwicz, 2015-08-23 19:10:35]

Patchset #3 (id:80001) has been deleted

[pkotwicz, 2015-08-23 19:42:56]

Patchset #3 (id:100001) has been deleted

[pkotwicz, 2015-08-23 19:59:47]

stuartmorgan@ can you please take a look? I have changed the CL so that it does
not change the current behavior on iOS.

https://codereview.chromium.org/1272413002/diff/120001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.cc (right):

https://codereview.chromium.org/1272413002/diff/120001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:103: // changes as a result of
CRWSessionController::goToEntry(). FaviconHandler
FetchFavicon() is called by
CRWUIWebViewWebController::checkForUnexpectedURLChange() asynchronously as a
result of CRWSessionController::goToEntry()

[pkotwicz, 2015-08-23 21:52:23]

Patchset #4 (id:140001) has been deleted

[pkotwicz, 2015-08-23 21:58:17]

Patchset #4 (id:160001) has been deleted

[pkotwicz, 2015-08-24 19:24:35]

pkotwicz@chromium.org changed reviewers:
+ rohitrao@chromium.org

[pkotwicz, 2015-08-24 19:27:32]

eugenebut@ can you please take a look at the iOS changes in this CL?
It looks like stuartmorgan@ is OOO this week.

[pkotwicz, 2015-08-24 20:34:01]

pkotwicz@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[pkotwicz, 2015-08-26 16:15:40]

rohitrao@ can you please take a look at the iOS changes in this CL? (Sorry, I
included the wrong LDAP in comment #22)
It looks like stuartmorgan@ is OOO this week.

[sdefresne, 2015-08-26 16:26:07]

sorry this review fell through my inbox, lgtm

https://codereview.chromium.org/1272413002/diff/180001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.cc (right):

https://codereview.chromium.org/1272413002/diff/180001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:81: return
ActiveURLChangedSinceFetchFavicon() ? false : GetFaviconStatus().valid;
nit (optional): return !ActiveURLChangedSinceFetchFavicon() &&
GetFaviconStatus().valid;

[pkotwicz, 2015-08-27 16:56:14]

rohitrao@ ping!

[stuartmorgan, 2015-09-03 23:11:29]

Two general questions, since the overall correctness of this isn't clear to me
(not to say it's wrong, just that I don't understand from just the CL why it's
necessarily right):
- What's the answer to comment 10?
- More generally, why is having the initial call be at the same time as a URL
change enough to remove all the URL tracking, given that the fetch is async?

https://codereview.chromium.org/1272413002/diff/200001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.cc (right):

https://codereview.chromium.org/1272413002/diff/200001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:98: // has changed.
Could you add a TODO (you can put me as the reference) to eliminate this once
iOS only triggers favicon fetches synchronously, matching other platforms?

https://codereview.chromium.org/1272413002/diff/200001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:106: return GetActiveURL() !=
fetch_favicon_url_;
Why drop the fragment-trimming logic?

[pkotwicz, 2015-09-08 18:45:51]

Patchset #6 (id:220001) has been deleted

[pkotwicz, 2015-09-08 19:04:08]

Patchset #6 (id:240001) has been deleted

[pkotwicz, 2015-09-08 19:24:44]

stuartmorgan@ can you please take another look?

> Two general questions, since the overall correctness of this isn't clear to me
> (not to say it's wrong, just that I don't understand from just the CL why it's
> necessarily right):
> - What's the answer to comment 10?
> - More generally, why is having the initial call be at the same time as a URL
> change enough to remove all the URL tracking, given that the fetch is async?
FaviconHandler::FetchFavicon():
- starts a fetch for the favicon for the passed in URL
- cancels any in-progress fetches
Because FaviconHandler::FetchFavicon() cancels any in progress fetches, we can
remove all of the URL tracking as long as the FaviconHandler::FetchFavicon()
call occurs at the same time as the when the return value of
FaviconDriver::GetActiveURL() changes

Answer to comment 10:
FaviconHandler::FetchFavicon() did not always cancel in-progress fetches.
https://codereview.chromium.org/1084473002 (landed 4 months ago) started
cancelling in-progress fetches because there were long standing bugs w.r.t to
processing in-progress fetches. From offline conversation with sky@, my
understanding is that sky@'s comment is not valid because
FaviconHandler::FetchFavicon() cancels in-progress fetches.

https://codereview.chromium.org/1272413002/diff/200001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.cc (right):

https://codereview.chromium.org/1272413002/diff/200001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:98: // has changed.
On 2015/09/03 23:11:29, stuartmorgan wrote:
> Could you add a TODO (you can put me as the reference) to eliminate this once
> iOS only triggers favicon fetches synchronously, matching other platforms?

Done.

https://codereview.chromium.org/1272413002/diff/200001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:106: return GetActiveURL() !=
fetch_favicon_url_;
I put it back in for iOS

[pkotwicz, 2015-09-10 15:00:28]

stuartmorgan@ ping!

[stuartmorgan, 2015-09-10 20:52:09]

iOS portion LGTM

[pkotwicz, 2015-09-10 23:30:32]

Scott, can you please take a look at the new CL? I have added the iOS handling
since you last looked at it

[sky, 2015-09-11 15:27:15]

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/cor...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/cor...
components/favicon/core/favicon_driver.h:83: virtual bool
ShouldSendFaviconAvailableNotifications() = 0;
This feels awkward. Why not cancel the fetch?

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.cc (right):

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:22: std::string
UrlWithoutFragment(const GURL& gurl) {
Return a GURL? Especially given the name you went with.

[pkotwicz, 2015-09-11 17:38:13]

Patchset #7 (id:280001) has been deleted

[pkotwicz, 2015-09-11 17:39:58]

Scott, can you please take another look?

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/cor...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/cor...
components/favicon/core/favicon_driver.h:83: virtual bool
ShouldSendFaviconAvailableNotifications() = 0;
It is awkward. In order to cancel the fetch we need to somehow get notified when
the active URL changes. I do not know how to get notified when the active URL
changes on iOS (Looking at the iOS code base this does not seem straightforward)

Personally, I am ok with this awkwardness on iOS for now. The correct thing to
do in the long term is to make iOS call FetchFavicon() at the same times as all
of the other OSes do. Landing this CL clears the way for further simplification
of FaviconHandler too

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.cc (right):

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.cc:22: std::string
UrlWithoutFragment(const GURL& gurl) {
On 2015/09/11 15:27:15, sky wrote:
> Return a GURL? Especially given the name you went with.

Done.

[sky, 2015-09-11 19:53:21]

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/cor...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/1272413002/diff/260001/components/favicon/cor...
components/favicon/core/favicon_driver.h:83: virtual bool
ShouldSendFaviconAvailableNotifications() = 0;
On 2015/09/11 17:39:58, pkotwicz wrote:
> It is awkward. In order to cancel the fetch we need to somehow get notified
when
> the active URL changes. I do not know how to get notified when the active URL
> changes on iOS (Looking at the iOS code base this does not seem
straightforward)

I'm not familiar with the iOS code. Might rohitrao know the answer?

> Personally, I am ok with this awkwardness on iOS for now. The correct thing to
> do in the long term is to make iOS call FetchFavicon() at the same times as
all
> of the other OSes do. Landing this CL clears the way for further
simplification
> of FaviconHandler too

[pkotwicz, 2015-09-11 20:26:17]

stuartmorgan@: Is there a way that WebFaviconDriver can be notified when
NavigationManager::GetVisibleItem() changes?

[pkotwicz, 2015-09-11 20:28:35]

stuartmorgan@: Is there a way that WebFaviconDriver can be notified when
NavigationManager::GetVisibleItem() changes? (I am asking w.r.t to sky@'s
comment #41)

[stuartmorgan, 2015-09-11 20:40:40]

On 2015/09/11 20:28:35, pkotwicz wrote:
> stuartmorgan@: Is there a way that WebFaviconDriver can be notified when
> NavigationManager::GetVisibleItem() changes? (I am asking w.r.t to sky@'s
> comment #41)

I believe if you registered as a WebSateObserver and looked at the URL when
either of NavigationItemCommitted or ProvisionalNavigationStarted is called that
should cover it. (I guess technically there could also be a transient entry that
would change visible URL, but it's not clear to me that matters for this case.)

[stuartmorgan, 2015-09-11 22:38:26]

Capturing from some discussion: my basic feeling is that what's being done for
the content side is fragile--the client code must always call into the favicon
code exactly when, and every time, the URL changes, otherwise the favicon code
will potentially assign favicons to the wrong navigation entry--and I'm not
really excited about trying to replicate it onto iOS.

Temporarily doing what's in the current CL on iOS in order to simplify the core
code if that will make it easier to get to a more sane system (i.e., actually
keeping track of what navigation entry the request is associated with in some
explicit way instead of having to ensure that only one is in flight so that we
can correctly infer which entry it goes with in a totally stateless way) is okay
with me, since it doesn't spread beyond the component, which is why I lg'd it.
If it's going to spread out into ios/web/, then I'd like to step back and ask
why we can't just implement a tracking system in the favicon code now.

[sky, 2015-09-11 23:22:14]

On Fri, Sep 11, 2015 at 3:38 PM,  <stuartmorgan@chromium.org> wrote:
> Capturing from some discussion: my basic feeling is that what's being done
> for
> the content side is fragile--the client code must always call into the
> favicon
> code exactly when, and every time, the URL changes, otherwise the favicon
> code
> will potentially assign favicons to the wrong navigation entry--and I'm not
> really excited about trying to replicate it onto iOS.

All code has expectations about how it is used.

> Temporarily doing what's in the current CL on iOS in order to simplify the
> core
> code if that will make it easier to get to a more sane system (i.e.,
> actually
> keeping track of what navigation entry the request is associated with in
> some
> explicit way instead of having to ensure that only one is in flight so that
> we
> can correctly infer which entry it goes with in a totally stateless way) is
> okay
> with me, since it doesn't spread beyond the component, which is why I lg'd
> it.
> If it's going to spread out into ios/web/, then I'd like to step back and
> ask
> why we can't just implement a tracking system in the favicon code now.

Would it make more sense for the ios side to create a WebFaviconDriver
per navigation?

  -Scott

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

[pkotwicz, 2015-09-11 23:47:03]

I think that what stuartmorgan@ would like it if there was a method
FaviconDriver::SetNavigationEntryFavicon(const GURL& page_url,
                                         const GURL icon_url,
                                         gfx::Image& image)
which replaces FaviconDriver::SetActiveFaviconURL() and
FaviconDriver::SetActiveFaviconImage()
FaviconDriver::SetNavigationEntryFavicon() would update the NavigationEntries /
NavigationItems which match the passed in page URL.

This has the benefit of:
- Protecting Chrome in the unlikely case that FaviconDriver goes rogue
- Moving the ugliness entirely into iOS. We would be able to remove
FaviconDriver::ShouldSendFaviconAvailableNotifications(). However,
WebFaviconDriver::FaviconUrlUpdated(), would still need to check
WebFaviconDriver::ActiveURLChangedSinceFetchFavicon()
(WebFaviconDriver::GetActiveFaviconValidity() and
WebFaviconDriver::GetActiveFaviconURL() would still need to too but those
methods will disappear soonish)

[pkotwicz, 2015-09-14 02:04:20]

I spent a lot of time thinking.
- This CL is now less awkward. It checks whether FaviconHandler's version of the
page URL is different from the active page URL in
FaviconHandler::NotifyFaviconAvailable()
  In a future CL, I will remove
WebFaviconDriver::ActiveURLChangedSinceFetchFavicon() (and
WebFaviconDriver::GetActiveFaviconValidity() and
WebFaviconDriver::GetActiveFaviconURL())
- There is now a DCHECK() which clearly indicates that the active page URL
should not change between calls to FaviconHandler::FaviconFavicon() on non-iOS
(Otherwise there will be people like me who will wonder why the if statement is
there. In the past I have tried to figure out why if statements got added 3 - 4
years ago and it is extremely difficult. The fragment stripping logic in
FaviconHandler::PageChangedSinceFaviconWasRequested() is one such example.)
- This CL now protects against FaviconHandler going haywire on all OSes because
the page URL is passed in FaviconHandler::NotifyFaviconAvailable()
- FaviconHandler::NotifyFaviconAvailable() does not check the equality of URLs
with the fragment stripped on iOS.
  - I have checked and http://crbug.com/119437 does not occur with fragment
stripping logic removed on the desktop
  - I do not understand why we added the fragment stripping logic in the first
place. Maybe the logic was added to handle how Gmail used to behave but no
longer does? (It was added in https://codereview.chromium.org/9852012)
  - Favicons are completely busted as a result of fragment navigations on iOS
with or without this CL. See http://crbug.com/531267

stuartmorgan@ I know that you were hoping that one day
FaviconHandler::FetchFavicon() will not cancel the previous request. This is a
feature request, and I do not foresee working on it this year.

[pkotwicz, 2015-09-14 02:04:50]

sky@ and stuartmorgan@ can you please take another look?

I spent a lot of time thinking.
- This CL is now less awkward. It checks whether FaviconHandler's version of the
page URL is different from the active page URL in
FaviconHandler::NotifyFaviconAvailable()
  In a future CL, I will remove
WebFaviconDriver::ActiveURLChangedSinceFetchFavicon() (and
WebFaviconDriver::GetActiveFaviconValidity() and
WebFaviconDriver::GetActiveFaviconURL())
- There is now a DCHECK() which clearly indicates that the active page URL
should not change between calls to FaviconHandler::FaviconFavicon() on non-iOS
(Otherwise there will be people like me who will wonder why the if statement is
there. In the past I have tried to figure out why if statements got added 3 - 4
years ago and it is extremely difficult. The fragment stripping logic in
FaviconHandler::PageChangedSinceFaviconWasRequested() is one such example.)
- This CL now protects against FaviconHandler going haywire on all OSes because
the page URL is passed in FaviconHandler::NotifyFaviconAvailable()
- FaviconHandler::NotifyFaviconAvailable() does not check the equality of URLs
with the fragment stripped on iOS.
  - I have checked and http://crbug.com/119437 does not occur with fragment
stripping logic removed on the desktop
  - I do not understand why we added the fragment stripping logic in the first
place. Maybe the logic was added to handle how Gmail used to behave but no
longer does? (It was added in https://codereview.chromium.org/9852012)
  - Favicons are completely busted as a result of fragment navigations on iOS
with or without this CL. See http://crbug.com/531267

stuartmorgan@ I know that you were hoping that one day
FaviconHandler::FetchFavicon() will not cancel the previous request. This is a
feature request, and I do not foresee working on it this year.

[pkotwicz, 2015-09-14 02:12:53]

Patchset #8 (id:320001) has been deleted

[pkotwicz, 2015-09-14 02:12:59]

Patchset #8 (id:340001) has been deleted

[pkotwicz, 2015-09-16 00:45:45]

sky@ Ping!

[sky, 2015-09-16 15:14:24]

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
File components/favicon/core/favicon_driver_impl.cc (right):

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
components/favicon/core/favicon_driver_impl.cc:113: if (page_url !=
GetActiveURL())
Shouldn't this be an a #else?

[pkotwicz, 2015-09-16 15:17:56]

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
File components/favicon/core/favicon_driver_impl.cc (right):

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
components/favicon/core/favicon_driver_impl.cc:113: if (page_url !=
GetActiveURL())
I still check this on non-IOS in the case that FaviconHandler goes rogue.

I have seen this pattern
DCHECK(should_be_true);
if (should_be_true) {
}

in a few places in the codebase.

[stuartmorgan, 2015-09-16 18:35:58]

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
File components/favicon/core/favicon_driver_impl.cc (right):

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
components/favicon/core/favicon_driver_impl.cc:113: if (page_url !=
GetActiveURL())
On 2015/09/16 15:17:56, pkotwicz wrote:
> I have seen this pattern
> DCHECK(should_be_true);
> if (should_be_true) {
> }
> 
> in a few places in the codebase.

It's explicitly against Chromium's style guide.

[pkotwicz, 2015-09-16 18:47:57]

sky@ and stuartmorgan@ can you please take another look?

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
File components/favicon/core/favicon_driver_impl.cc (right):

https://codereview.chromium.org/1272413002/diff/360001/components/favicon/cor...
components/favicon/core/favicon_driver_impl.cc:113: if (page_url !=
GetActiveURL())
Fixed now. You are right it is specifically mentioned in the style guide

[stuartmorgan, 2015-09-16 18:55:26]

On 2015/09/11 23:22:14, sky wrote:
> On Fri, Sep 11, 2015 at 3:38 PM,  <mailto:stuartmorgan@chromium.org> wrote:
> > Capturing from some discussion: my basic feeling is that what's being done
> > for
> > the content side is fragile--the client code must always call into the
> > favicon
> > code exactly when, and every time, the URL changes, otherwise the favicon
> > code
> > will potentially assign favicons to the wrong navigation entry--and I'm not
> > really excited about trying to replicate it onto iOS.
> 
> All code has expectations about how it is used.

Sure, but code where the expectation is that anyone ever making any changes to
the navigation system must know about the implementation details of the favicon
service is, IMO, a poor expectation for it to create. Good API is hard to use
wrong, and an API that will essentially corrupt data if you ever forget to call
part of it at the exact right time is easy to use wrong.

I think a request that's specific to a navigation entry should be tracked in a
way that binds back to that entry, rather than requiring that global state be in
exactly the state it wants when it finishes.

[pkotwicz, 2015-09-16 19:17:47]

I will defer to sky@ for design/architecture guidance. I will implement the
design that stuartmorgan@ and sky@ agree upon.

[sky, 2015-09-16 20:11:55]

On Wed, Sep 16, 2015 at 11:55 AM,  <stuartmorgan@chromium.org> wrote:
> On 2015/09/11 23:22:14, sky wrote:
>>
>> On Fri, Sep 11, 2015 at 3:38 PM,  <mailto:stuartmorgan@chromium.org>
>> wrote:
>> > Capturing from some discussion: my basic feeling is that what's being
>> > done
>> > for
>> > the content side is fragile--the client code must always call into the
>> > favicon
>> > code exactly when, and every time, the URL changes, otherwise the
>> > favicon
>> > code
>> > will potentially assign favicons to the wrong navigation entry--and I'm
>> > not
>> > really excited about trying to replicate it onto iOS.
>
>
>> All code has expectations about how it is used.
>
>
> Sure, but code where the expectation is that anyone ever making any changes
> to
> the navigation system must know about the implementation details of the
> favicon
> service is, IMO, a poor expectation for it to create. Good API is hard to
> use
> wrong, and an API that will essentially corrupt data if you ever forget to
> call
> part of it at the exact right time is easy to use wrong.

Agreed.

> I think a request that's specific to a navigation entry should be tracked in
> a
> way that binds back to that entry, rather than requiring that global state
> be in
> exactly the state it wants when it finishes.

Agreed.

IMO these problems are because of how the favicon code was refactored.
It could really use a cleanup.

  -Scott

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

[pkotwicz, 2015-09-18 01:06:14]

sky@ and stuartmorgan@ can you please take another look?

I agree that it is not obvious that the active entry's URL changes only when
WebContentsObserver::DidNavigateMainFrame() is called. I added checking whether
the active URL changed under us in FaviconDriverImpl::OnFaviconAvailable() for
all OSes

Favicon handling will forever be coupled tightly to navigations. We rely on the
web contents to send us the icon URLs for the page. The correct favicon for the
page cannot be determined till we get this information from the renderer. (We
get this information when the page stops loading)

It should not be necessary for FaviconHandler to know anything about
NavigationEntries. If the current request needs to be cancelled, we can have a
FaviconHandler::Cancel() method. Right now things are super confusing because
FaviconHandler does not own its state. It stores its state in the
NavigationEntry which is super confusing. I am trying to remove this coupling.

[pkotwicz, 2015-09-22 00:50:06]

Ping!

[stuartmorgan, 2015-09-22 21:57:57]

I'm not sure I'm understanding the current patch; if you're checking for URL
changes on all platforms, why does there need to be iOS-specific tracking of the
URL that was requested?

[pkotwicz, 2015-09-22 22:04:10]

The iOS-specific tracking is because FaviconHandler currently checks
WebFaviconDriver::GetActiveFaviconValidity() and
WebFaviconDriver::GetActiveFaviconURL(). I have a CL which removes
WebFaviconDriver::GetActiveFaviconValidity() and
WebFaviconDriver::GetActiveFaviconURL() Unfortunately, that CL is blocked on
this one landing.

[stuartmorgan, 2015-09-22 23:22:23]

Okay, iOS lgtm then.

[pkotwicz, 2015-09-22 23:26:00]

Scott, can you please take another look?

[sky, 2015-09-22 23:43:31]

Can you provide an outline of the changes you're planning on doing?

[pkotwicz, 2015-09-23 00:34:16]

My goal is to make the code in FaviconHandler more understandable. The end goal
is not to fix any bugs but for the code to be simpler and clearer. I am not
planning any major changes but rather to make lots of small changes which help
readability. (My thinking is that if the code is simpler, it will be easier for
others to fix bugs)
I am planning on:
- Making FaviconHandler not call FaviconDriver::GetActiveFaviconValidity() and
not call FaviconDriver::GetActiveFaviconURL(). FaviconHandler should not know
about the NavigationEntry. This looks easy to do.
- Make FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() and
FaviconHandler::ProcessCurrentUrl() share code
- Add an early exit so that if FaviconHandler::OnUpdateFaviconURL() is called
twice with the same URLs that the second call is a no-op 
- Figure out why FaviconDriverImpl::OnFaviconAvailable() has the early exit if
the passed in gfx::Image is empty and write a comment as to why the early exit
is there
- Look into whether the notification dispatching code in
FaviconHandler::OnUpdateFaviconURL() can be simplified. (Why do we have two
methods: FaviconDriverObserver::OnFaviconAvailable() and
FaviconDriverObserver::OnFaviconUpdated() ?)

[pkotwicz, 2015-09-23 00:39:49]

For instance, https://codereview.chromium.org/1295733002/ implements: "Add an
early exit so that if FaviconHandler::OnUpdateFaviconURL() is called
twice with the same URLs that the second call is a no-op"

[sky, 2015-09-23 18:19:46]

I think FaviconHandler should be changed to focus on a single url. By
that I mean you create a FaviconHandler with the url to get the icons
for (FetchFavicon is nuked, the url is moved to the constructor). I
think doing this would result in much less error prone class. I
imagine this will entail lots of changes (for the better) to the
surrounding classes as well.

  -Scott

On Tue, Sep 22, 2015 at 5:34 PM,  <pkotwicz@chromium.org> wrote:
> My goal is to make the code in FaviconHandler more understandable. The end
> goal
> is not to fix any bugs but for the code to be simpler and clearer. I am not
> planning any major changes but rather to make lots of small changes which
> help
> readability. (My thinking is that if the code is simpler, it will be easier
> for
> others to fix bugs)
> I am planning on:
> - Making FaviconHandler not call FaviconDriver::GetActiveFaviconValidity()
> and
> not call FaviconDriver::GetActiveFaviconURL(). FaviconHandler should not
> know
> about the NavigationEntry. This looks easy to do.
> - Make FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() and
> FaviconHandler::ProcessCurrentUrl() share code
> - Add an early exit so that if FaviconHandler::OnUpdateFaviconURL() is
> called
> twice with the same URLs that the second call is a no-op
> - Figure out why FaviconDriverImpl::OnFaviconAvailable() has the early exit
> if
> the passed in gfx::Image is empty and write a comment as to why the early
> exit
> is there
> - Look into whether the notification dispatching code in
> FaviconHandler::OnUpdateFaviconURL() can be simplified. (Why do we have two
> methods: FaviconDriverObserver::OnFaviconAvailable() and
> FaviconDriverObserver::OnFaviconUpdated() ?)
>
> https://codereview.chromium.org/1272413002/

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

[pkotwicz, 2015-09-23 18:36:30]

Making FaviconHandler take a single URL in the constructor is reasonable. I
think that this CL moves us towards that goal.

After this CL:
- We need to make FaviconHandler not call
FaviconDriver::GetActiveFaviconValidity() and
FaviconDriver::GetActiveFaviconURL()
- Do some minor refactoring such that FaviconHandler takes the page URL in the
constructor

[sky, 2015-09-23 21:55:11]

LGTM

[pkotwicz, 2015-09-28 18:21:15]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-09-28 18:21:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1272413002/#ps390001
(title: " ")

[commit-bot: I haz the power, 2015-09-28 18:24:24]

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

[commit-bot: I haz the power, 2015-09-28 19:31:01]

Committed patchset #10 (id:390001)

[commit-bot: I haz the power, 2015-09-28 19:32:30]

Patchset 10 (id:??) landed as
https://crrev.com/38c2b85c4d3f1426e6abab4990779d10edb0e550
Cr-Commit-Position: refs/heads/master@{#351113}