Update favicon URLs on FinishLoad for main frame

Update favicon URLs on FinishLoad for main frame

Previously it was done on didStopLoading for _any_ frame.
That causes a stream of spurious favicon URL updates for
every <iframe>.
This is now moved to on didFinishLoad and get triggered only
for the main frame.  This ensures that icon loads always get
initiated after all of the other page resources have been
fetched.  Note that DidFinishDocumentLoad runs potentially
before all subresources have been fetched, which means that
the icon requests may compete with subresources for network
bandwidth.  didFinishLoad is the closest place to where
IconController does it, and we already have icons at this point.

BUG=131567
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150232

[aruslan, 2012-08-03 22:26:30]

Please help with the review.  I'm moving OnUpdateFaviconURL call from
DidStopLoading() to DidFinishLoad().  Currently we update the icon URLs set on
every DidStopLoading() for every <iframe>, which is not optimal.

This point is the closest to IconController.startLoader (of those I found) that
passes a "frame" and happens after the frame goes FrameStateComplete (which is
necessary to get icon URLs).

The code was in didStopLoading() since the "initial revision" by "mit" with the
comment
"For now we're doing the safest thing, and sending out notification when done
loading. This currently isn't an issue as the favicon is only displayed when
done loading."

This move solves a half of problems in http://crbug.com/131567, the other half
being the fact that we do not cache 404'd icons.

[jam, 2012-08-06 18:31:15]

deferring to Nate/Nico

[Nico, 2012-08-06 18:38:38]

As mentioned over email, this change makes sense to me, but I'm not intimately
familiar with this code. I'd be willing to let it in and see what breaks.

Darin had a suggestion on that email thread, so maybe you can explore his
suggestion first?

[aruslan, 2012-08-06 20:40:26]

As discussed with Darin, DidFinishLoad is the closest place to update favicon
URLs and trigger favicon fetch requests, where we 1) know the icon URLs and 2)
do not compete with subresources for network bandwidth.

[Nico, 2012-08-06 20:42:36]

Sounds reasonable, lgtm.

Darin, shout if you disagree, but it sounded like you're cool with this.

[darin (slow to review), 2012-08-06 20:46:32]

https://chromiumcodereview.appspot.com/10831163/diff/9001/chrome/renderer/chr...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/10831163/diff/9001/chrome/renderer/chr...
chrome/renderer/chrome_render_view_observer.cc:701: void
ChromeRenderViewObserver::CollectAndUpdateFaviconURLs(
please list functions in the .cpp file in the same order as they are declared in
the header file.  that means this new function should be listed just before
IsStrictSecurityHost.

[aruslan, 2012-08-06 20:52:27]

https://chromiumcodereview.appspot.com/10831163/diff/9001/chrome/renderer/chr...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/10831163/diff/9001/chrome/renderer/chr...
chrome/renderer/chrome_render_view_observer.cc:701: void
ChromeRenderViewObserver::CollectAndUpdateFaviconURLs(
On 2012/08/06 20:46:32, darin wrote:
> please list functions in the .cpp file in the same order as they are declared
in
> the header file.  that means this new function should be listed just before
> IsStrictSecurityHost.

Done.

[aruslan, 2012-08-06 21:58:14]

Thanks for the review and discussion!
I'm committing the change to CQ.

[commit-bot: I haz the power, 2012-08-06 22:17:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/10831163/3002

[darin (slow to review), 2012-08-06 22:40:10]

LGTM

[commit-bot: I haz the power, 2012-08-06 22:55:49]

Try job failure for 10831163-3002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-07 00:01:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/10831163/3002

[commit-bot: I haz the power, 2012-08-07 02:11:59]

Change committed as 150232