Use synced favicons in the Other Devices menu when possible.

Use synced favicons in the Other Devices menu when possible.

BUG=124441
TEST=Open two signed-in sessions, and visit a page in one browser that
has never been visited in the other browser (verify that the favicon is
not cached in browser #2 by checking chrome://favicon). When the open
tab is synced to browser #2, it should also get the favicon.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134547

[Patrick Dubroy, 2012-04-24 14:01:23]

Hey Evan, please take a look.

(cc: zea)

[Evan Stade, 2012-04-24 18:08:12]

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
(base::Base64Encode(favicon_data, &encoded_favicon_data))
cook this into the favicon data source IMO.

[Evan Stade, 2012-04-24 18:11:27]

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
(base::Base64Encode(favicon_data, &encoded_favicon_data))
On 2012/04/24 18:08:12, Evan Stade wrote:
> cook this into the favicon data source IMO.

in fact, I would try to make a session-favicon data source which inherits from
the plain one.

[Patrick Dubroy, 2012-04-25 09:32:54]

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
(base::Base64Encode(favicon_data, &encoded_favicon_data))
On 2012/04/24 18:11:27, Evan Stade wrote:
> On 2012/04/24 18:08:12, Evan Stade wrote:
> > cook this into the favicon data source IMO.
> 
> in fact, I would try to make a session-favicon data source which inherits from
> the plain one.

Hmmm....I see the point, but do you think it's worth the extra code/complexity?
I'm loath to add a new class and data source when a method will do the job, and
I can't think of any other places that we'll need to use the session favicons.

Nicolas, any thoughts on this?

[Evan Stade, 2012-04-25 19:34:21]

On 2012/04/25 09:32:54, dubroy wrote:
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):
> 
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
> (base::Base64Encode(favicon_data, &encoded_favicon_data))
> On 2012/04/24 18:11:27, Evan Stade wrote:
> > On 2012/04/24 18:08:12, Evan Stade wrote:
> > > cook this into the favicon data source IMO.
> > 
> > in fact, I would try to make a session-favicon data source which inherits
from
> > the plain one.
> 
> Hmmm....I see the point, but do you think it's worth the extra
code/complexity?

I believe the complexity will be reduced, as well as tucked away in a separate
class instead of sprinkled on top of existing code. Although, yes, the LoC might
be slightly higher.

> I'm loath to add a new class and data source when a method will do the job,
and
> I can't think of any other places that we'll need to use the session favicons.
> 
> Nicolas, any thoughts on this?

[Nicolas Zea, 2012-04-25 20:41:29]

On 2012/04/25 19:34:21, Evan Stade wrote:
> On 2012/04/25 09:32:54, dubroy wrote:
> >
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> > File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):
> > 
> >
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> > chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
> > (base::Base64Encode(favicon_data, &encoded_favicon_data))
> > On 2012/04/24 18:11:27, Evan Stade wrote:
> > > On 2012/04/24 18:08:12, Evan Stade wrote:
> > > > cook this into the favicon data source IMO.
> > > 
> > > in fact, I would try to make a session-favicon data source which inherits
> from
> > > the plain one.
> > 
> > Hmmm....I see the point, but do you think it's worth the extra
> code/complexity?
> 
> I believe the complexity will be reduced, as well as tucked away in a separate
> class instead of sprinkled on top of existing code. Although, yes, the LoC
might
> be slightly higher.
> 
> > I'm loath to add a new class and data source when a method will do the job,
> and
> > I can't think of any other places that we'll need to use the session
favicons.
> > 
> > Nicolas, any thoughts on this?

Moving the base64 encoding into the SessionModelAssociator seems reasonable, but
I'm not sure how the adding a new class (if I understand the suggestion
properly) would help. We're passing around a string, and it seems the UI expects
a string (albeit base64 encoded), so putting into and pulling it out of another
container doesn't seem to improve anything.

[arv (Not doing code reviews), 2012-04-25 22:06:21]

http://codereview.chromium.org/10205014/diff/1/chrome/browser/resources/ntp4/...
File chrome/browser/resources/ntp4/other_sessions.js (right):

http://codereview.chromium.org/10205014/diff/1/chrome/browser/resources/ntp4/...
chrome/browser/resources/ntp4/other_sessions.js:180: var favicon_url;
no underscores in js

http://codereview.chromium.org/10205014/diff/1/chrome/browser/resources/ntp4/...
chrome/browser/resources/ntp4/other_sessions.js:185: a.style.backgroundImage =
'url(' + favicon_url + ')';
You should probably use the url function found in util.js. Just doing the naïve
string concat has issues in webkit.

[Patrick Dubroy, 2012-04-26 16:49:14]

