Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
64853004:
Use high resolution icons where possible for streamlined hosted app icons. (Closed)
|
Created:
7 years, 1 month ago by calamity Modified:
7 years ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, benwells Base URL:
https://chromium.googlesource.com/chromium/src.git@browser_experiment_create_app_from_page Visibility:
Public. |
DescriptionUse high resolution icons where possible for streamlined hosted app icons.
This adds a FaviconDownloader which downloads all icons
when creating streamlined hosted apps from the current web
contents, providing higher resolution icons.
BUG=318607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241462
Patch Set 1 : #
Total comments: 46
Patch Set 2 : rebase #Patch Set 3 : rework, add tests #
Total comments: 56
Patch Set 4 : rebase #Patch Set 5 : rework #
Total comments: 2
Patch Set 6 : #Patch Set 7 : remove window icon updating #
Total comments: 11
Patch Set 8 : rework #Patch Set 9 : mass refactor: pull code into FaviconDownloader class #
Total comments: 17
Patch Set 10 : rework #
Total comments: 16
Patch Set 11 : rework #
Total comments: 4
Patch Set 12 : move favicon_downloader* to c/b/extensions #
Total comments: 2
Patch Set 13 : rebase #Patch Set 14 : fix nit, remove unused declaration #Patch Set 15 : add RenderViewImpl test to ensure FaviconTabHelper is not sent an empty vector #
Total comments: 10
Patch Set 16 : rework #Patch Set 17 : rebase #Patch Set 18 : fix tests for linux #Messages
Total messages: 41 (0 generated)
I think I still have a bit to absorb. CL needs a BUG#. Also, I think you can update web_app_unittests.cc to get some coverage -- there's `WebApplicationTest, MAYBE_GetShortcutInfoForTab` which is in the same kinda vein as this. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:1755: LoadingStateChanged(source); This doesn't feel right. WebContentsImpl only calls this when is_loading changes true <-> false (and it's a WebContentsDelegate function). Is the goal just to update the taskbar icon? Maybe you can do that more directly, or else comment what this is trying to achieve. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2097: HostedAppTabHelper* hosted_app_tab_helper = Is there an overhead to consider for this? E.g. would it be better to only do this when `is_app()` is true, and to pass the delegate in when calling CreateHostedApp() for the case when is_app is false? (assuming I haven't missed something..). But I'm actually not sure.. there doesn't seem to be much precedence for things like this. E.g. I thought PasswordManager::CreateForWebContentsAndDelegate(..) might be one, but it seems special. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2099: hosted_app_tab_helper->set_delegate(delegate); What about SetDelegate(delegate, is_app()) - that would avoid the separate set_fetch_icons_immediately(..) and avoid littering an already large file with #ifdefs https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.h:287: const gfx::Image GetCurrentPageIcon(int icon_size) const; nit: non-reference `const` return types aren't usually useful, it just encourages more copying if someone wants a non-const version, even though it's the "only" copy. and `dimension` might be a better name than |icon_size| (a foo_size is more often size in bytes when it's just an int), but it also needs some documentation. E.g. Gets the best Favicon of the page in the selected tab to fit in a square of |dimension| width in pixels. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:179: // The size of the window icon in pixels. Are these device-independent pixels? (ImageFamily works in DIPs) https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:180: const int kWindowIconSize = 32; I wonder if this should be IconUtil::kMediumIconSize (48), which icon_util.cc claims is the Alt+Tab icon size on line 155. http://msdn.microsoft.com/en-us/library/aa511280.aspx#size suggests this is the case too. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1573: return browser_->GetCurrentPageIcon(kWindowIconSize).AsImageSkia(); Looks like c/b/ui/gtk/browser_titlebar.cc might use this too... but I guess ToT is now linux_aura, so maybe you don't need to worry https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:18: HostedAppTabHelper::HostedAppTabHelper(content::WebContents* web_contents) initialize delegate_ to NULL https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:33: web_app_info_.reset(new WebApplicationInfo()); nit: pretty sure you can still call the copy constructor here. I.e. do web_app_info_.reset(new WebApplicationInfo(web_app_info)); rather than the line below https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:38: if (!favicon_url_candidates_) Could this just be `if (favicon_url_candidates.empty())`, and ditch the scoped_ptr on it? https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:47: in_progress_requests_.insert( Maybe make these 7 lines a helper function `DownloadURL(const GURL& url)`?, since it's repeated below with different formatting. Also is Unretained safe here? Haven't played much with WebContentsUserData, but it looks like the lifetime is bound to the WebContents. NetErrorTabHelper uses weak_factory_.GetWeakPtr(), but there are others that seem content with Unretained too. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:58: // If there are extra icon URLs in the WebApplicationInfo, we should download nit: remove `we should` from comment https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:65: favicon_url_candidates_->lower_bound(it->url); I don't think lower_bound is what's needed here.. to keep the insertion hint, it might work if the next line was `if (url_it != end() && url_it != it->url)`, but since the url is always in the map after (and we just want to know if it's new), I'd probably go with if (it->url.is_valid() && favicon_url_candidates_->insert(it->url).second) DownloadUrl(it->url); (but if that's not what you're trying to do, maybe a comment, since it would be unexpected for me) https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:91: web_app_info_->title = web_app_info_->title.empty() nit: the nested ternaries are a bit hard to read, and it looks like you can end up with `title = title`. Also WebContentsImpl::GetTitle looks kinda complex (might be good just to call once). Maybe if (web_app_info_->title.empty()) web_app_info_->title = web_contents()->GetTitle(); if (web_app_info_->title.empty()) web_app_info_->title = UTF8ToUTF16(web_app_info_->app_url.spec()); Same above, can just be if (web_app_info_->app_url.empty()) web_app_info_->app_url = web_contents()->GetURL() https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:128: // Check that this request is one of ours. should this always be true? (DCHECK?) https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:132: in_progress_requests_.erase(iter); erase(val) returns the number of things erased. So the above 6 lines could just be if (in_progress_requests_.erase(id) == 0) return; // Request canceled by DidUpdateFaviconURL(). https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:166: favicon_url_candidates_.reset(new std::set<GURL>()); std::set constructor can take two iterators i.e. favicon_url_candidates_.reset( new std::set<GURL>(candidates.begin(), candidates.end())); (or use std::set::assign(..) if you ditch the scoped_ptr) https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:43: fetch_icons_immediately_ = fetch_icons_immediately; nit: remove extra space after `=` https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:83: // The current page's icons. This stores . Populated by FetchIcons(). there's a sentence fragment in the comment here https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:84: gfx::ImageFamily icon_family_; nit: perhaps call this image_family_? (icon_family used to be a thing on Mac)
https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2099: hosted_app_tab_helper->set_delegate(delegate); On 2013/11/11 08:52:57, tapted wrote: > What about SetDelegate(delegate, is_app()) - that would avoid the separate > set_fetch_icons_immediately(..) and avoid littering an already large file with > #ifdefs Why is this limited OS_WIN? This flag should work currently on ChromeOS, and it would be great if we could also make this available on Linux and Mac.
https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:1755: LoadingStateChanged(source); On 2013/11/11 08:52:57, tapted wrote: > This doesn't feel right. WebContentsImpl only calls this when is_loading changes > true <-> false (and it's a WebContentsDelegate function). Is the goal just to > update the taskbar icon? Maybe you can do that more directly, or else comment > what this is trying to achieve. Yes, updating the loading animation and title bar should be sufficient here. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2097: HostedAppTabHelper* hosted_app_tab_helper = On 2013/11/11 08:52:57, tapted wrote: > Is there an overhead to consider for this? > > E.g. would it be better to only do this when `is_app()` is true, and to pass the > delegate in when calling CreateHostedApp() for the case when is_app is false? > (assuming I haven't missed something..). > > But I'm actually not sure.. there doesn't seem to be much precedence for things > like this. E.g. I thought PasswordManager::CreateForWebContentsAndDelegate(..) > might be one, but it seems special. I don't think so. Any overhead would be in creating the tab helper which is done in BrowserTabContents and I don't think we can check for is_app() there. Either way, this isn't necessary unless we are an app window. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2099: hosted_app_tab_helper->set_delegate(delegate); On 2013/11/11 08:52:57, tapted wrote: > What about SetDelegate(delegate, is_app()) - that would avoid the separate > set_fetch_icons_immediately(..) and avoid littering an already large file with > #ifdefs Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2099: hosted_app_tab_helper->set_delegate(delegate); On 2013/11/11 23:15:55, benwells wrote: > On 2013/11/11 08:52:57, tapted wrote: > > What about SetDelegate(delegate, is_app()) - that would avoid the separate > > set_fetch_icons_immediately(..) and avoid littering an already large file with > > #ifdefs > > Why is this limited OS_WIN? This flag should work currently on ChromeOS, and it > would be great if we could also make this available on Linux and Mac. This is for Window Icons which are only used on Windows at the moment. The feature as a whole will be turned on for ChromeOS. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.h:287: const gfx::Image GetCurrentPageIcon(int icon_size) const; On 2013/11/11 08:52:57, tapted wrote: > nit: non-reference `const` return types aren't usually useful, it just > encourages more copying if someone wants a non-const version, even though it's > the "only" copy. > > and `dimension` might be a better name than |icon_size| (a foo_size is more > often size in bytes when it's just an int), but it also needs some > documentation. E.g. Gets the best Favicon of the page in the selected tab to fit > in a square of |dimension| width in pixels. Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:179: // The size of the window icon in pixels. On 2013/11/11 08:52:57, tapted wrote: > Are these device-independent pixels? (ImageFamily works in DIPs) Removed. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:180: const int kWindowIconSize = 32; On 2013/11/11 08:52:57, tapted wrote: > I wonder if this should be IconUtil::kMediumIconSize (48), which icon_util.cc > claims is the Alt+Tab icon size on line 155. > http://msdn.microsoft.com/en-us/library/aa511280.aspx#size suggests this is the > case too. Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1573: return browser_->GetCurrentPageIcon(kWindowIconSize).AsImageSkia(); On 2013/11/11 08:52:57, tapted wrote: > Looks like c/b/ui/gtk/browser_titlebar.cc might use this too... but I guess ToT > is now linux_aura, so maybe you don't need to worry Fixed it just in case. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:18: HostedAppTabHelper::HostedAppTabHelper(content::WebContents* web_contents) On 2013/11/11 08:52:57, tapted wrote: > initialize delegate_ to NULL Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:33: web_app_info_.reset(new WebApplicationInfo()); On 2013/11/11 08:52:57, tapted wrote: > nit: pretty sure you can still call the copy constructor here. I.e. do > > web_app_info_.reset(new WebApplicationInfo(web_app_info)); > > rather than the line below Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:38: if (!favicon_url_candidates_) On 2013/11/11 08:52:57, tapted wrote: > Could this just be `if (favicon_url_candidates.empty())`, and ditch the > scoped_ptr on it? I wanted it as a scoped_ptr so that its existence would be a signal for whether DidUpdateFaviconURLs had been called. If there are no favicon URLs, this will return here incorrectly. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:47: in_progress_requests_.insert( On 2013/11/11 08:52:57, tapted wrote: > Maybe make these 7 lines a helper function `DownloadURL(const GURL& url)`?, > since it's repeated below with different formatting. > > Also is Unretained safe here? Haven't played much with WebContentsUserData, but > it looks like the lifetime is bound to the WebContents. NetErrorTabHelper uses > weak_factory_.GetWeakPtr(), but there are others that seem content with > Unretained too. Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:58: // If there are extra icon URLs in the WebApplicationInfo, we should download On 2013/11/11 08:52:57, tapted wrote: > nit: remove `we should` from comment Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:65: favicon_url_candidates_->lower_bound(it->url); On 2013/11/11 08:52:57, tapted wrote: > I don't think lower_bound is what's needed here.. to keep the insertion hint, it > might work if the next line was `if (url_it != end() && url_it != it->url)`, but > since the url is always in the map after (and we just want to know if it's new), > I'd probably go with > > if (it->url.is_valid() && favicon_url_candidates_->insert(it->url).second) > DownloadUrl(it->url); > > > (but if that's not what you're trying to do, maybe a comment, since it would be > unexpected for me) Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:91: web_app_info_->title = web_app_info_->title.empty() On 2013/11/11 08:52:57, tapted wrote: > nit: the nested ternaries are a bit hard to read, and it looks like you can end > up with `title = title`. Also WebContentsImpl::GetTitle looks kinda complex > (might be good just to call once). Maybe > > if (web_app_info_->title.empty()) > web_app_info_->title = web_contents()->GetTitle(); > if (web_app_info_->title.empty()) > web_app_info_->title = UTF8ToUTF16(web_app_info_->app_url.spec()); > > > Same above, can just be > > if (web_app_info_->app_url.empty()) > web_app_info_->app_url = web_contents()->GetURL() Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:128: // Check that this request is one of ours. On 2013/11/11 08:52:57, tapted wrote: > should this always be true? (DCHECK?) in_progress_requests could have been cleared by a navigate or a favicon url update. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:132: in_progress_requests_.erase(iter); On 2013/11/11 08:52:57, tapted wrote: > erase(val) returns the number of things erased. So the above 6 lines could just > be > > if (in_progress_requests_.erase(id) == 0) > return; // Request canceled by DidUpdateFaviconURL(). Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:166: favicon_url_candidates_.reset(new std::set<GURL>()); On 2013/11/11 08:52:57, tapted wrote: > std::set constructor can take two iterators i.e. > > favicon_url_candidates_.reset( > new std::set<GURL>(candidates.begin(), candidates.end())); > > (or use std::set::assign(..) if you ditch the scoped_ptr) This needs to grab the icon_url from each element. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:43: fetch_icons_immediately_ = fetch_icons_immediately; On 2013/11/11 08:52:57, tapted wrote: > nit: remove extra space after `=` Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:83: // The current page's icons. This stores . Populated by FetchIcons(). On 2013/11/11 08:52:57, tapted wrote: > there's a sentence fragment in the comment here Done. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:84: gfx::ImageFamily icon_family_; On 2013/11/11 08:52:57, tapted wrote: > nit: perhaps call this image_family_? (icon_family used to be a thing on Mac) Done.
An initial batch of comments - I'll do a thorough pass tomorrow. Looks really good. And your tests are awesome. Don't forget bug# https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:2097: HostedAppTabHelper* hosted_app_tab_helper = On 2013/11/13 06:37:14, calamity wrote: > On 2013/11/11 08:52:57, tapted wrote: > > Is there an overhead to consider for this? > > > > E.g. would it be better to only do this when `is_app()` is true, and to pass > the > > delegate in when calling CreateHostedApp() for the case when is_app is false? > > (assuming I haven't missed something..). > > > > But I'm actually not sure.. there doesn't seem to be much precedence for > things > > like this. E.g. I thought PasswordManager::CreateForWebContentsAndDelegate(..) > > might be one, but it seems special. > > I don't think so. Any overhead would be in creating the tab helper which is done > in BrowserTabContents Yeah - that's what I meant (sorry - should have commented there). E.g. you could remove it from BrowserTabContents and instead create it on demand. Either here (when is_app()) or in GetCurrentPageIcon > and I don't think we can check for is_app() there. Either > way, this isn't necessary unless we are an app window. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:47: in_progress_requests_.insert( On 2013/11/13 06:37:14, calamity wrote: > On 2013/11/11 08:52:57, tapted wrote: > > Maybe make these 7 lines a helper function `DownloadURL(const GURL& url)`?, > > since it's repeated below with different formatting. > > > > Also is Unretained safe here? Haven't played much with WebContentsUserData, > but > > it looks like the lifetime is bound to the WebContents. NetErrorTabHelper uses > > weak_factory_.GetWeakPtr(), but there are others that seem content with > > Unretained too. > > Done. Did you check up on the weakptr vs unretained goo? https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:128: // Check that this request is one of ours. On 2013/11/13 06:37:14, calamity wrote: > On 2013/11/11 08:52:57, tapted wrote: > > should this always be true? (DCHECK?) > > in_progress_requests could have been cleared by a navigate or a favicon url > update. oops - I realised that after writing that comment. https://codereview.chromium.org/64853004/diff/40001/chrome/browser/ui/web_app... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:166: favicon_url_candidates_.reset(new std::set<GURL>()); On 2013/11/13 06:37:14, calamity wrote: > On 2013/11/11 08:52:57, tapted wrote: > > std::set constructor can take two iterators i.e. > > > > favicon_url_candidates_.reset( > > new std::set<GURL>(candidates.begin(), candidates.end())); > > > > (or use std::set::assign(..) if you ditch the scoped_ptr) > > This needs to grab the icon_url from each element. Ah, so it does. Does this needs a comment that (for now?) it is just loading all available favicons (i.e. of types FAVICON, TOUCH_ICON and TOUCH_COMPRESSED_ICON)? And.. should you skip INVALID_ICON (assuming that gets set when <waves hands>there's a URL but it's invalid</waves hands>) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2094: #if defined(OS_WIN) If it's still needed, maybe move this #ifdef to HostedAppTabHelper::SetDelegate? browser.cc is too big to have #ifdefs https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.h:286: // Gets the Favicon of the page in the selected tab . nit: stray space wants more documentation (I suggested `Gets the best Favicon of the page in the selected tab to fit in a square of |dimension| width in pixels.`) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.h:287: const gfx::Image* GetCurrentPageIcon(int dimension) const; Ah - I meant go back to what was there previously, before `const` was added. I.e. doing `return *icon;` into non-const gfx::Image shouldn't care that the pointer is const, since it's copying it anyway. But if compiler doesn't like that `return gfx::Image(*icon);` should work. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:54: in_progress_requests_.insert(DownloadImage(*it)); would it make sense for the insert to happen in DownloadImage as well? (but if it makes the test more fiddly, I guess it's fine like this.) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:48: size_t pending_requests() { return in_progress_requests_.size(); } nit: can probably declare this function const https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:51: void DidDownloadFavicon(int id, nit: should this and the overrides be in private? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:39: bitmaps->clear(); It's pretty common just to return std::vectors for things like this. I'd probably do this like std::vector<SkBitmap> bitmaps(sizes.size()); for (size_t i = 0; i < sizes.size(); ++i) { SkBitmap& bitmap = bitmaps[i]; ... } return bitmaps; https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:44: sizes[i].width(), nit: outdent 1 space (and next line) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:51: class TestHostedAppTabHelper : public HostedAppTabHelper{ nit: space before { https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:63: int id_counter_; nit: private: before https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:64: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:73: web_app_info_.title = UTF8ToUTF16("Hosted App"); nit: I usually see ASCIIToUTF16 for string literals. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:74: extension_ = NULL; maybe do this in the constructor with an initializer? uninitialized primitives make me nervous https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:98: nit: extra blank line https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:101: const content::NotificationDetails& details) { OVERRIDE?, and // Overridden from content::NotificationObserver: https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:107: private: from here? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:112: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:131: std::vector<gfx::Size> sizes; nit: I'd make a vector of size 1 like std::vector<gfx::Size> sizes(1, gfx::Size(32, 32)); https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:138: EXPECT_EQ(1, helper.image_family().size()); I'm surprised you don't need to make the integer literals explicitly unsigned.. i.e. EXPECT_EQ(1u, helper.image_family().size()); Maybe that's a mac thing https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:187: content::FaviconURL(GURL("http://www.google.com/favicon.ico"), maybe include favicon2.ico as well to get coverage of the other branch? And an invalid URL?
https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... chrome/browser/extensions/tab_helper.cc:305: switch (pending_web_app_action_) { We should remove all this pending_web_app_action_ stuff if your stuff sticks. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:527: const gfx::Image* Browser::GetCurrentPageIcon(int dimension) const { Maybe this function can move off browser? It just uses the tab helper anyway, can't callers get the tab helper and use it themselves? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2094: #if defined(OS_WIN) On 2013/11/13 07:44:53, tapted wrote: > If it's still needed, maybe move this #ifdef to HostedAppTabHelper::SetDelegate? > browser.cc is too big to have #ifdefs Yeah no ifdefs in here please. There is browser_win.cc for windows specific stuff as well https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2098: SetDelegate(delegate, true /* fetch_icons_immediately*/); Is it ever called with fetch_icons_immediately false? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:1577: if (browser_->is_app() || browser_->is_type_popup()) { It seems strange that the hosted app tab helper is involved with popup windows. Do you know why the is_type_popup is here? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:28: // Per-tab class to help manage hosted app taskbar icons. Nit: this does more than manage hosted app icons, it creates hosted apps. If it is just about getting a wider variety of icons we should make it a more general thing (e.g. MultiSizeFaviconTabHelper, FaviconFamilyTabHelper). https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:68: explicit HostedAppTabHelper(content::WebContents* web_contents); Nit - remove blank line above. Nit - is 'they' all the protected things? If so /DownloadImage/DownloadImageForTesting/. If not /are used for/are protected to be overridden in/. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h:17: virtual void OnWindowIconLoaded(content::WebContents* source) = 0; Which icon / size is finished?
https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... chrome/browser/extensions/tab_helper.cc:305: switch (pending_web_app_action_) { We should remove all this pending_web_app_action_ stuff if your stuff sticks. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:527: const gfx::Image* Browser::GetCurrentPageIcon(int dimension) const { Maybe this function can move off browser? It just uses the tab helper anyway, can't callers get the tab helper and use it themselves? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2094: #if defined(OS_WIN) On 2013/11/13 07:44:53, tapted wrote: > If it's still needed, maybe move this #ifdef to HostedAppTabHelper::SetDelegate? > browser.cc is too big to have #ifdefs Yeah no ifdefs in here please. There is browser_win.cc for windows specific stuff as well https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2098: SetDelegate(delegate, true /* fetch_icons_immediately*/); Is it ever called with fetch_icons_immediately false? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:1577: if (browser_->is_app() || browser_->is_type_popup()) { It seems strange that the hosted app tab helper is involved with popup windows. Do you know why the is_type_popup is here? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:28: // Per-tab class to help manage hosted app taskbar icons. Nit: this does more than manage hosted app icons, it creates hosted apps. If it is just about getting a wider variety of icons we should make it a more general thing (e.g. MultiSizeFaviconTabHelper, FaviconFamilyTabHelper). https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:68: explicit HostedAppTabHelper(content::WebContents* web_contents); Nit - remove blank line above. Nit - is 'they' all the protected things? If so /DownloadImage/DownloadImageForTesting/. If not /are used for/are protected to be overridden in/. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h:17: virtual void OnWindowIconLoaded(content::WebContents* source) = 0; Which icon / size is finished?
https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... chrome/browser/extensions/tab_helper.cc:305: switch (pending_web_app_action_) { We should remove all this pending_web_app_action_ stuff if your stuff sticks. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:527: const gfx::Image* Browser::GetCurrentPageIcon(int dimension) const { Maybe this function can move off browser? It just uses the tab helper anyway, can't callers get the tab helper and use it themselves? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2094: #if defined(OS_WIN) On 2013/11/13 07:44:53, tapted wrote: > If it's still needed, maybe move this #ifdef to HostedAppTabHelper::SetDelegate? > browser.cc is too big to have #ifdefs Yeah no ifdefs in here please. There is browser_win.cc for windows specific stuff as well https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2098: SetDelegate(delegate, true /* fetch_icons_immediately*/); Is it ever called with fetch_icons_immediately false? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:1577: if (browser_->is_app() || browser_->is_type_popup()) { It seems strange that the hosted app tab helper is involved with popup windows. Do you know why the is_type_popup is here? https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:28: // Per-tab class to help manage hosted app taskbar icons. Nit: this does more than manage hosted app icons, it creates hosted apps. If it is just about getting a wider variety of icons we should make it a more general thing (e.g. MultiSizeFaviconTabHelper, FaviconFamilyTabHelper). https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:68: explicit HostedAppTabHelper(content::WebContents* web_contents); Nit - remove blank line above. Nit - is 'they' all the protected things? If so /DownloadImage/DownloadImageForTesting/. If not /are used for/are protected to be overridden in/. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h:17: virtual void OnWindowIconLoaded(content::WebContents* source) = 0; Which icon / size is finished?
Meant to mention: still looking, this is just a first quick pass.
https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/extension... chrome/browser/extensions/tab_helper.cc:305: switch (pending_web_app_action_) { On 2013/11/13 21:55:17, benwells wrote: > We should remove all this pending_web_app_action_ stuff if your stuff sticks. Maybe. The update stuff may still apply. We still need an indicator that a hosted app creation is in progress because this class fetches the WebApplicationInfo async from the renderer. Ideally, if this all sticks, we can remove web_app_ui.cc which creates and updates the "Create application shortcuts" stuff. We'd do the updates via our app shortcut update stuff which should work for free once I figure out how to update these generated extensions with new icons (if that's something we want). https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:527: const gfx::Image* Browser::GetCurrentPageIcon(int dimension) const { On 2013/11/13 21:55:17, benwells wrote: > Maybe this function can move off browser? It just uses the tab helper anyway, > can't callers get the tab helper and use it themselves? Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2094: #if defined(OS_WIN) On 2013/11/13 07:44:53, tapted wrote: > If it's still needed, maybe move this #ifdef to HostedAppTabHelper::SetDelegate? > browser.cc is too big to have #ifdefs Moved the #ifdef into HostedAppTabHelper::SetDelegate. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.cc:2098: SetDelegate(delegate, true /* fetch_icons_immediately*/); On 2013/11/13 21:55:17, benwells wrote: > Is it ever called with fetch_icons_immediately false? Nope. Moved the setting of this into HostedAppTabHelper::SetDelegate. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.h:286: // Gets the Favicon of the page in the selected tab . On 2013/11/13 07:44:53, tapted wrote: > nit: stray space wants more documentation (I suggested `Gets the best Favicon of > the page in the selected tab to fit in a square of |dimension| width in > pixels.`) Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/browse... chrome/browser/ui/browser.h:287: const gfx::Image* GetCurrentPageIcon(int dimension) const; On 2013/11/13 07:44:53, tapted wrote: > Ah - I meant go back to what was there previously, before `const` was added. > I.e. doing `return *icon;` into non-const gfx::Image shouldn't care that the > pointer is const, since it's copying it anyway. But if compiler doesn't like > that `return gfx::Image(*icon);` should work. > Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:1577: if (browser_->is_app() || browser_->is_type_popup()) { On 2013/11/13 21:55:17, benwells wrote: > It seems strange that the hosted app tab helper is involved with popup windows. > Do you know why the is_type_popup is here? According to https://code.google.com/p/chromium/issues/detail?id=257481, this is to let popup windows display favicons. This is bugged at the moment because the taskbar icon becomes the favicon in "Never combine" mode. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:54: in_progress_requests_.insert(DownloadImage(*it)); On 2013/11/13 07:44:53, tapted wrote: > would it make sense for the insert to happen in DownloadImage as well? (but if > it makes the test more fiddly, I guess it's fine like this.) That's where it was, but it made testing difficult. I had to make |in_progress_requests_| protected and this seemed like a better solution. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:28: // Per-tab class to help manage hosted app taskbar icons. On 2013/11/13 21:55:17, benwells wrote: > Nit: this does more than manage hosted app icons, it creates hosted apps. If it > is just about getting a wider variety of icons we should make it a more general > thing (e.g. MultiSizeFaviconTabHelper, FaviconFamilyTabHelper). Fixed comment. This will also be where we update the generated hosted app (if that's something we want to do). https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:48: size_t pending_requests() { return in_progress_requests_.size(); } On 2013/11/13 07:44:53, tapted wrote: > nit: can probably declare this function const Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:51: void DidDownloadFavicon(int id, On 2013/11/13 07:44:53, tapted wrote: > nit: should this and the overrides be in private? They need to be public so the tests can use them to poke at the class. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:68: explicit HostedAppTabHelper(content::WebContents* web_contents); On 2013/11/13 21:55:17, benwells wrote: > Nit - remove blank line above. > Nit - is 'they' all the protected things? If so > /DownloadImage/DownloadImageForTesting/. If not /are used for/are protected to > be overridden in/. The tests switch out the definition of DownloadImage. There is a real implementation in hosted_app_tab_helper.cc which actually downloads an image. The testing implementation just returns incrementing ids. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h:17: virtual void OnWindowIconLoaded(content::WebContents* source) = 0; On 2013/11/13 21:55:17, benwells wrote: > Which icon / size is finished? All favicon icons/sizes provided by the webpage. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:39: bitmaps->clear(); On 2013/11/13 07:44:53, tapted wrote: > It's pretty common just to return std::vectors for things like this. I'd > probably do this like > > std::vector<SkBitmap> bitmaps(sizes.size()); > for (size_t i = 0; i < sizes.size(); ++i) { > SkBitmap& bitmap = bitmaps[i]; > ... > } > return bitmaps; Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:44: sizes[i].width(), On 2013/11/13 07:44:53, tapted wrote: > nit: outdent 1 space (and next line) Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:51: class TestHostedAppTabHelper : public HostedAppTabHelper{ On 2013/11/13 07:44:53, tapted wrote: > nit: space before { Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:63: int id_counter_; On 2013/11/13 07:44:53, tapted wrote: > nit: private: before Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:64: }; On 2013/11/13 07:44:53, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:73: web_app_info_.title = UTF8ToUTF16("Hosted App"); On 2013/11/13 07:44:53, tapted wrote: > nit: I usually see ASCIIToUTF16 for string literals. Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:74: extension_ = NULL; On 2013/11/13 07:44:53, tapted wrote: > maybe do this in the constructor with an initializer? uninitialized primitives > make me nervous Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:98: On 2013/11/13 07:44:53, tapted wrote: > nit: extra blank line Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:101: const content::NotificationDetails& details) { On 2013/11/13 07:44:53, tapted wrote: > OVERRIDE?, and // Overridden from content::NotificationObserver: Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:107: On 2013/11/13 07:44:53, tapted wrote: > private: from here? Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:112: }; On 2013/11/13 07:44:53, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:131: std::vector<gfx::Size> sizes; On 2013/11/13 07:44:53, tapted wrote: > nit: I'd make a vector of size 1 like > > std::vector<gfx::Size> sizes(1, gfx::Size(32, 32)); Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:138: EXPECT_EQ(1, helper.image_family().size()); On 2013/11/13 07:44:53, tapted wrote: > I'm surprised you don't need to make the integer literals explicitly unsigned.. > > i.e. EXPECT_EQ(1u, helper.image_family().size()); > > Maybe that's a mac thing Done. https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:187: content::FaviconURL(GURL("http://www.google.com/favicon.ico"), On 2013/11/13 07:44:53, tapted wrote: > maybe include favicon2.ico as well to get coverage of the other branch? And an > invalid URL? Done.
CL description might need an update too (e.g. 48x48 rather than 32x32) https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... File chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc (right): https://codereview.chromium.org/64853004/diff/510001/chrome/browser/ui/web_ap... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:107: On 2013/11/15 04:25:50, calamity wrote: > On 2013/11/13 07:44:53, tapted wrote: > > private: from here? > > Done. optional: members on test harnesses can be protected if you like that better (just not public ;). Accessors are fine too though. https://codereview.chromium.org/64853004/diff/770002/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/64853004/diff/770002/chrome/browser/ui/browse... chrome/browser/ui/browser.h:84: class Image; nit: looks like this isn't needed any more here https://codereview.chromium.org/64853004/diff/770002/chrome/browser/ui/gtk/br... File chrome/browser/ui/gtk/browser_titlebar.cc (right): https://codereview.chromium.org/64853004/diff/770002/chrome/browser/ui/gtk/br... chrome/browser/ui/gtk/browser_titlebar.cc:618: WebContents* web_contents = browser_window_->browser()-> This is a lot of boilerplate, that's repeated below and in browser_view.cc. What about static gfx::Image HostedAppTabHelper::IconForBrowser(Browser*);
Removed the window icon stuff because it adds complexity and is orthogonal to the core change. Will revisit later if we decide to keep the dynamic hosted app taskbar icon. PTAL, thanks.
https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:36: void CreateHostedApp(const WebApplicationInfo& web_app_info); I think this is all that needs to be public on this class. If there are other things that only tests call, make the tests friends. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:82: // The current page's icons. Populated by FetchIcons() or CreateHostedApp(). Is this comment out of date? Seems like only FetchIcons fetches them. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... File chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc (right): https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:92: HostedAppTabHelper::CreateForWebContents(web_contents()); Why is this created here, when there are Test ones created in the tests? https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:142: // Download should not be initiated because |fetch_icons_immediately| is not This comment is out of date. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:193: helper.DidUpdateFaviconURL(0, favicon_urls); I think it would be clearer to check the number of pending requests here.
https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:40: FetchIcons(); Can we add a method to FaviconTabHelper to get the page's current favicon URLs? e.g. FaviconTabHelper::GetFaviconURLs() We can probably ignore the case of someone creating a hosted app before the web page has fully loaded for now. If we do want to consider that case, it would be nice to try to see if we can do something a bit more generic.
On 2013/11/25 01:21:28, pkotwicz wrote: > https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... > File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): > > https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... > chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:40: FetchIcons(); > Can we add a method to FaviconTabHelper to get the page's current favicon URLs? > e.g. FaviconTabHelper::GetFaviconURLs() I feel that adding GetFaviconURLs as a synchronous method on FaviconTabHelper is disingenuous since we're not providing any way to determine when the URLs are correct. Is there any reason for moving away from the current observer pattern? > We can probably ignore the case of someone creating a hosted app before the web > page has fully loaded for now. If we do want to consider that case, it would be > nice to try to see if we can do something a bit more generic. I'd like to be able to handle this case because it will create a more consistent flow for this feature (esp during QA testing). Did you have any ideas for doing something more generic?
On 2013/11/26 03:31:22, calamity wrote: > On 2013/11/25 01:21:28, pkotwicz wrote: > > > https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... > > File chrome/browser/ui/web_applications/hosted_app_tab_helper.cc (right): > > > > > https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... > > chrome/browser/ui/web_applications/hosted_app_tab_helper.cc:40: FetchIcons(); > > Can we add a method to FaviconTabHelper to get the page's current favicon > URLs? > > e.g. FaviconTabHelper::GetFaviconURLs() > > I feel that adding GetFaviconURLs as a synchronous method on FaviconTabHelper is > disingenuous > since we're not providing any way to determine when the URLs are correct. Is > there any reason > for moving away from the current observer pattern? > You can tell if the URLs are valid based on whether the result of FaviconTabHelper::GetFaviconURLs() is non empty? Based on RenderViewImpl::SendUpdateFaviconURL(), it looks like ViewHostMsg_UpdateFaviconURL only gets sent when there is at least one favicon URL. Furthermore, it looks like the default favicon URL is sent if there are no favicon <link> tags specified in the document. If you want, you can create a WebContentsObserver to listen for DidUpdateFaviconURL() once you have determined that FaviconTabHelper (i.e. the result of FaviconTabHelper::GetFaviconURLs() is not valid) has not yet received the favicon URLs because the page has not yet been loaded. I prefer this approach because it does not involve code (granted very little code) which is running in perpetuity.
I moved everything around as pkotwicz suggested. There is no longer a persistent tab helper. Instead, a FaviconDownloader during CreateHostedApp which does the icon download and creates the hosted app once complete via a callback. PTAL. Thanks. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... File chrome/browser/ui/web_applications/hosted_app_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:36: void CreateHostedApp(const WebApplicationInfo& web_app_info); On 2013/11/25 00:40:16, benwells wrote: > I think this is all that needs to be public on this class. If there are other > things that only tests call, make the tests friends. Done. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper.h:82: // The current page's icons. Populated by FetchIcons() or CreateHostedApp(). On 2013/11/25 00:40:16, benwells wrote: > Is this comment out of date? Seems like only FetchIcons fetches them. Done. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... File chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc (right): https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:92: HostedAppTabHelper::CreateForWebContents(web_contents()); On 2013/11/25 00:40:16, benwells wrote: > Why is this created here, when there are Test ones created in the tests? Removed. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:142: // Download should not be initiated because |fetch_icons_immediately| is not On 2013/11/25 00:40:16, benwells wrote: > This comment is out of date. Done. https://codereview.chromium.org/64853004/diff/1290001/chrome/browser/ui/web_a... chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc:193: helper.DidUpdateFaviconURL(0, favicon_urls); On 2013/11/25 00:40:16, benwells wrote: > I think it would be clearer to check the number of pending requests here. Done.
Looks a lot better https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.cc (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:18: callback_(callback) { Nit: Initialization order should match that in the .h file https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:83: // Request canceled by DidUpdateFaviconURL() or DidNavigateMainFrame(). Nit: DidUpdateFaviconURL() does not cancel requests. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.h (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:12: #include "base/memory/scoped_ptr.h" You do not need to include scoped_ptr.h https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:26: // Class to help download the full-size favicons for a tab. Can you update the class description? The goal of the class is to download all of the favicons for a tab, not the largest one. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:37: FaviconDownloaderCallback callback); Can the extra favicon URLs be passed into the constructor? Can you also add a comment as to why the |extra_favicons| field is necessary. (i.e. because you want touch icons elsewhere than on Android) https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:52: // This is overridden in testing. How about: "Queries FaviconTabHelper for the page's current favicon URLs." https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:59: // should implement WebContentsObserver::DidUpdateFaviconURL. "Callers that want to handle favicon urls before the page has loaded should implement WebContentsObserver::DidUpdateFaviconURL." is misleading. I think that what you mean is that a caller may want to add an observer in case that the favicon URLs have not been set yet. I think that goes without saying in the context of the comment on line 54 and 55 and that you can just remove the sentence. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:107: scoped_ptr<std::vector<content::FaviconURL> > favicon_urls_; Can this just be a std::vector<content::FaviconURL> ?
https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.cc (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:18: callback_(callback) { On 2013/12/02 01:00:57, pkotwicz wrote: > Nit: Initialization order should match that in the .h file Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:83: // Request canceled by DidUpdateFaviconURL() or DidNavigateMainFrame(). On 2013/12/02 01:00:57, pkotwicz wrote: > Nit: DidUpdateFaviconURL() does not cancel requests. Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.h (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:12: #include "base/memory/scoped_ptr.h" On 2013/12/02 01:00:57, pkotwicz wrote: > You do not need to include scoped_ptr.h Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:26: // Class to help download the full-size favicons for a tab. On 2013/12/02 01:00:57, pkotwicz wrote: > Can you update the class description? The goal of the class is to download all > of the favicons for a tab, not the largest one. Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:37: FaviconDownloaderCallback callback); On 2013/12/02 01:00:57, pkotwicz wrote: > Can the extra favicon URLs be passed into the constructor? Can you also add a > comment as to why the |extra_favicons| field is necessary. (i.e. because you > want touch icons elsewhere than on Android) Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:52: // This is overridden in testing. On 2013/12/02 01:00:57, pkotwicz wrote: > How about: "Queries FaviconTabHelper for the page's current favicon URLs." Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:59: // should implement WebContentsObserver::DidUpdateFaviconURL. On 2013/12/02 01:00:57, pkotwicz wrote: > "Callers that want to handle favicon urls before the page has loaded should > implement WebContentsObserver::DidUpdateFaviconURL." is misleading. I think that > what you mean is that a caller may want to add an observer in case that the > favicon URLs have not been set yet. I think that goes without saying in the > context of the comment on line 54 and 55 and that you can just remove the > sentence. Done. https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:107: scoped_ptr<std::vector<content::FaviconURL> > favicon_urls_; On 2013/12/02 01:00:57, pkotwicz wrote: > Can this just be a std::vector<content::FaviconURL> ? This would require a separate bool to track whether the favicons have been populated (or making a hard assumption that there is always a favicon URL). Is that preferable?
I think you can get sky@ to take a look at the changes in chrome/browser/favicon on the next round of review https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1440001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:107: scoped_ptr<std::vector<content::FaviconURL> > favicon_urls_; Given the current implementation, we should make the hard assumption. (Note RenderViewImpl::SendUpdateFaviconURL()) WebContentsObserver::DidUpdateFaviconURL() will never be called with an empty vector. A browser test would be useful to ensure that we get a non empty vector in WebContentsObserver::DidUpdateFaviconURL in when the page does not provide any favicons. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.cc (right): https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:11: #include "third_party/skia/include/core/SkBitmap.h" You should include "ui/gfx/size.h" https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:26: void FaviconDownloader::Start() { If |extra_favicon_urls_| is non empty, you can start downloading those even if |favicon_url_candidates_| is still empty https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:81: // Request canceled by DidNavigateMainFrame(). How about: "Request may have been canceled by DidNavigateMainFrame()." https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:110: favicon_url_candidates_ = &candidates; You need to make a copy here. |candidates| does not live past the end of WebContentsImpl::OnUpdateFaviconURL() However, as I mentioned in the comment in the .h file, I think that you can replace |favicon_url_candidates_| with |got_favicon_urls_|. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.h (right): https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:26: // Class to help download all favicons for a tab. You are sure that you want to get the favicons for all the favicon URLs mapped to the page as opposed of trying to figure out the best URL? http://www.redditstatic.com/favicon.ico http://www.redditstatic.com/icon.png http://www.redditstatic.com/icon-touch.png look pretty different On Android, I think that we are trying to avoid using touch icons. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:36: FaviconDownloader(content::WebContents* web_contents, Can you add a comment as to why |extra_favicon_urls| is useful here too? https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:74: // This is populated when PopulateFaviconURLsFromWebContents() is called if Can this be a bool instead? |got_favicon_urls_| or suchlike. You would need to add a parameter to FetchIcons() to pass in the URLs to download. PopulateFaviconURLsFromWebContents() should be renamed to GetFaviconURLsFromWebContents() and have a return value. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:84: // The current page's icons. Populated by FetchIcons(). How about: "The icons which were downloaded."
+sky for chrome/browser/favicon OWNERS Thanks. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.cc (right): https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:11: #include "third_party/skia/include/core/SkBitmap.h" On 2013/12/03 19:06:49, pkotwicz wrote: > You should include "ui/gfx/size.h" Done. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:26: void FaviconDownloader::Start() { On 2013/12/03 19:06:49, pkotwicz wrote: > If |extra_favicon_urls_| is non empty, you can start downloading those even if > |favicon_url_candidates_| is still empty Done. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:81: // Request canceled by DidNavigateMainFrame(). On 2013/12/03 19:06:49, pkotwicz wrote: > How about: "Request may have been canceled by DidNavigateMainFrame()." Done. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.cc:110: favicon_url_candidates_ = &candidates; On 2013/12/03 19:06:49, pkotwicz wrote: > You need to make a copy here. |candidates| does not live past the end of > WebContentsImpl::OnUpdateFaviconURL() > > However, as I mentioned in the comment in the .h file, I think that you can > replace |favicon_url_candidates_| with |got_favicon_urls_|. Done. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_downloader.h (right): https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:26: // Class to help download all favicons for a tab. On 2013/12/03 19:06:49, pkotwicz wrote: > You are sure that you want to get the favicons for all the favicon URLs mapped > to the page as opposed of trying to figure out the best URL? > http://www.redditstatic.com/favicon.ico > http://www.redditstatic.com/icon.png > http://www.redditstatic.com/icon-touch.png > look pretty different In this case, the large icon is used as the app icon on the NTP which is much better than using any of the others. OTOH, the favicon is the best icon for the taskbar badge because it's designed at the right size and doesn't lose detail from downscaling. > On Android, I think that we are trying to avoid using touch icons. Yeah, I think we are trying to push for the "sizes" attribute but very few websites support that right now. I think we can do something smarter if that information exists (like only downloading the favicon urls we're actually interested in), but we would want to do our best for sites that don't have this info by downloading all the favicons and combining them to create an icon that can handle as many sizes as possible. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:36: FaviconDownloader(content::WebContents* web_contents, On 2013/12/03 19:06:49, pkotwicz wrote: > Can you add a comment as to why |extra_favicon_urls| is useful here too? Done. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:74: // This is populated when PopulateFaviconURLsFromWebContents() is called if On 2013/12/03 19:06:49, pkotwicz wrote: > Can this be a bool instead? |got_favicon_urls_| or suchlike. You would need to > add a parameter to FetchIcons() to pass in the URLs to download. > PopulateFaviconURLsFromWebContents() should be renamed to > GetFaviconURLsFromWebContents() and have a return value. Done. https://codereview.chromium.org/64853004/diff/1480001/chrome/browser/favicon/... chrome/browser/favicon/favicon_downloader.h:84: // The current page's icons. Populated by FetchIcons(). On 2013/12/03 19:06:49, pkotwicz wrote: > How about: "The icons which were downloaded." Done.
https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:54: // Returns the current tab's favicon urls. Returns NULL if the current tab's Nit: This comment needs to be updated
I only looked at this briefly. Can you give a bit more motivation here? Is the plan to make the existing favicon downloading code use this? If not, why is the code here rather than where it is used (some where in apps)? https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:56: const std::vector<content::FaviconURL>& GetFaviconURLs() { Name this favicon_urls, and make it a const method as well.
The motivation is to create hosted apps from websites with better icons. At the moment, a 16x16 icon is used and scaled for the taskbar icon and app launcher. Many sites actually provide a 32x32 image for high DPI devices which we can use for the taskbar on Windows. Existing favicon code won't use this. I put FaviconDownloader in c/b/favicon because there's nothing binding it to apps. Ostensibly, it could be used by other clients interested in processing all favicons that a web page provides. If you prefer, it can be moved to c/b/extensions where it's used by tab_helper.cc. https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:54: // Returns the current tab's favicon urls. Returns NULL if the current tab's On 2013/12/04 05:48:18, pkotwicz wrote: > Nit: This comment needs to be updated Done. https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:56: const std::vector<content::FaviconURL>& GetFaviconURLs() { On 2013/12/04 16:05:24, sky wrote: > Name this favicon_urls, and make it a const method as well. Done.
Yes, I prefer keeping it near extension code unless someone else needs it. That way you don't need me to review either:) -Scott On Wed, Dec 4, 2013 at 6:42 PM, <calamity@chromium.org> wrote: > The motivation is to create hosted apps from websites with better icons. At > the > moment, a 16x16 icon is used and scaled for the taskbar icon and app > launcher. > Many sites actually provide a 32x32 image for high DPI devices which we can > use > for the taskbar on Windows. > > Existing favicon code won't use this. I put FaviconDownloader in c/b/favicon > because there's nothing binding it to apps. Ostensibly, it could be used by > other clients interested in processing all favicons that a web page > provides. If > you prefer, it can be moved to c/b/extensions where it's used by > tab_helper.cc. > > > > https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... > File chrome/browser/favicon/favicon_tab_helper.h (right): > > https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... > chrome/browser/favicon/favicon_tab_helper.h:54: // Returns the current > tab's favicon urls. Returns NULL if the current tab's > On 2013/12/04 05:48:18, pkotwicz wrote: >> >> Nit: This comment needs to be updated > > > Done. > > > https://codereview.chromium.org/64853004/diff/1500001/chrome/browser/favicon/... > chrome/browser/favicon/favicon_tab_helper.h:56: const > std::vector<content::FaviconURL>& GetFaviconURLs() { > On 2013/12/04 16:05:24, sky wrote: >> >> Name this favicon_urls, and make it a const method as well. > > > Done. > > https://codereview.chromium.org/64853004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Moved favicon_downloader* to c/b/extensions where it can be OWNed by benwells@; sky@ now only OWNs favicon_tab_helper. PTAL
LGTM https://codereview.chromium.org/64853004/diff/1540001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1540001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:56: const std::vector<content::FaviconURL>& GetFaviconURLs() { GetFaviconURLS()-> favicon_urls() and make const.
+jamesr for content/renderer/ OWNERS Added a test to verify the assumption made in FaviconTabHelper that DidUpdateFaviconURL will never be given an empty vector. https://codereview.chromium.org/64853004/diff/1540001/chrome/browser/favicon/... File chrome/browser/favicon/favicon_tab_helper.h (right): https://codereview.chromium.org/64853004/diff/1540001/chrome/browser/favicon/... chrome/browser/favicon/favicon_tab_helper.h:56: const std::vector<content::FaviconURL>& GetFaviconURLs() { On 2013/12/11 15:12:06, sky wrote: > GetFaviconURLS()-> favicon_urls() and make const. Done.
https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/favicon_downloader.cc (right): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/favicon_downloader.cc:34: if (!favicon_tab_helper_urls.empty()) { Could this ever be empty, but all favicon urls have been retrieved? https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/favicon_downloader.h (right): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/favicon_downloader.h:56: // |callback| is run when all downloads complete. Nit: what |callback|? Update: Oh - the |callback_| field? It's missing the underscore. https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/favicon_downloader.h:76: // FaviconURLs are received from GetFaviconURLsFromWebContents() if available. Nit: I'd recommend just the first line of this comment describing what the bool represents, not how the favicon urls are provided. The details here are probably just going to get out of date. https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.cc (left): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:333: web_app::GetShortcutInfoForTab(web_contents(), &shortcut_info); Is GetShortcutInfoForTab still used? https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:584: favicon_downloader_.reset(); Can the downloader be removed once it is finished, instead of here? That would be much cleaner as FinishCreateHostedApp could always do the reset and we wouldn't need to reset in all possible install finishing notifications.
It seems a little strange for a content test to be verifying things that only chrome/ depends on, but ok. lgtm
https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/favicon_downloader.cc (right): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/favicon_downloader.cc:34: if (!favicon_tab_helper_urls.empty()) { On 2013/12/12 21:17:57, benwells wrote: > Could this ever be empty, but all favicon urls have been retrieved? No, WebContentsObserver::DidUpdateFaviconURL is only called with a non-empty vector so favicon_tab_helper->favicon_urls() is only empty if the favicon urls haven't been retrieved yet. There is a test I added in the latest patch set that enforces this. Added a comment below that explains the assumption. https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/favicon_downloader.h (right): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/favicon_downloader.h:56: // |callback| is run when all downloads complete. On 2013/12/12 21:17:57, benwells wrote: > Nit: what |callback|? > > Update: Oh - the |callback_| field? It's missing the underscore. Done. https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/favicon_downloader.h:76: // FaviconURLs are received from GetFaviconURLsFromWebContents() if available. On 2013/12/12 21:17:57, benwells wrote: > Nit: I'd recommend just the first line of this comment describing what the bool > represents, not how the favicon urls are provided. The details here are probably > just going to get out of date. Done. https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.cc (left): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:333: web_app::GetShortcutInfoForTab(web_contents(), &shortcut_info); On 2013/12/12 21:17:57, benwells wrote: > Is GetShortcutInfoForTab still used? Yes, in the create applications shortcut dialog =( https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/64853004/diff/1600001/chrome/browser/extensio... chrome/browser/extensions/tab_helper.cc:584: favicon_downloader_.reset(); On 2013/12/12 21:17:57, benwells wrote: > Can the downloader be removed once it is finished, instead of here? That would > be much cleaner as FinishCreateHostedApp could always do the reset and we > wouldn't need to reset in all possible install finishing notifications. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/64853004/1620001
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/64853004/1660001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/64853004/1660001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/64853004/1660001
Message was sent while issue was closed.
Change committed as 241462 |
