Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11195010:
Extract Favicon Download logic from FaviconTabHelper (Closed)
|
Created:
8 years, 2 months ago by Cait (Slow) Modified:
8 years, 1 month ago CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionExtract Favicon Download logic from FaviconTabHelper
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165216
Patch Set 1 #
Total comments: 18
Patch Set 2 : Reviewer comments #
Total comments: 4
Patch Set 3 : DEPS and nits #
Total comments: 1
Patch Set 4 : DEPS fix and delegate #
Total comments: 11
Patch Set 5 : Extract OnUpdateFaviconURL code #
Total comments: 6
Patch Set 6 : #
Total comments: 12
Patch Set 7 : Comments #
Total comments: 16
Patch Set 8 : #
Total comments: 10
Patch Set 9 : #
Messages
Total messages: 32 (0 generated)
Hi Jói -- PTAL. This is a first pass at extracting the Favicon download logic into a WebContentsUserData + Delegate form. One thing I was a little unsure of was how to allow users of the FaviconDownloadHelper to set their own delegates (as presumably this is what WebView would need to do). I followed the Autofill example and provided a SetExternalDelegate method -- not sure if there is a better approach though. If it looks like I'm on the right track, I'll loop in Jonathan and Favicon/ OWNERS. Thanks, Cait
I think you're on the right track, yes. I added a bunch of nits. Cheers, Jói http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... File chrome/browser/favicon/favicon_download_delegate.h (right): http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_delegate.h:15: class FaviconDownloadDelegate { Seems like this should be named FaviconDownloadHelperDelegate. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_delegate.h:17: virtual void OnDidDownloadFavicon( Needs documentation. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:12: #include "chrome/common/favicon_url.h" Is favicon_url.h free of other dependencies? Once you have this change nailed down, I would suggest adding a specific_include_rules section to chrome/browser/favicon/DEPS that nails down the dependencies for the individual files reused by WebView, i.e. has a blanket "-chrome" and "-chrome/browser" (the latter is needed because a DEPS file further up will have "+chrome/browser") and then just allows the files we know aren't going to transitively include the rest of the world. That, or I guess this code could move to a new chrome/browser/component subdirectory with strict DEPS rules. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:27: class FaviconDownloadHelper : public content::WebContentsObserver, indent like this class FaviconDownloadHelper : public content::WebContentsObserver, public content::WebContentsUserData<FaviconDownloadHelper> { or like this if it fits class A : public Foo, public Blat { http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:33: int DownloadFavicon(const GURL& url, int image_size); Need to document the return value is the 'id' you will get via the delegate method. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:41: virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; I think this can be protected; likewise OnDidDownloadFavicon. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:53: FaviconDownloadDelegate* external_delegate_; Should be private. Can name it just delegate_. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:56: explicit FaviconDownloadHelper(content::WebContents* web_contents); You could follow the example of AutofillManager::CreateForWebContentsAndDelegate, instead of having a separate SetExternalDelegate method. I think this would be preferable since it seems without a delegate this class is kind of pointless? http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... File chrome/browser/favicon/favicon_tab_helper.h (right): http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_tab_helper.h:102: const GURL& image_url, indentation
http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... File chrome/browser/favicon/favicon_download_delegate.h (right): http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_delegate.h:15: class FaviconDownloadDelegate { On 2012/10/16 21:56:52, Jói wrote: > Seems like this should be named FaviconDownloadHelperDelegate. Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_delegate.h:17: virtual void OnDidDownloadFavicon( On 2012/10/16 21:56:52, Jói wrote: > Needs documentation. Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:12: #include "chrome/common/favicon_url.h" On 2012/10/16 21:56:52, Jói wrote: > Is favicon_url.h free of other dependencies? > > Once you have this change nailed down, I would suggest adding a > specific_include_rules section to chrome/browser/favicon/DEPS that nails down > the dependencies for the individual files reused by WebView, i.e. has a blanket > "-chrome" and "-chrome/browser" (the latter is needed because a DEPS file > further up will have "+chrome/browser") and then just allows the files we know > aren't going to transitively include the rest of the world. > > That, or I guess this code could move to a new chrome/browser/component > subdirectory with strict DEPS rules. Agree. Looking into breaking the remaining DEPS for favicon_util (c/b/history/history_types and c/b/history/select_favicon_frames) and then I'll put strict DEPS rules on it and favicon_download_helper. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:27: class FaviconDownloadHelper : public content::WebContentsObserver, On 2012/10/16 21:56:52, Jói wrote: > indent like this > > class FaviconDownloadHelper > : public content::WebContentsObserver, > public content::WebContentsUserData<FaviconDownloadHelper> { > > or like this if it fits > > class A : public Foo, > public Blat { Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:33: int DownloadFavicon(const GURL& url, int image_size); On 2012/10/16 21:56:52, Jói wrote: > Need to document the return value is the 'id' you will get via the delegate > method. Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:41: virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; On 2012/10/16 21:56:52, Jói wrote: > I think this can be protected; likewise OnDidDownloadFavicon. Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:53: FaviconDownloadDelegate* external_delegate_; On 2012/10/16 21:56:52, Jói wrote: > Should be private. Can name it just delegate_. Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_download_helper.h:56: explicit FaviconDownloadHelper(content::WebContents* web_contents); On 2012/10/16 21:56:52, Jói wrote: > You could follow the example of > AutofillManager::CreateForWebContentsAndDelegate, instead of having a separate > SetExternalDelegate method. I think this would be preferable since it seems > without a delegate this class is kind of pointless? Done. http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... File chrome/browser/favicon/favicon_tab_helper.h (right): http://codereview.chromium.org/11195010/diff/1/chrome/browser/favicon/favicon... chrome/browser/favicon/favicon_tab_helper.h:102: const GURL& image_url, On 2012/10/16 21:56:52, Jói wrote: > indentation Done.
LGTM with a nit. Will DEPS improvements be in the same change or a follow-up? http://codereview.chromium.org/11195010/diff/6001/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/6001/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_download_helper.h:40: Just one blank line http://codereview.chromium.org/11195010/diff/6001/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_download_helper.h:42: // request. The id will be sent in the IPC message. More relevant to this interface, is "The id will be passed to FaviconDownloadHelperDelegate::OnDidDownloadFavicon once the favicon has been retrieved."
Added the DEPS specific_rules. Will save the history/ DEPS for a later CL though. Cheers, Cait http://codereview.chromium.org/11195010/diff/6001/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/6001/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_download_helper.h:40: On 2012/10/18 10:31:37, Jói wrote: > Just one blank line Done. http://codereview.chromium.org/11195010/diff/6001/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_download_helper.h:42: // request. The id will be sent in the IPC message. On 2012/10/18 10:31:37, Jói wrote: > More relevant to this interface, is "The id will be passed to > FaviconDownloadHelperDelegate::OnDidDownloadFavicon once the favicon has been > retrieved." Done.
http://codereview.chromium.org/11195010/diff/2007/chrome/browser/favicon/DEPS File chrome/browser/favicon/DEPS (right): http://codereview.chromium.org/11195010/diff/2007/chrome/browser/favicon/DEPS... chrome/browser/favicon/DEPS:40: "+chrome/browser/favicon", You probably don't want to allow all of chrome/browser/favicon since some of them may transitively include the rest of the world. Same thing below I think.
[+stevenjb, joth] stevenjb (OWNERS): PTAL: this change is so that WebView can download favicons w/o having to set up a FaviconService. joth: Will this delegate interface work for your needs?
Sorry for slow review, taken me a while to assemble all the pieces in my mind. This is definitely a step towards our needs, but we need some more of the renderer side bit both for handling the download, but also determining the page URL => icon URL mapping prior to starting the icon download. http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.cc (right): http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:56: int id = FaviconUtil::DownloadFavicon(host, url, image_size); internally this sends IconMsg_DownloadFavicon ipc which is handled by chrome/renderer/chrome_render_view_observer.cc which we don't (or, won't) have installed in webview, so some of the renderer side code also needs extracting I think. working back, this line adds a dependency on FaviconUtil which itself pulls in chrome/browser/history... types. So I think we'll need to unpick those too. (maybe, just remove DownloadFavicon from FaviconUtil and do it right here?) http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:31: public base::RefCounted<FaviconDownloadHelper> { does this need to be ref-counted? seems overkill if it's only for benefit of reusing the user data adapter. You might be able to extend WebContentsUserData<FaviconDownloadHelper> instead. You'll then need to move the delegate param out of c'tor and into a separate setter method though. http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:34: static void CreateForWebContentsAndDelegate( nit: I think the AndDelegate is spurious here, and potentially misleading: this method will create at most one helper instance per web contents, and bind the lifetime of the helper to that WebContents instance (only). The delegate doesn't effect this logic. (indeed if you call this method a multiple time for a given web_contents but with a different delegate, the calls are ignored. perhaps we should assert against that, or if really necessary, update the existing helper to point to new delegate) http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:43: // been retrieved. could mention what |image_size| is. (is it a _max_size?) (I assume it doesn't need to be int64? :-) http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:69: DISALLOW_COPY_AND_ASSIGN(FaviconDownloadHelper); think this can be DISALLOW_IMPLICIT_CONSTRUCTORS http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.cc (right): http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.cc:58: gfx::Image FaviconTabHelper::GetFavicon() const { FWIW, we're going to need a method equivalent to this too, to implement WebView.getFavicon() http://developer.android.com/reference/android/webkit/WebView.html#getFavicon() However... it's probably easiest if we do this bit ourselves: most likely, we'd bounce the _encoded_ image to java and decode it over there to pass as a java Bitmap to the embedder, and also stash the same Bitmap instance to implement the above getter. Just mention this here, in case there's some other issue we might run into if we *don't* have the following code that keeps NavigationController::GetEntry()->GetFavIcon() up to date? (that is a content API, so there maybe some hidden dependency somewhere else in content that expects it to be working correctly?) http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.cc:136: void FaviconTabHelper::OnUpdateFaviconURL( hmmm OK I'm just re-reminding myself how the whole favicon flow works. What we are after is the ability to generate a callback with the page favicon in response to each call to top-level call to WebView.loadUrl (i.e. per WebContents::OpenURL) This requires at least two parts 1/ load content for page_url and parse it for <head><link rel="icon".. tag. (falling back to site-wide icon). This yeilds icon_url. (and possibly, a touch-icon URL, which we can pass to the application unaltered) 2/ make download request for icon_url (3/ decode the received data to an Image, and stash it inside NavigationController.) With this patch, I think FaviconDownloadHelper::DownloadFavicon would only give us #2 of that list. To get #1, we need to have this code to handle IconHostMsg_UpdateFaviconURL (and of course also need the associated renderer-side code that causes that IPC to be sent.) #3 seems to be distributed across FaviconService (decoding) and FaviconHandler (for stashing in NavigationController), so yeah as per my comment above it's probably easiest if we just roll that bit ourselves. (hoping there's no hidden gotchas in it) For reference, the callbacks we ultimately need to generate are http://developer.android.com/reference/android/webkit/WebChromeClient.html onReceivedIcon(android.webkit.WebView, android.graphics.Bitmap) onReceivedTouchIconUrl(android.webkit.WebView, java.lang.String, boolean) - note the former passes the decoded Bitmap, the latter just passes the icon URL :-)
On 2012/10/18 21:51:13, joth wrote: > http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.h:34: static void > CreateForWebContentsAndDelegate( > nit: I think the AndDelegate is spurious here, and potentially misleading: this > method will create at most one helper instance per web contents, and bind the > lifetime of the helper to that WebContents instance (only). The delegate doesn't > effect this logic. (indeed if you call this method a multiple time for a given > web_contents but with a different delegate, the calls are ignored. perhaps we > should assert against that, or if really necessary, update the existing helper > to point to new delegate) > > Hmm OK "git grep CreateForWebContentsAnd" tells me the boat may already have sailed on this one. I'd still argue that my point above is still valid one though, and it would be worth considering fixing up the existing examples:)
http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.cc (right): http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:56: int id = FaviconUtil::DownloadFavicon(host, url, image_size); On 2012/10/18 21:51:13, joth wrote: > internally this sends IconMsg_DownloadFavicon ipc which is handled by > chrome/renderer/chrome_render_view_observer.cc which we don't (or, won't) have > installed in webview, so some of the renderer side code also needs extracting I > think. > CL extracting renderer-side code: http://codereview.chromium.org/11232068/ > working back, this line adds a dependency on FaviconUtil which itself pulls in > chrome/browser/history... types. So I think we'll need to unpick those too. > (maybe, just remove DownloadFavicon from FaviconUtil and do it right here?) > Unfortunately, there are a couple places in (mostly in ash) which call FaviconUtil::DownloadFavicon directly, so I can't remove it just yet. Once this CL has landed, I'll try to switch them over to using the FaviconDownloadHelper/Delegate model (and move DownloadFavicon into the helper). http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:43: // been retrieved. On 2012/10/18 21:51:13, joth wrote: > could mention what |image_size| is. (is it a _max_size?) > (I assume it doesn't need to be int64? :-) Added a description of image_size. It most likely could be an int32, but that is not what is used elsewhere in the favicon code when dealing with image sizes, so I am a bit hesitant to change it here. http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:69: DISALLOW_COPY_AND_ASSIGN(FaviconDownloadHelper); On 2012/10/18 21:51:13, joth wrote: > think this can be DISALLOW_IMPLICIT_CONSTRUCTORS Done. http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.cc (right): http://codereview.chromium.org/11195010/diff/12001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.cc:136: void FaviconTabHelper::OnUpdateFaviconURL( On 2012/10/18 21:51:13, joth wrote: > hmmm OK I'm just re-reminding myself how the whole favicon flow works. > What we are after is the ability to generate a callback with the page favicon in > response to each call to top-level call to WebView.loadUrl (i.e. per > WebContents::OpenURL) > > This requires at least two parts > 1/ load content for page_url and parse it for <head><link rel="icon".. tag. > (falling back to site-wide icon). This yeilds icon_url. (and possibly, a > touch-icon URL, which we can pass to the application unaltered) > 2/ make download request for icon_url > (3/ decode the received data to an Image, and stash it inside > NavigationController.) > > With this patch, I think FaviconDownloadHelper::DownloadFavicon would only give > us #2 of that list. > > To get #1, we need to have this code to handle IconHostMsg_UpdateFaviconURL > (and of course also need the associated renderer-side code that causes that IPC > to be sent.) > FaviconDownloadHelper should now handle IconHostMsg_UpdateFaviconURL messages. And the renderer-side code to send the message is in: http://codereview.chromium.org/11232068/ > #3 seems to be distributed across FaviconService (decoding) and FaviconHandler > (for stashing in NavigationController), so yeah as per my comment above it's > probably easiest if we just roll that bit ourselves. (hoping there's no hidden > gotchas in it) > > > For reference, the callbacks we ultimately need to generate are > > http://developer.android.com/reference/android/webkit/WebChromeClient.html > onReceivedIcon(android.webkit.WebView, android.graphics.Bitmap) > onReceivedTouchIconUrl(android.webkit.WebView, java.lang.String, boolean) > > - note the former passes the decoded Bitmap, the latter just passes the icon URL > :-) > >
What is the rationale behind this change? http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:32: public base::RefCounted<FaviconDownloadHelper> { Why is this ref counted? http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.cc (right): http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { return helper ? helper->... : -1; http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... File chrome/browser/ui/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... chrome/browser/ui/tab_contents/tab_contents.cc:136: FaviconTabHelper::FromWebContents(contents)); Doesn't doing this lead to the possibility of FaviconTabHelper being destroyed before FaviconDownloadHelper?
The rationale behind this change is to extract the favicon downloading code from the FaviconTabHelper (which requires a HistroyService, FaviconService etc.) so that the WebView team can reuse the code without having to depend on these services. http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:32: public base::RefCounted<FaviconDownloadHelper> { On 2012/10/24 16:38:10, sky wrote: > Why is this ref counted? It needs to be ref counted because it uses the UserDataAdapter, which expects a ref-counted object. http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.cc (right): http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { On 2012/10/24 16:38:10, sky wrote: > return helper ? helper->... : -1; Done. http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... File chrome/browser/ui/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... chrome/browser/ui/tab_contents/tab_contents.cc:136: FaviconTabHelper::FromWebContents(contents)); On 2012/10/24 16:38:10, sky wrote: > Doesn't doing this lead to the possibility of FaviconTabHelper being destroyed > before FaviconDownloadHelper? That is a possibility. The FaviconDownloadHelper only keeps a weak pointer to its delegate, though. The other option would be to have a separate SetDelegate method, but seeing as the FaviconDownloadHelper's main purpose is to relay data back to the delegate, there is not much point in creating an instance of it without a delegate...
I like the idea of promoting this code, but I tend to think we should try and promote it all. The delegate should provide the hooks for getting the favicon from history. That way the chrome side hooks up to historyservice and other embedders hook it to whatever they need. If an embedder isn't caching favicons the return code could indicate this (or maybe no delegate). -Scott On Wed, Oct 24, 2012 at 10:31 AM, <caitkp@chromium.org> wrote: > The rationale behind this change is to extract the favicon downloading code > from > the FaviconTabHelper (which requires a HistroyService, FaviconService etc.) > so > that the WebView team can reuse the code without having to depend on these > services. > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > File chrome/browser/favicon/favicon_download_helper.h (right): > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.h:32: public > base::RefCounted<FaviconDownloadHelper> { > On 2012/10/24 16:38:10, sky wrote: >> >> Why is this ref counted? > > It needs to be ref counted because it uses the UserDataAdapter, which > expects a ref-counted object. > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > File chrome/browser/favicon/favicon_tab_helper.cc (right): > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { > On 2012/10/24 16:38:10, sky wrote: >> >> return helper ? helper->... : -1; > > > Done. > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > File chrome/browser/ui/tab_contents/tab_contents.cc (right): > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > chrome/browser/ui/tab_contents/tab_contents.cc:136: > FaviconTabHelper::FromWebContents(contents)); > On 2012/10/24 16:38:10, sky wrote: >> >> Doesn't doing this lead to the possibility of FaviconTabHelper being > > destroyed >> >> before FaviconDownloadHelper? > > That is a possibility. The FaviconDownloadHelper only keeps a weak > pointer to its delegate, though. > > The other option would be to have a separate SetDelegate method, but > seeing as the FaviconDownloadHelper's main purpose is to relay data > back to the delegate, there is not much point in creating an instance of > it without a delegate... > > http://codereview.chromium.org/11195010/
I agree that in future it would make sense for the delegate to handle the history case as well as the download case. Jonathan is better equipped than I to comment on whether WebView would be able to make use of this feature. In the interim, I would like to push forward with the FaviconDownloadHelper, so it is not blocking WebView. There are also a couple places in Chrome which could be converted to using it and remove a lot of boilerplate message handling code: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... and http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... On 2012/10/24 20:58:34, sky wrote: > I like the idea of promoting this code, but I tend to think we should > try and promote it all. The delegate should provide the hooks for > getting the favicon from history. That way the chrome side hooks up to > historyservice and other embedders hook it to whatever they need. If > an embedder isn't caching favicons the return code could indicate this > (or maybe no delegate). > > -Scott > > On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> wrote: > > The rationale behind this change is to extract the favicon downloading code > > from > > the FaviconTabHelper (which requires a HistroyService, FaviconService etc.) > > so > > that the WebView team can reuse the code without having to depend on these > > services. > > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > > File chrome/browser/favicon/favicon_download_helper.h (right): > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > > chrome/browser/favicon/favicon_download_helper.h:32: public > > base::RefCounted<FaviconDownloadHelper> { > > On 2012/10/24 16:38:10, sky wrote: > >> > >> Why is this ref counted? > > > > It needs to be ref counted because it uses the UserDataAdapter, which > > expects a ref-counted object. > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > > File chrome/browser/favicon/favicon_tab_helper.cc (right): > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { > > On 2012/10/24 16:38:10, sky wrote: > >> > >> return helper ? helper->... : -1; > > > > > > Done. > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > > File chrome/browser/ui/tab_contents/tab_contents.cc (right): > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > > chrome/browser/ui/tab_contents/tab_contents.cc:136: > > FaviconTabHelper::FromWebContents(contents)); > > On 2012/10/24 16:38:10, sky wrote: > >> > >> Doesn't doing this lead to the possibility of FaviconTabHelper being > > > > destroyed > >> > >> before FaviconDownloadHelper? > > > > That is a possibility. The FaviconDownloadHelper only keeps a weak > > pointer to its delegate, though. > > > > The other option would be to have a separate SetDelegate method, but > > seeing as the FaviconDownloadHelper's main purpose is to relay data > > back to the delegate, there is not much point in creating an instance of > > it without a delegate... > > > > http://codereview.chromium.org/11195010/
Jonathan, do you want the history feature? -Scott On Thu, Oct 25, 2012 at 8:59 AM, <caitkp@chromium.org> wrote: > I agree that in future it would make sense for the delegate to handle the > history case as well as the download case. Jonathan is better equipped than > I to > comment on whether WebView would be able to make use of this feature. > > In the interim, I would like to push forward with the FaviconDownloadHelper, > so > it is not blocking WebView. There are also a couple places in Chrome which > could > be converted to using it and remove a lot of boilerplate message handling > code: > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... > and > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... > > > On 2012/10/24 20:58:34, sky wrote: >> >> I like the idea of promoting this code, but I tend to think we should >> try and promote it all. The delegate should provide the hooks for >> getting the favicon from history. That way the chrome side hooks up to >> historyservice and other embedders hook it to whatever they need. If >> an embedder isn't caching favicons the return code could indicate this >> (or maybe no delegate). > > >> -Scott > > >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> wrote: >> > The rationale behind this change is to extract the favicon downloading >> > code >> > from >> > the FaviconTabHelper (which requires a HistroyService, FaviconService >> > etc.) >> > so >> > that the WebView team can reuse the code without having to depend on >> > these >> > services. >> > >> > >> > >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> > File chrome/browser/favicon/favicon_download_helper.h (right): >> > >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> > chrome/browser/favicon/favicon_download_helper.h:32: public >> > base::RefCounted<FaviconDownloadHelper> { >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> Why is this ref counted? >> > >> > It needs to be ref counted because it uses the UserDataAdapter, which >> > expects a ref-counted object. >> > >> > >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): >> > >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> return helper ? helper->... : -1; >> > >> > >> > Done. >> > >> > >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >> >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): >> > >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >> >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: >> > FaviconTabHelper::FromWebContents(contents)); >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> Doesn't doing this lead to the possibility of FaviconTabHelper being >> > >> > destroyed >> >> >> >> before FaviconDownloadHelper? >> > >> > That is a possibility. The FaviconDownloadHelper only keeps a weak >> > pointer to its delegate, though. >> > >> > The other option would be to have a separate SetDelegate method, but >> > seeing as the FaviconDownloadHelper's main purpose is to relay data >> > back to the delegate, there is not much point in creating an instance of >> > it without a delegate... >> > >> > http://codereview.chromium.org/11195010/ > > > > http://codereview.chromium.org/11195010/
On 25 October 2012 09:31, Scott Violet <sky@chromium.org> wrote: > Jonathan, do you want the history feature? > > Just to be super clear- are we talking about the feature that populate the back-froward history (i.e. to make NavigationController->GetActiveItem()->GetFavicon() work correctly) or the persistent history as part of Profile? (I've seen both referred to as history in different contexts, but assuming it's the latter here) Yes - while It's not an absolute requirement for us, we could optionally use it, if a version existed without onerous chrome/ layer dependencies. (AIUI this would allow us to implement a minimal working version of http://developer.android.com/reference/android/webkit/WebIconDatabase.html to 'cache' download icons) We're definitely keen to get the basic favicon support in place as a higher priority, but no need to rush it through on our account if the full helper extraction is generally preferred. > -Scott > > On Thu, Oct 25, 2012 at 8:59 AM, <caitkp@chromium.org> wrote: > > I agree that in future it would make sense for the delegate to handle the > > history case as well as the download case. Jonathan is better equipped > than > > I to > > comment on whether WebView would be able to make use of this feature. > > > > In the interim, I would like to push forward with the > FaviconDownloadHelper, > > so > > it is not blocking WebView. There are also a couple places in Chrome > which > > could > > be converted to using it and remove a lot of boilerplate message handling > > code: > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... > > and > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... > > > > > > On 2012/10/24 20:58:34, sky wrote: > >> > >> I like the idea of promoting this code, but I tend to think we should > >> try and promote it all. The delegate should provide the hooks for > >> getting the favicon from history. That way the chrome side hooks up to > >> historyservice and other embedders hook it to whatever they need. If > >> an embedder isn't caching favicons the return code could indicate this > >> (or maybe no delegate). > > > > > >> -Scott > > > > > >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> wrote: > >> > The rationale behind this change is to extract the favicon downloading > >> > code > >> > from > >> > the FaviconTabHelper (which requires a HistroyService, FaviconService > >> > etc.) > >> > so > >> > that the WebView team can reuse the code without having to depend on > >> > these > >> > services. > >> > > >> > > >> > > >> > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> > >> > File chrome/browser/favicon/favicon_download_helper.h (right): > >> > > >> > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> > >> > chrome/browser/favicon/favicon_download_helper.h:32: public > >> > base::RefCounted<FaviconDownloadHelper> { > >> > On 2012/10/24 16:38:10, sky wrote: > >> >> > >> >> Why is this ref counted? > >> > > >> > It needs to be ref counted because it uses the UserDataAdapter, which > >> > expects a ref-counted object. > >> > > >> > > >> > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> > >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): > >> > > >> > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> > >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { > >> > On 2012/10/24 16:38:10, sky wrote: > >> >> > >> >> return helper ? helper->... : -1; > >> > > >> > > >> > Done. > >> > > >> > > >> > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > >> > >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): > >> > > >> > > > > > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > >> > >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: > >> > FaviconTabHelper::FromWebContents(contents)); > >> > On 2012/10/24 16:38:10, sky wrote: > >> >> > >> >> Doesn't doing this lead to the possibility of FaviconTabHelper being > >> > > >> > destroyed > >> >> > >> >> before FaviconDownloadHelper? > >> > > >> > That is a possibility. The FaviconDownloadHelper only keeps a weak > >> > pointer to its delegate, though. > >> > > >> > The other option would be to have a separate SetDelegate method, but > >> > seeing as the FaviconDownloadHelper's main purpose is to relay data > >> > back to the delegate, there is not much point in creating an instance > of > >> > it without a delegate... > >> > > >> > http://codereview.chromium.org/11195010/ > > > > > > > > http://codereview.chromium.org/11195010/ >
On Thu, Oct 25, 2012 at 3:21 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 25 October 2012 09:31, Scott Violet <sky@chromium.org> wrote: >> >> Jonathan, do you want the history feature? >> > > Just to be super clear- are we talking about the feature that populate the > back-froward history (i.e. to make > NavigationController->GetActiveItem()->GetFavicon() work correctly) or the > persistent history as part of Profile? (I've seen both referred to as > history in different contexts, but assuming it's the latter here) In this case we're referring to the latter, persisting favicons to some sort of long term storage. Once you have that you can implement back-forward history, but it's separate than what we're referring to here. -Scott > > > Yes - while It's not an absolute requirement for us, we could optionally > use it, if a version existed without onerous chrome/ layer dependencies. > (AIUI this would allow us to implement a minimal working version of > http://developer.android.com/reference/android/webkit/WebIconDatabase.html > to 'cache' download icons) > > We're definitely keen to get the basic favicon support in place as a higher > priority, but no need to rush it through on our account if the full helper > extraction is generally preferred. > > >> >> -Scott >> >> On Thu, Oct 25, 2012 at 8:59 AM, <caitkp@chromium.org> wrote: >> > I agree that in future it would make sense for the delegate to handle >> > the >> > history case as well as the download case. Jonathan is better equipped >> > than >> > I to >> > comment on whether WebView would be able to make use of this feature. >> > >> > In the interim, I would like to push forward with the >> > FaviconDownloadHelper, >> > so >> > it is not blocking WebView. There are also a couple places in Chrome >> > which >> > could >> > be converted to using it and remove a lot of boilerplate message >> > handling >> > code: >> > >> > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... >> > and >> > >> > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... >> > >> > >> > On 2012/10/24 20:58:34, sky wrote: >> >> >> >> I like the idea of promoting this code, but I tend to think we should >> >> try and promote it all. The delegate should provide the hooks for >> >> getting the favicon from history. That way the chrome side hooks up to >> >> historyservice and other embedders hook it to whatever they need. If >> >> an embedder isn't caching favicons the return code could indicate this >> >> (or maybe no delegate). >> > >> > >> >> -Scott >> > >> > >> >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> wrote: >> >> > The rationale behind this change is to extract the favicon >> >> > downloading >> >> > code >> >> > from >> >> > the FaviconTabHelper (which requires a HistroyService, FaviconService >> >> > etc.) >> >> > so >> >> > that the WebView team can reuse the code without having to depend on >> >> > these >> >> > services. >> >> > >> >> > >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> > File chrome/browser/favicon/favicon_download_helper.h (right): >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> > chrome/browser/favicon/favicon_download_helper.h:32: public >> >> > base::RefCounted<FaviconDownloadHelper> { >> >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> >> >> Why is this ref counted? >> >> > >> >> > It needs to be ref counted because it uses the UserDataAdapter, which >> >> > expects a ref-counted object. >> >> > >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { >> >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> >> >> return helper ? helper->... : -1; >> >> > >> >> > >> >> > Done. >> >> > >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >> >> >> >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >> >> >> >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: >> >> > FaviconTabHelper::FromWebContents(contents)); >> >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> >> >> Doesn't doing this lead to the possibility of FaviconTabHelper being >> >> > >> >> > destroyed >> >> >> >> >> >> before FaviconDownloadHelper? >> >> > >> >> > That is a possibility. The FaviconDownloadHelper only keeps a weak >> >> > pointer to its delegate, though. >> >> > >> >> > The other option would be to have a separate SetDelegate method, but >> >> > seeing as the FaviconDownloadHelper's main purpose is to relay data >> >> > back to the delegate, there is not much point in creating an instance >> >> > of >> >> > it without a delegate... >> >> > >> >> > http://codereview.chromium.org/11195010/ >> > >> > >> > >> > http://codereview.chromium.org/11195010/ > >
I looked into this a bit on Friday, and I don't think extracting the history-related functions will be as straight-forward as the download case. ATM, there is a lot of back and forth between the HistoryService and FaviconHandler, as the service is used to look up favicon urls for page urls, as well as to fetch favicons, so delegating could be difficult. On 2012/10/26 19:32:26, sky wrote: > On Thu, Oct 25, 2012 at 3:21 PM, Jonathan Dixon <mailto:joth@chromium.org> wrote: > > > > > > > > On 25 October 2012 09:31, Scott Violet <mailto:sky@chromium.org> wrote: > >> > >> Jonathan, do you want the history feature? > >> > > > > Just to be super clear- are we talking about the feature that populate the > > back-froward history (i.e. to make > > NavigationController->GetActiveItem()->GetFavicon() work correctly) or the > > persistent history as part of Profile? (I've seen both referred to as > > history in different contexts, but assuming it's the latter here) > > In this case we're referring to the latter, persisting favicons to > some sort of long term storage. Once you have that you can implement > back-forward history, but it's separate than what we're referring to > here. > > -Scott > > > > > > > Yes - while It's not an absolute requirement for us, we could optionally > > use it, if a version existed without onerous chrome/ layer dependencies. > > (AIUI this would allow us to implement a minimal working version of > > http://developer.android.com/reference/android/webkit/WebIconDatabase.html > > to 'cache' download icons) > > > > We're definitely keen to get the basic favicon support in place as a higher > > priority, but no need to rush it through on our account if the full helper > > extraction is generally preferred. > > > > > >> > >> -Scott > >> > >> On Thu, Oct 25, 2012 at 8:59 AM, <mailto:caitkp@chromium.org> wrote: > >> > I agree that in future it would make sense for the delegate to handle > >> > the > >> > history case as well as the download case. Jonathan is better equipped > >> > than > >> > I to > >> > comment on whether WebView would be able to make use of this feature. > >> > > >> > In the interim, I would like to push forward with the > >> > FaviconDownloadHelper, > >> > so > >> > it is not blocking WebView. There are also a couple places in Chrome > >> > which > >> > could > >> > be converted to using it and remove a lot of boilerplate message > >> > handling > >> > code: > >> > > >> > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... > >> > and > >> > > >> > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... > >> > > >> > > >> > On 2012/10/24 20:58:34, sky wrote: > >> >> > >> >> I like the idea of promoting this code, but I tend to think we should > >> >> try and promote it all. The delegate should provide the hooks for > >> >> getting the favicon from history. That way the chrome side hooks up to > >> >> historyservice and other embedders hook it to whatever they need. If > >> >> an embedder isn't caching favicons the return code could indicate this > >> >> (or maybe no delegate). > >> > > >> > > >> >> -Scott > >> > > >> > > >> >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> wrote: > >> >> > The rationale behind this change is to extract the favicon > >> >> > downloading > >> >> > code > >> >> > from > >> >> > the FaviconTabHelper (which requires a HistroyService, FaviconService > >> >> > etc.) > >> >> > so > >> >> > that the WebView team can reuse the code without having to depend on > >> >> > these > >> >> > services. > >> >> > > >> >> > > >> >> > > >> >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> >> > >> >> > File chrome/browser/favicon/favicon_download_helper.h (right): > >> >> > > >> >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> >> > >> >> > chrome/browser/favicon/favicon_download_helper.h:32: public > >> >> > base::RefCounted<FaviconDownloadHelper> { > >> >> > On 2012/10/24 16:38:10, sky wrote: > >> >> >> > >> >> >> Why is this ref counted? > >> >> > > >> >> > It needs to be ref counted because it uses the UserDataAdapter, which > >> >> > expects a ref-counted object. > >> >> > > >> >> > > >> >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> >> > >> >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): > >> >> > > >> >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... > >> >> > >> >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { > >> >> > On 2012/10/24 16:38:10, sky wrote: > >> >> >> > >> >> >> return helper ? helper->... : -1; > >> >> > > >> >> > > >> >> > Done. > >> >> > > >> >> > > >> >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > >> >> > >> >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): > >> >> > > >> >> > > >> > > >> > > >> > > >> > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... > >> >> > >> >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: > >> >> > FaviconTabHelper::FromWebContents(contents)); > >> >> > On 2012/10/24 16:38:10, sky wrote: > >> >> >> > >> >> >> Doesn't doing this lead to the possibility of FaviconTabHelper being > >> >> > > >> >> > destroyed > >> >> >> > >> >> >> before FaviconDownloadHelper? > >> >> > > >> >> > That is a possibility. The FaviconDownloadHelper only keeps a weak > >> >> > pointer to its delegate, though. > >> >> > > >> >> > The other option would be to have a separate SetDelegate method, but > >> >> > seeing as the FaviconDownloadHelper's main purpose is to relay data > >> >> > back to the delegate, there is not much point in creating an instance > >> >> > of > >> >> > it without a delegate... > >> >> > > >> >> > http://codereview.chromium.org/11195010/ > >> > > >> > > >> > > >> > http://codereview.chromium.org/11195010/ > > > >
I don't doubt it's more complex, but it's the right thing to do long term. -Scott On Mon, Oct 29, 2012 at 8:23 AM, <caitkp@chromium.org> wrote: > I looked into this a bit on Friday, and I don't think extracting the > history-related functions will be as straight-forward as the download case. > ATM, > there is a lot of back and forth between the HistoryService and > FaviconHandler, > as the service is used to look up favicon urls for page urls, as well as to > fetch favicons, so delegating could be difficult. > > On 2012/10/26 19:32:26, sky wrote: >> >> On Thu, Oct 25, 2012 at 3:21 PM, Jonathan Dixon <mailto:joth@chromium.org> > > wrote: > >> > >> > >> > >> > On 25 October 2012 09:31, Scott Violet <mailto:sky@chromium.org> wrote: >> >> >> >> Jonathan, do you want the history feature? >> >> >> > >> > Just to be super clear- are we talking about the feature that populate >> > the >> > back-froward history (i.e. to make >> > NavigationController->GetActiveItem()->GetFavicon() work correctly) or >> > the >> > persistent history as part of Profile? (I've seen both referred to as >> > history in different contexts, but assuming it's the latter here) > > >> In this case we're referring to the latter, persisting favicons to >> some sort of long term storage. Once you have that you can implement >> back-forward history, but it's separate than what we're referring to >> here. > > >> -Scott > > >> > >> > >> > Yes - while It's not an absolute requirement for us, we could >> > optionally >> > use it, if a version existed without onerous chrome/ layer dependencies. >> > (AIUI this would allow us to implement a minimal working version of >> > >> > http://developer.android.com/reference/android/webkit/WebIconDatabase.html >> > to 'cache' download icons) >> > >> > We're definitely keen to get the basic favicon support in place as a >> > higher >> > priority, but no need to rush it through on our account if the full >> > helper >> > extraction is generally preferred. >> > >> > >> >> >> >> -Scott >> >> >> >> On Thu, Oct 25, 2012 at 8:59 AM, <mailto:caitkp@chromium.org> wrote: >> >> > I agree that in future it would make sense for the delegate to handle >> >> > the >> >> > history case as well as the download case. Jonathan is better >> >> > equipped >> >> > than >> >> > I to >> >> > comment on whether WebView would be able to make use of this feature. >> >> > >> >> > In the interim, I would like to push forward with the >> >> > FaviconDownloadHelper, >> >> > so >> >> > it is not blocking WebView. There are also a couple places in Chrome >> >> > which >> >> > could >> >> > be converted to using it and remove a lot of boilerplate message >> >> > handling >> >> > code: >> >> > >> >> > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... >> >> >> > and >> >> > >> >> > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... >> >> >> > >> >> > >> >> > On 2012/10/24 20:58:34, sky wrote: >> >> >> >> >> >> I like the idea of promoting this code, but I tend to think we >> >> >> should >> >> >> try and promote it all. The delegate should provide the hooks for >> >> >> getting the favicon from history. That way the chrome side hooks up >> >> >> to >> >> >> historyservice and other embedders hook it to whatever they need. If >> >> >> an embedder isn't caching favicons the return code could indicate >> >> >> this >> >> >> (or maybe no delegate). >> >> > >> >> > >> >> >> -Scott >> >> > >> >> > >> >> >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> >> >> >> wrote: >> >> >> > The rationale behind this change is to extract the favicon >> >> >> > downloading >> >> >> > code >> >> >> > from >> >> >> > the FaviconTabHelper (which requires a HistroyService, >> >> >> > FaviconService >> >> >> > etc.) >> >> >> > so >> >> >> > that the WebView team can reuse the code without having to depend >> >> >> > on >> >> >> > these >> >> >> > services. >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> >> >> >> > File chrome/browser/favicon/favicon_download_helper.h (right): >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> >> >> >> > chrome/browser/favicon/favicon_download_helper.h:32: public >> >> >> > base::RefCounted<FaviconDownloadHelper> { >> >> >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> >> >> >> >> Why is this ref counted? >> >> >> > >> >> >> > It needs to be ref counted because it uses the UserDataAdapter, >> >> >> > which >> >> >> > expects a ref-counted object. >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> >> >> >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >> >> >> >> >> >> >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { >> >> >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> >> >> >> >> return helper ? helper->... : -1; >> >> >> > >> >> >> > >> >> >> > Done. >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >> >> >> >> >> >> >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> >> > > > > http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >> >> >> >> >> >> >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: >> >> >> > FaviconTabHelper::FromWebContents(contents)); >> >> >> > On 2012/10/24 16:38:10, sky wrote: >> >> >> >> >> >> >> >> Doesn't doing this lead to the possibility of FaviconTabHelper >> >> >> >> being >> >> >> > >> >> >> > destroyed >> >> >> >> >> >> >> >> before FaviconDownloadHelper? >> >> >> > >> >> >> > That is a possibility. The FaviconDownloadHelper only keeps a weak >> >> >> > pointer to its delegate, though. >> >> >> > >> >> >> > The other option would be to have a separate SetDelegate method, >> >> >> > but >> >> >> > seeing as the FaviconDownloadHelper's main purpose is to relay >> >> >> > data >> >> >> > back to the delegate, there is not much point in creating an >> >> >> > instance >> >> >> > of >> >> >> > it without a delegate... >> >> >> > >> >> >> > http://codereview.chromium.org/11195010/ >> >> > >> >> > >> >> > >> >> > http://codereview.chromium.org/11195010/ >> > >> > > > > > > http://codereview.chromium.org/11195010/
Let me jump in here for a moment to give some background. The current primary goal for Browser Components, as requested by Ben Goodger as well as John Abd-el-Malek, is to extract components out of src/chrome in kind of a "leaves first" order, to the extent needed so that a lot of the src/chrome/browser/chromeos stuff can also move to src/chromeos (i.e. the non-browser stuff that lives there). For these, we're trying to do them in an order that makes sense, i.e. we're starting with things that have few dependencies (Prefs), then those that depend only on those extracted already (e.g. WebData which primarily depends on Prefs and sync, which is already largely extracted) and so forth. Extracting stuff that Android WebView needs is a secondary goal for us, and the Favicon work is one of the things needed for Android WebView. To leave as much time as possible for the primary goal, I'd like to do whatever (a) fulfills WebView's needs, (b) is clean, and (c) does not cost significantly more to finish later, once we get there. Note that our primary goal should bring us to componentizing a lot of src/chrome/ in the end, and the dependency graph will have been simplified by the time we get to this bit, thus making the work easier. I think the work needed to extract the history-related stuff fits all of the criteria I mentioned, and may even benefit from the work we're doing as part of our primary goals, so we'd like to defer it for now, to free up some time for our primary goals as well as any high-priority WebView needs that may come up during the rest of the quarter. The change so far is already better than what was there before, and fulfills an immediate need. Cheers, Jói On Mon, Oct 29, 2012 at 6:09 PM, Scott Violet <sky@chromium.org> wrote: > I don't doubt it's more complex, but it's the right thing to do long term. > > -Scott > > On Mon, Oct 29, 2012 at 8:23 AM, <caitkp@chromium.org> wrote: >> I looked into this a bit on Friday, and I don't think extracting the >> history-related functions will be as straight-forward as the download case. >> ATM, >> there is a lot of back and forth between the HistoryService and >> FaviconHandler, >> as the service is used to look up favicon urls for page urls, as well as to >> fetch favicons, so delegating could be difficult. >> >> On 2012/10/26 19:32:26, sky wrote: >>> >>> On Thu, Oct 25, 2012 at 3:21 PM, Jonathan Dixon <mailto:joth@chromium.org> >> >> wrote: >> >>> > >>> > >>> > >>> > On 25 October 2012 09:31, Scott Violet <mailto:sky@chromium.org> wrote: >>> >> >>> >> Jonathan, do you want the history feature? >>> >> >>> > >>> > Just to be super clear- are we talking about the feature that populate >>> > the >>> > back-froward history (i.e. to make >>> > NavigationController->GetActiveItem()->GetFavicon() work correctly) or >>> > the >>> > persistent history as part of Profile? (I've seen both referred to as >>> > history in different contexts, but assuming it's the latter here) >> >> >>> In this case we're referring to the latter, persisting favicons to >>> some sort of long term storage. Once you have that you can implement >>> back-forward history, but it's separate than what we're referring to >>> here. >> >> >>> -Scott >> >> >>> > >>> > >>> > Yes - while It's not an absolute requirement for us, we could >>> > optionally >>> > use it, if a version existed without onerous chrome/ layer dependencies. >>> > (AIUI this would allow us to implement a minimal working version of >>> > >>> > http://developer.android.com/reference/android/webkit/WebIconDatabase.html >>> > to 'cache' download icons) >>> > >>> > We're definitely keen to get the basic favicon support in place as a >>> > higher >>> > priority, but no need to rush it through on our account if the full >>> > helper >>> > extraction is generally preferred. >>> > >>> > >>> >> >>> >> -Scott >>> >> >>> >> On Thu, Oct 25, 2012 at 8:59 AM, <mailto:caitkp@chromium.org> wrote: >>> >> > I agree that in future it would make sense for the delegate to handle >>> >> > the >>> >> > history case as well as the download case. Jonathan is better >>> >> > equipped >>> >> > than >>> >> > I to >>> >> > comment on whether WebView would be able to make use of this feature. >>> >> > >>> >> > In the interim, I would like to push forward with the >>> >> > FaviconDownloadHelper, >>> >> > so >>> >> > it is not blocking WebView. There are also a couple places in Chrome >>> >> > which >>> >> > could >>> >> > be converted to using it and remove a lot of boilerplate message >>> >> > handling >>> >> > code: >>> >> > >>> >> > >> >> >> http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... >>> >>> >> > and >>> >> > >>> >> > >> >> >> http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... >>> >>> >> > >>> >> > >>> >> > On 2012/10/24 20:58:34, sky wrote: >>> >> >> >>> >> >> I like the idea of promoting this code, but I tend to think we >>> >> >> should >>> >> >> try and promote it all. The delegate should provide the hooks for >>> >> >> getting the favicon from history. That way the chrome side hooks up >>> >> >> to >>> >> >> historyservice and other embedders hook it to whatever they need. If >>> >> >> an embedder isn't caching favicons the return code could indicate >>> >> >> this >>> >> >> (or maybe no delegate). >>> >> > >>> >> > >>> >> >> -Scott >>> >> > >>> >> > >>> >> >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> >>> >> >> wrote: >>> >> >> > The rationale behind this change is to extract the favicon >>> >> >> > downloading >>> >> >> > code >>> >> >> > from >>> >> >> > the FaviconTabHelper (which requires a HistroyService, >>> >> >> > FaviconService >>> >> >> > etc.) >>> >> >> > so >>> >> >> > that the WebView team can reuse the code without having to depend >>> >> >> > on >>> >> >> > these >>> >> >> > services. >>> >> >> > >>> >> >> > >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> >> > >>> >> > >> >> >> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>> >>> >> >> >>> >> >> > File chrome/browser/favicon/favicon_download_helper.h (right): >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> >> > >>> >> > >> >> >> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>> >>> >> >> >>> >> >> > chrome/browser/favicon/favicon_download_helper.h:32: public >>> >> >> > base::RefCounted<FaviconDownloadHelper> { >>> >> >> > On 2012/10/24 16:38:10, sky wrote: >>> >> >> >> >>> >> >> >> Why is this ref counted? >>> >> >> > >>> >> >> > It needs to be ref counted because it uses the UserDataAdapter, >>> >> >> > which >>> >> >> > expects a ref-counted object. >>> >> >> > >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> >> > >>> >> > >> >> >> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>> >>> >> >> >>> >> >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> >> > >>> >> > >> >> >> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>> >>> >> >> >>> >> >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { >>> >> >> > On 2012/10/24 16:38:10, sky wrote: >>> >> >> >> >>> >> >> >> return helper ? helper->... : -1; >>> >> >> > >>> >> >> > >>> >> >> > Done. >>> >> >> > >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> >> > >>> >> > >> >> >> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >>> >>> >> >> >>> >> >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> >> > >>> >> > >> >> >> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >>> >>> >> >> >>> >> >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: >>> >> >> > FaviconTabHelper::FromWebContents(contents)); >>> >> >> > On 2012/10/24 16:38:10, sky wrote: >>> >> >> >> >>> >> >> >> Doesn't doing this lead to the possibility of FaviconTabHelper >>> >> >> >> being >>> >> >> > >>> >> >> > destroyed >>> >> >> >> >>> >> >> >> before FaviconDownloadHelper? >>> >> >> > >>> >> >> > That is a possibility. The FaviconDownloadHelper only keeps a weak >>> >> >> > pointer to its delegate, though. >>> >> >> > >>> >> >> > The other option would be to have a separate SetDelegate method, >>> >> >> > but >>> >> >> > seeing as the FaviconDownloadHelper's main purpose is to relay >>> >> >> > data >>> >> >> > back to the delegate, there is not much point in creating an >>> >> >> > instance >>> >> >> > of >>> >> >> > it without a delegate... >>> >> >> > >>> >> >> > http://codereview.chromium.org/11195010/ >>> >> > >>> >> > >>> >> > >>> >> > http://codereview.chromium.org/11195010/ >>> > >>> > >> >> >> >> >> http://codereview.chromium.org/11195010/
My original thinking is all consumers of favicons want the history part so that it makes no sense to promote FaviconDownloadHelper . But I think I'm wrong there. FaviconTabHelper::DownloadImage() should not be part of FaviconTabHelper and callers should instead use FaviconDownloadHelper. Additionally there are other places that want to download favicons ( http://codereview.chromium.org/11306005/ ). They would benefit from FaviconDownloadHelper . -Scott On Mon, Oct 29, 2012 at 2:02 PM, Jói Sigurðsson <joi@chromium.org> wrote: > Let me jump in here for a moment to give some background. > > The current primary goal for Browser Components, as requested by Ben > Goodger as well as John Abd-el-Malek, is to extract components out of > src/chrome in kind of a "leaves first" order, to the extent needed so > that a lot of the src/chrome/browser/chromeos stuff can also move to > src/chromeos (i.e. the non-browser stuff that lives there). For > these, we're trying to do them in an order that makes sense, i.e. > we're starting with things that have few dependencies (Prefs), then > those that depend only on those extracted already (e.g. WebData which > primarily depends on Prefs and sync, which is already largely > extracted) and so forth. > > Extracting stuff that Android WebView needs is a secondary goal for > us, and the Favicon work is one of the things needed for Android > WebView. To leave as much time as possible for the primary goal, I'd > like to do whatever (a) fulfills WebView's needs, (b) is clean, and > (c) does not cost significantly more to finish later, once we get > there. Note that our primary goal should bring us to componentizing a > lot of src/chrome/ in the end, and the dependency graph will have been > simplified by the time we get to this bit, thus making the work > easier. > > I think the work needed to extract the history-related stuff fits all > of the criteria I mentioned, and may even benefit from the work we're > doing as part of our primary goals, so we'd like to defer it for now, > to free up some time for our primary goals as well as any > high-priority WebView needs that may come up during the rest of the > quarter. The change so far is already better than what was there > before, and fulfills an immediate need. > > Cheers, > Jói > > > On Mon, Oct 29, 2012 at 6:09 PM, Scott Violet <sky@chromium.org> wrote: >> I don't doubt it's more complex, but it's the right thing to do long term. >> >> -Scott >> >> On Mon, Oct 29, 2012 at 8:23 AM, <caitkp@chromium.org> wrote: >>> I looked into this a bit on Friday, and I don't think extracting the >>> history-related functions will be as straight-forward as the download case. >>> ATM, >>> there is a lot of back and forth between the HistoryService and >>> FaviconHandler, >>> as the service is used to look up favicon urls for page urls, as well as to >>> fetch favicons, so delegating could be difficult. >>> >>> On 2012/10/26 19:32:26, sky wrote: >>>> >>>> On Thu, Oct 25, 2012 at 3:21 PM, Jonathan Dixon <mailto:joth@chromium.org> >>> >>> wrote: >>> >>>> > >>>> > >>>> > >>>> > On 25 October 2012 09:31, Scott Violet <mailto:sky@chromium.org> wrote: >>>> >> >>>> >> Jonathan, do you want the history feature? >>>> >> >>>> > >>>> > Just to be super clear- are we talking about the feature that populate >>>> > the >>>> > back-froward history (i.e. to make >>>> > NavigationController->GetActiveItem()->GetFavicon() work correctly) or >>>> > the >>>> > persistent history as part of Profile? (I've seen both referred to as >>>> > history in different contexts, but assuming it's the latter here) >>> >>> >>>> In this case we're referring to the latter, persisting favicons to >>>> some sort of long term storage. Once you have that you can implement >>>> back-forward history, but it's separate than what we're referring to >>>> here. >>> >>> >>>> -Scott >>> >>> >>>> > >>>> > >>>> > Yes - while It's not an absolute requirement for us, we could >>>> > optionally >>>> > use it, if a version existed without onerous chrome/ layer dependencies. >>>> > (AIUI this would allow us to implement a minimal working version of >>>> > >>>> > http://developer.android.com/reference/android/webkit/WebIconDatabase.html >>>> > to 'cache' download icons) >>>> > >>>> > We're definitely keen to get the basic favicon support in place as a >>>> > higher >>>> > priority, but no need to rush it through on our account if the full >>>> > helper >>>> > extraction is generally preferred. >>>> > >>>> > >>>> >> >>>> >> -Scott >>>> >> >>>> >> On Thu, Oct 25, 2012 at 8:59 AM, <mailto:caitkp@chromium.org> wrote: >>>> >> > I agree that in future it would make sense for the delegate to handle >>>> >> > the >>>> >> > history case as well as the download case. Jonathan is better >>>> >> > equipped >>>> >> > than >>>> >> > I to >>>> >> > comment on whether WebView would be able to make use of this feature. >>>> >> > >>>> >> > In the interim, I would like to push forward with the >>>> >> > FaviconDownloadHelper, >>>> >> > so >>>> >> > it is not blocking WebView. There are also a couple places in Chrome >>>> >> > which >>>> >> > could >>>> >> > be converted to using it and remove a lot of boilerplate message >>>> >> > handling >>>> >> > code: >>>> >> > >>>> >> > >>> >>> >>> http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/ash/laun... >>>> >>>> >> > and >>>> >> > >>>> >> > >>> >>> >>> http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/as... >>>> >>>> >> > >>>> >> > >>>> >> > On 2012/10/24 20:58:34, sky wrote: >>>> >> >> >>>> >> >> I like the idea of promoting this code, but I tend to think we >>>> >> >> should >>>> >> >> try and promote it all. The delegate should provide the hooks for >>>> >> >> getting the favicon from history. That way the chrome side hooks up >>>> >> >> to >>>> >> >> historyservice and other embedders hook it to whatever they need. If >>>> >> >> an embedder isn't caching favicons the return code could indicate >>>> >> >> this >>>> >> >> (or maybe no delegate). >>>> >> > >>>> >> > >>>> >> >> -Scott >>>> >> > >>>> >> > >>>> >> >> On Wed, Oct 24, 2012 at 10:31 AM, <mailto:caitkp@chromium.org> >>>> >> >> wrote: >>>> >> >> > The rationale behind this change is to extract the favicon >>>> >> >> > downloading >>>> >> >> > code >>>> >> >> > from >>>> >> >> > the FaviconTabHelper (which requires a HistroyService, >>>> >> >> > FaviconService >>>> >> >> > etc.) >>>> >> >> > so >>>> >> >> > that the WebView team can reuse the code without having to depend >>>> >> >> > on >>>> >> >> > these >>>> >> >> > services. >>>> >> >> > >>>> >> >> > >>>> >> >> > >>>> >> >> > >>>> >> > >>>> >> > >>>> >> > >>>> >> > >>> >>> >>> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>>> >>>> >> >> >>>> >> >> > File chrome/browser/favicon/favicon_download_helper.h (right): >>>> >> >> > >>>> >> >> > >>>> >> > >>>> >> > >>>> >> > >>>> >> > >>> >>> >>> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>>> >>>> >> >> >>>> >> >> > chrome/browser/favicon/favicon_download_helper.h:32: public >>>> >> >> > base::RefCounted<FaviconDownloadHelper> { >>>> >> >> > On 2012/10/24 16:38:10, sky wrote: >>>> >> >> >> >>>> >> >> >> Why is this ref counted? >>>> >> >> > >>>> >> >> > It needs to be ref counted because it uses the UserDataAdapter, >>>> >> >> > which >>>> >> >> > expects a ref-counted object. >>>> >> >> > >>>> >> >> > >>>> >> >> > >>>> >> > >>>> >> > >>>> >> > >>>> >> > >>> >>> >>> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>>> >>>> >> >> >>>> >> >> > File chrome/browser/favicon/favicon_tab_helper.cc (right): >>>> >> >> > >>>> >> >> > >>>> >> > >>>> >> > >>>> >> > >>>> >> > >>> >>> >>> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/favicon/fav... >>>> >>>> >> >> >>>> >> >> > chrome/browser/favicon/favicon_tab_helper.cc:153: if (helper) { >>>> >> >> > On 2012/10/24 16:38:10, sky wrote: >>>> >> >> >> >>>> >> >> >> return helper ? helper->... : -1; >>>> >> >> > >>>> >> >> > >>>> >> >> > Done. >>>> >> >> > >>>> >> >> > >>>> >> >> > >>>> >> > >>>> >> > >>>> >> > >>>> >> > >>> >>> >>> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >>>> >>>> >> >> >>>> >> >> > File chrome/browser/ui/tab_contents/tab_contents.cc (right): >>>> >> >> > >>>> >> >> > >>>> >> > >>>> >> > >>>> >> > >>>> >> > >>> >>> >>> http://codereview.chromium.org/11195010/diff/17001/chrome/browser/ui/tab_cont... >>>> >>>> >> >> >>>> >> >> > chrome/browser/ui/tab_contents/tab_contents.cc:136: >>>> >> >> > FaviconTabHelper::FromWebContents(contents)); >>>> >> >> > On 2012/10/24 16:38:10, sky wrote: >>>> >> >> >> >>>> >> >> >> Doesn't doing this lead to the possibility of FaviconTabHelper >>>> >> >> >> being >>>> >> >> > >>>> >> >> > destroyed >>>> >> >> >> >>>> >> >> >> before FaviconDownloadHelper? >>>> >> >> > >>>> >> >> > That is a possibility. The FaviconDownloadHelper only keeps a weak >>>> >> >> > pointer to its delegate, though. >>>> >> >> > >>>> >> >> > The other option would be to have a separate SetDelegate method, >>>> >> >> > but >>>> >> >> > seeing as the FaviconDownloadHelper's main purpose is to relay >>>> >> >> > data >>>> >> >> > back to the delegate, there is not much point in creating an >>>> >> >> > instance >>>> >> >> > of >>>> >> >> > it without a delegate... >>>> >> >> > >>>> >> >> > http://codereview.chromium.org/11195010/ >>>> >> > >>>> >> > >>>> >> > >>>> >> > http://codereview.chromium.org/11195010/ >>>> > >>>> > >>> >>> >>> >>> >>> http://codereview.chromium.org/11195010/
http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:29: // nit: remove empty line. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:32: public base::RefCounted<FaviconDownloadHelper> { Why is this ref counted? Doing so makes it all too easy for the Delegate to be deleted before this. I think this class should be owned by the FaviconTabHelper and have a normal constructor/destructor and not be ref counted. Seem my email on the rationale here. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:33: nit: remove newline. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:77: DISALLOW_IMPLICIT_CONSTRUCTORS(FaviconDownloadHelper); nit: newline between 76/77. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper_delegate.h (right): http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper_delegate.h:31: }; protected virtual destructor.
lgtm from webview side. Thanks! https://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_tab_helper.cc (right): https://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_tab_helper.cc:150: FaviconDownloadHelper::FromWebContents(web_contents()); I think it maybe clearer to either DCHECK the result rather than try and deal with a possible NULL here. or else call CreateForWebContentsAndDelegate(web_contents(), this) which ensures it will never be NULL.
http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:29: // On 2012/10/29 23:12:48, sky wrote: > nit: remove empty line. Done. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:32: public base::RefCounted<FaviconDownloadHelper> { On 2012/10/29 23:12:48, sky wrote: > Why is this ref counted? Doing so makes it all too easy for the Delegate to be > deleted before this. I think this class should be owned by the FaviconTabHelper > and have a normal constructor/destructor and not be ref counted. Seem my email > on the rationale here. Done. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:33: On 2012/10/29 23:12:48, sky wrote: > nit: remove newline. Done. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:77: DISALLOW_IMPLICIT_CONSTRUCTORS(FaviconDownloadHelper); On 2012/10/29 23:12:48, sky wrote: > nit: newline between 76/77. Done. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper_delegate.h (right): http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper_delegate.h:31: }; On 2012/10/29 23:12:48, sky wrote: > protected virtual destructor. Done. http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.cc (right): http://codereview.chromium.org/11195010/diff/24001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.cc:150: FaviconDownloadHelper::FromWebContents(web_contents()); On 2012/10/30 14:17:46, joth wrote: > I think it maybe clearer to either DCHECK the result rather than try and deal > with a possible NULL here. or else call > CreateForWebContentsAndDelegate(web_contents(), this) which ensures it will > never be NULL. Done.
lgtm http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:30: : public content::WebContentsObserver { nit: on one line now http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:47: nit: no need for extra \n after protected:
metro_pin_tab_helper_win could be converted to use this code too. That can be handled separately though. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.cc (right): http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:54: std::find(download_ids_.begin(), download_ids_.end(), id); nit: indent 4. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:60: if (delegate_) { Why the check for null delegate_? The constructor has a DCHECK and you don't allow seeing it to NULL. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:70: if (delegate_) { Same comment about checking delegate_ http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:39: // FaviconDownloadHelperDelegate::OnDidDownloadFavicon once the favicon has Use () after function names, eg FaviconDownloadHelperDelegate::OnDidDownloadFavicon() http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:51: // Message handler for IconHostMsg_DidDownloadFavicon. Called when the icon Is there a reason to have these non-virtual methods in the protected section? Can they be in the private section? http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:69: typedef std::vector<int> DownloadIdList; typedefs should be before functions and members. Also, is there a reason to go with a vector here rather than a set?
metro_pin_tab_helper_win, balloon_view_ash, and launcher_favicon_loader will all get converted to using FaviconDownloadHelper in a later CL. On 2012/10/30 19:59:38, sky wrote: > metro_pin_tab_helper_win could be converted to use this code too. That can be > handled separately though. > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > File chrome/browser/favicon/favicon_download_helper.cc (right): > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.cc:54: > std::find(download_ids_.begin(), download_ids_.end(), id); > nit: indent 4. > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.cc:60: if (delegate_) { > Why the check for null delegate_? The constructor has a DCHECK and you don't > allow seeing it to NULL. > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.cc:70: if (delegate_) { > Same comment about checking delegate_ > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > File chrome/browser/favicon/favicon_download_helper.h (right): > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.h:39: // > FaviconDownloadHelperDelegate::OnDidDownloadFavicon once the favicon has > Use () after function names, eg > FaviconDownloadHelperDelegate::OnDidDownloadFavicon() > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.h:51: // Message handler for > IconHostMsg_DidDownloadFavicon. Called when the icon > Is there a reason to have these non-virtual methods in the protected section? > Can they be in the private section? > > http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... > chrome/browser/favicon/favicon_download_helper.h:69: typedef std::vector<int> > DownloadIdList; > typedefs should be before functions and members. > Also, is there a reason to go with a vector here rather than a set?
http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.cc (right): http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:54: std::find(download_ids_.begin(), download_ids_.end(), id); On 2012/10/30 19:59:38, sky wrote: > nit: indent 4. Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:60: if (delegate_) { On 2012/10/30 19:59:38, sky wrote: > Why the check for null delegate_? The constructor has a DCHECK and you don't > allow seeing it to NULL. Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:70: if (delegate_) { On 2012/10/30 19:59:38, sky wrote: > Same comment about checking delegate_ Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:30: : public content::WebContentsObserver { On 2012/10/30 19:19:54, joth wrote: > nit: on one line now Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:39: // FaviconDownloadHelperDelegate::OnDidDownloadFavicon once the favicon has On 2012/10/30 19:59:38, sky wrote: > Use () after function names, eg > FaviconDownloadHelperDelegate::OnDidDownloadFavicon() Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:47: On 2012/10/30 19:19:54, joth wrote: > nit: no need for extra \n after protected: Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:51: // Message handler for IconHostMsg_DidDownloadFavicon. Called when the icon On 2012/10/30 19:59:38, sky wrote: > Is there a reason to have these non-virtual methods in the protected section? > Can they be in the private section? Done. http://codereview.chromium.org/11195010/diff/37001/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:69: typedef std::vector<int> DownloadIdList; On 2012/10/30 19:59:38, sky wrote: > typedefs should be before functions and members. > Also, is there a reason to go with a vector here rather than a set? Done.
LGTM http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.cc (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:54: std::find(download_ids_.begin(), download_ids_.end(), id); nit: indent 4. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:51: // Message handler for IconHostMsg_DidDownloadFavicon. Called when the icon nit: newline between 50/51. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper_delegate.h (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper_delegate.h:27: // Message Handler. Document this better. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.h (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.h:63: virtual void OnUpdateFaviconURL(int32 page_id, nit: wrap page_id to next line so that it aligns with indentation of 64 (like you did for id on 102). http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.h:63: virtual void OnUpdateFaviconURL(int32 page_id, Move implementation to match position.
(optional) ping: stevenjb - unless you have any comments on the Favicon/ changes, I will go ahead and submit tomorrow a.m. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.cc (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.cc:54: std::find(download_ids_.begin(), download_ids_.end(), id); On 2012/10/30 21:39:09, sky wrote: > nit: indent 4. Done. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper.h (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper.h:51: // Message handler for IconHostMsg_DidDownloadFavicon. Called when the icon On 2012/10/30 21:39:09, sky wrote: > nit: newline between 50/51. Done. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_download_helper_delegate.h (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_download_helper_delegate.h:27: // Message Handler. On 2012/10/30 21:39:09, sky wrote: > Document this better. Done. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... File chrome/browser/favicon/favicon_tab_helper.h (right): http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.h:63: virtual void OnUpdateFaviconURL(int32 page_id, On 2012/10/30 21:39:09, sky wrote: > nit: wrap page_id to next line so that it aligns with indentation of 64 (like > you did for id on 102). Done. http://codereview.chromium.org/11195010/diff/38009/chrome/browser/favicon/fav... chrome/browser/favicon/favicon_tab_helper.h:63: virtual void OnUpdateFaviconURL(int32 page_id, On 2012/10/30 21:39:09, sky wrote: > Move implementation to match position. Done.
No red flags here, I'll trust the others review.
Change committed as 165216 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