On 2012/04/25 19:34:21, Evan Stade wrote:
> On 2012/04/25 09:32:54, dubroy wrote:
> >
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> > File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):
> > 
> >
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> > chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
> > (base::Base64Encode(favicon_data, &encoded_favicon_data))
> > On 2012/04/24 18:11:27, Evan Stade wrote:
> > > On 2012/04/24 18:08:12, Evan Stade wrote:
> > > > cook this into the favicon data source IMO.
> > > 
> > > in fact, I would try to make a session-favicon data source which inherits
> from
> > > the plain one.
> > 
> > Hmmm....I see the point, but do you think it's worth the extra
> code/complexity?
> 
> I believe the complexity will be reduced, as well as tucked away in a separate
> class instead of sprinkled on top of existing code. Although, yes, the LoC
might
> be slightly higher.

It seems to me that the new SessionFaviconSource would need to have a pointer to
the FaviconSource, so that we can unconditionally use
chrome://session-favicon/http://blah for the img URLs. Then we'll just have to
duplicate the code here to decide whether to use the synced favicon or the
regular one. So, complexity wise, I don't see how it would be better.

I can see the benefit in making it work more like regular favicons, but it _is_
only 7 lines of code.

> > I'm loath to add a new class and data source when a method will do the job,
> and
> > I can't think of any other places that we'll need to use the session
favicons.
> > 
> > Nicolas, any thoughts on this?

[Evan Stade, 2012-04-26 19:26:16]

On 2012/04/26 16:49:14, dubroy wrote:
> On 2012/04/25 19:34:21, Evan Stade wrote:
> > On 2012/04/25 09:32:54, dubroy wrote:
> > >
> >
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> > > File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
> > > chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
> > > (base::Base64Encode(favicon_data, &encoded_favicon_data))
> > > On 2012/04/24 18:11:27, Evan Stade wrote:
> > > > On 2012/04/24 18:08:12, Evan Stade wrote:
> > > > > cook this into the favicon data source IMO.
> > > > 
> > > > in fact, I would try to make a session-favicon data source which
inherits
> > from
> > > > the plain one.
> > > 
> > > Hmmm....I see the point, but do you think it's worth the extra
> > code/complexity?
> > 
> > I believe the complexity will be reduced, as well as tucked away in a
separate
> > class instead of sprinkled on top of existing code. Although, yes, the LoC
> might
> > be slightly higher.
> 
> It seems to me that the new SessionFaviconSource would need to have a pointer
to
> the FaviconSource, so that we can unconditionally use
> chrome://session-favicon/http://blah for the img URLs.

no, I meant to inherit from the normal faviconsource.

> Then we'll just have to
> duplicate the code here to decide whether to use the synced favicon or the
> regular one. So, complexity wise, I don't see how it would be better.

the javascript would be almost unchanged, and this code would be moved rather
than duplicated.

> 
> I can see the benefit in making it work more like regular favicons, but it
_is_
> only 7 lines of code.
> 
> > > I'm loath to add a new class and data source when a method will do the
job,
> > and
> > > I can't think of any other places that we'll need to use the session
> favicons.
> > > 
> > > Nicolas, any thoughts on this?

[Patrick Dubroy, 2012-04-27 18:27:02]

Ok, implemented SessionFaviconSource (chrome://session-favicon/) as suggested by
Evan.

http://codereview.chromium.org/10205014/diff/1/chrome/browser/resources/ntp4/...
File chrome/browser/resources/ntp4/other_sessions.js (right):

http://codereview.chromium.org/10205014/diff/1/chrome/browser/resources/ntp4/...
chrome/browser/resources/ntp4/other_sessions.js:180: var favicon_url;
On 2012/04/25 22:06:22, arv wrote:
> no underscores in js

This is gone.

http://codereview.chromium.org/10205014/diff/1/chrome/browser/resources/ntp4/...
chrome/browser/resources/ntp4/other_sessions.js:185: a.style.backgroundImage =
'url(' + favicon_url + ')';
On 2012/04/25 22:06:22, arv wrote:
> You should probably use the url function found in util.js. Just doing the
naïve
> string concat has issues in webkit.

Done.

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):

http://codereview.chromium.org/10205014/diff/1/chrome/browser/ui/webui/ntp/fo...
chrome/browser/ui/webui/ntp/foreign_session_handler.cc:268: if
(base::Base64Encode(favicon_data, &encoded_favicon_data))
On 2012/04/24 18:11:27, Evan Stade wrote:
> On 2012/04/24 18:08:12, Evan Stade wrote:
> > cook this into the favicon data source IMO.
> 
> in fact, I would try to make a session-favicon data source which inherits from
> the plain one.

Done.

[Evan Stade, 2012-04-27 19:56:28]

thanks. lgtm

http://codereview.chromium.org/10205014/diff/18010/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right):

http://codereview.chromium.org/10205014/diff/18010/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/foreign_session_handler.cc:78:
profile->GetChromeURLDataManager()->AddDataSource(
ChromeURLDataManager::AddDataSource(

http://codereview.chromium.org/10205014/diff/18010/chrome/browser/ui/webui/se...
File chrome/browser/ui/webui/session_favicon_source.cc (right):

http://codereview.chromium.org/10205014/diff/18010/chrome/browser/ui/webui/se...
chrome/browser/ui/webui/session_favicon_source.cc:23: SessionModelAssociator*
SessionFaviconSource::GetSessionModelAssociator() {
I don't see a strong need for this function