DidStopLoadingIcons should account for remote main frame.

DidStopLoadingIcons should account for remote main frame.

BUG=399775
Committed: https://crrev.com/2411ba3706ff9a594df9ce766a5d05f8f7355294
Cr-Commit-Position: refs/heads/master@{#299318}

[nasko, 2014-10-07 23:38:01]

nasko@chromium.org changed reviewers:
+ creis@chromium.org

[nasko, 2014-10-07 23:38:02]

Hey Charlie,
This is one of the little things we hit along the way when we have main
WebRemoteFrame. Should we return early in that case?
Thanks,
Nasko

[Charlie Reis, 2014-10-08 19:18:39]

LGTM

https://codereview.chromium.org/634123002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/634123002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:4177: if
(webview()->mainFrame()->isWebRemoteFrame())
This is probably the right thing long-term, right?  I think Favicons / touch
icons only matter for the main frame, so it sounds right to ignore them when the
main frame is remote.

Can you put a comment to that effect?

[nasko, 2014-10-13 16:55:10]

The CQ bit was checked by nasko@chromium.org

[nasko, 2014-10-13 16:55:50]

https://codereview.chromium.org/634123002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/634123002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:4177: if
(webview()->mainFrame()->isWebRemoteFrame())
On 2014/10/08 19:18:38, Charlie Reis wrote:
> This is probably the right thing long-term, right?  I think Favicons / touch
> icons only matter for the main frame, so it sounds right to ignore them when
the
> main frame is remote.
> 
> Can you put a comment to that effect?

Done.

[commit-bot: I haz the power, 2014-10-13 16:56:33]

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

[commit-bot: I haz the power, 2014-10-13 17:45:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-13 17:46:31]

Patchset 2 (id:??) landed as
https://crrev.com/2411ba3706ff9a594df9ce766a5d05f8f7355294
Cr-Commit-Position: refs/heads/master@{#299318}