Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928005:
Retrieve favicons from history as NavigationEntries are converted from TabNav...
|
Created:
10 years, 5 months ago by dill Modified:
9 years, 9 months ago CC:
chromium-reviews, ben+cc_chromium.org, brettw-cc_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionBUG=5679
TEST=Restore a tab with a navigation history, check favicons.
Patch Set 1 #
Total comments: 2
Patch Set 2 : BUG=5679 #
Total comments: 15
Patch Set 3 : '' #
Total comments: 21
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 18
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 10
Patch Set 8 : '' #
Total comments: 4
Patch Set 9 : '' #
Total comments: 7
Patch Set 10 : '' #Patch Set 11 : Updated to compile against trunk. #
Total comments: 19
Patch Set 12 : Addressed code review findings, updated to compile against trunk. #Patch Set 13 : Updated to compile against trunk. #
Total comments: 9
Messages
Total messages: 55 (0 generated)
http://codereview.chromium.org/2928005/diff/1/2 File chrome/browser/tab_contents/navigation_controller.cc (right): http://codereview.chromium.org/2928005/diff/1/2#newcode721 chrome/browser/tab_contents/navigation_controller.cc:721: profile()->GetFaviconService(Profile::EXPLICIT_ACCESS); I don't think this code belongs in the NavigationController. It seems like it should be at a higher level in the session service or something. Maybe Scott can find a good place. http://codereview.chromium.org/2928005/diff/1/2#newcode739 chrome/browser/tab_contents/navigation_controller.cc:739: NavigationEntry* entry = consumer_.GetClientData(favicon_service, handle); Nothing you have here guarantees that this pointer is still valid when you use it. It could have been deleted for many reasons.
All of the code to handle this is in FaviconHelper. Currently FaviconHelper only works with the tab's active web page. Does it make sense to update FaviconHelper to work with any NavigationEntry? On 2010/07/11 23:14:24, brettw wrote: > http://codereview.chromium.org/2928005/diff/1/2 > File chrome/browser/tab_contents/navigation_controller.cc (right): > > http://codereview.chromium.org/2928005/diff/1/2#newcode721 > chrome/browser/tab_contents/navigation_controller.cc:721: > profile()->GetFaviconService(Profile::EXPLICIT_ACCESS); > I don't think this code belongs in the NavigationController. It seems like it > should be at a higher level in the session service or something. Maybe Scott can > find a good place. > > http://codereview.chromium.org/2928005/diff/1/2#newcode739 > chrome/browser/tab_contents/navigation_controller.cc:739: NavigationEntry* entry > = consumer_.GetClientData(favicon_service, handle); > Nothing you have here guarantees that this pointer is still valid when you use > it. It could have been deleted for many reasons.
On Sun, Jul 11, 2010 at 7:25 PM, <dill.sellars@gmail.com> wrote: > All of the code to handle this is in FaviconHelper. Currently FaviconHelper > only > works with the tab's active web page. Does it make sense to update > FaviconHelper > to work with any NavigationEntry? That sounds better to me, but I haven't seen the favicon helper recently to know if it can be done cleanly. There is also whatever component that makes the TabNavigation objects (the session service?) to consider. Brett
This bug can also happen if you navigate quick enough. I think the fix should be in the menu model. It should ask history for any favicons that haven't been loaded. On Sun, Jul 11, 2010 at 1:13 PM, <dill.sellars@gmail.com> wrote: > Reviewers: sky, > > Description: > Retrieve favicons from history as NavigationEntries are converted from > TabNavigations so they are available when requested from > BackForwardMenuModel. > > BUG=5679 > TEST=Restore a tab with a navigation history, check favicons. > > > Please review this at http://codereview.chromium.org/2928005/show > > SVN Base: http://src.chromium.org/svn/trunk/src/ > > Affected files: > M chrome/browser/tab_contents/navigation_controller.h > M chrome/browser/tab_contents/navigation_controller.cc > > > Index: chrome/browser/tab_contents/navigation_controller.cc > =================================================================== > --- chrome/browser/tab_contents/navigation_controller.cc (revision > 52060) > +++ chrome/browser/tab_contents/navigation_controller.cc (working > copy) > @@ -28,6 +28,7 @@ > #include "chrome/common/pref_names.h" > #include "chrome/common/render_messages.h" > #include "chrome/common/url_constants.h" > +#include "gfx/codec/png_codec.h" > #include "grit/app_resources.h" > #include "net/base/escape.h" > #include "net/base/net_util.h" > @@ -709,9 +710,46 @@ > navigations.begin(); i != navigations.end(); ++i, ++page_id) { > linked_ptr<NavigationEntry> entry(i->ToNavigationEntry(page_id, > profile_)); > entries->push_back(entry); > + if (!entry.get()->favicon().is_valid()) { > + GetFaviconFromHistory(entry.get()); > + } > } > } > > +void NavigationController::GetFaviconFromHistory(NavigationEntry* entry) { > + FaviconService* favicon_service = > + profile()->GetFaviconService(Profile::EXPLICIT_ACCESS); > + if (favicon_service) { > + CancelableRequestProvider::Handle h = > + favicon_service->GetFaviconForURL( > + entry->url(), &consumer_, > + NewCallback(this, &NavigationController::OnFavIconAvailable)); > + consumer_.SetClientData(favicon_service, h, entry); > + } > +} > + > +void NavigationController::OnFavIconAvailable( > + FaviconService::Handle handle, > + bool know_favicon, > + scoped_refptr<RefCountedMemory> data, > + bool expired, > + GURL icon_url) { > + FaviconService* favicon_service = > + profile()->GetFaviconService(Profile::EXPLICIT_ACCESS); > + NavigationEntry* entry = consumer_.GetClientData(favicon_service, > handle); > + if (know_favicon) { > + entry->favicon().set_url(icon_url); > + if (data.get() && data->size()) { > + SkBitmap image; > + gfx::PNGCodec::Decode(data->front(), data->size(), &image); > + if (!image.empty()) { > + entry->favicon().set_bitmap(image); > + entry->favicon().set_is_valid(true); > + } > + } > + } > +} > + > void NavigationController::RendererDidNavigateToNewPage( > const ViewHostMsg_FrameNavigate_Params& params, bool* did_replace_entry) > { > NavigationEntry* new_entry; > Index: chrome/browser/tab_contents/navigation_controller.h > =================================================================== > --- chrome/browser/tab_contents/navigation_controller.h (revision 52060) > +++ chrome/browser/tab_contents/navigation_controller.h (working copy) > @@ -14,6 +14,7 @@ > #include "base/string16.h" > #include "base/time.h" > #include "googleurl/src/gurl.h" > +#include "chrome/browser/favicon_service.h" > #include "chrome/browser/sessions/session_id.h" > #include "chrome/browser/ssl/ssl_manager.h" > #include "chrome/common/navigation_types.h" > @@ -507,6 +508,14 @@ > const std::vector<TabNavigation>& navigations, > std::vector<linked_ptr<NavigationEntry> >* entries); > > + virtual void OnFavIconAvailable(FaviconService::Handle h, > + bool fav_icon_available, > + scoped_refptr<RefCountedMemory> data, > + bool expired, > + GURL icon_url); > + > + virtual void GetFaviconFromHistory(NavigationEntry* entry); > + > // > --------------------------------------------------------------------------- > > // The user profile associated with this controller > @@ -582,6 +591,9 @@ > // NO_RELOAD otherwise. > ReloadType pending_reload_; > > + // Consumer for HistoryService. > + CancelableRequestConsumerT<NavigationEntry*, NULL> consumer_; > + > DISALLOW_COPY_AND_ASSIGN(NavigationController); > }; > > > >
I need a little bit of guidance, the asynchronous nature of querying the history is throwing me. I'm thinking I should query the history in BackForwardMenuModel::GetIconAt. As the history query is asynchronous I would need to provide a way to notify the menu that it needs to redraw the menu item when the favicon callback is called. If the favicon is expired or not in the history (never downloaded due to quick navigation canceling the download request), it should be downloaded. On 2010/07/12 15:29:18, sky wrote: > This bug can also happen if you navigate quick enough. I think the fix > should be in the menu model. It should ask history for any favicons > that haven't been loaded. >
On Sun, Jul 18, 2010 at 9:23 PM, <dill.sellars@gmail.com> wrote: > I need a little bit of guidance, the asynchronous nature of querying the > history > is throwing me. I'm thinking I should query the history in > BackForwardMenuModel::GetIconAt. As the history query is asynchronous I > would > need to provide a way to notify the menu that it needs to redraw the menu > item > when the favicon callback is called. If the favicon is expired or not in the > history (never downloaded due to quick navigation canceling the download > request), it should be downloaded. Triggering from the menu opening is probably better than fetching preemptively, as that side-steps a lot of issues. I don't know how the menu code, works, though. It seems like there should be a way to invalidate the view if something changes. You can also initiate the query before being asked for the icon the first time (like when the initial menu data is computed). I'd say we definitely don't want to download favicons from the web pages if they aren't in history. Generally, we should have them so this should be quite rare, and the amount and complexity of the code won't be worthwhile. Brett
On Sun, Jul 18, 2010 at 9:23 PM, <dill.sellars@gmail.com> wrote: > I need a little bit of guidance, the asynchronous nature of querying the > history > is throwing me. I'm thinking I should query the history in > BackForwardMenuModel::GetIconAt. As the history query is asynchronous I > would > need to provide a way to notify the menu that it needs to redraw the menu > item > when the favicon callback is called. If the favicon is expired or not in the > history (never downloaded due to quick navigation canceling the download > request), it should be downloaded. Yes. You'll need to modify menus to support a delegate this is notified when the icon becomes available and the menu classes will then update any internal state as appropriate when the icon changes. -Scott > On 2010/07/12 15:29:18, sky wrote: >> >> This bug can also happen if you navigate quick enough. I think the fix >> should be in the menu model. It should ask history for any favicons >> that haven't been loaded. > > > > http://codereview.chromium.org/2928005/show >
When the BackForward menu is open it seems that the message loop pauses or defers tasks and the scheduled task to query the history is put on hold, I only get the callback with the favicon after the menu is dismissed. Even the page rendering / busy spinner icon is put on hold when the menu is open. I'll need some guidance on how to handle this. On 2010/07/19 15:37:51, sky wrote: > On Sun, Jul 18, 2010 at 9:23 PM, <mailto:dill.sellars@gmail.com> wrote: > > I need a little bit of guidance, the asynchronous nature of querying the > > history > > is throwing me. I'm thinking I should query the history in > > BackForwardMenuModel::GetIconAt. As the history query is asynchronous I > > would > > need to provide a way to notify the menu that it needs to redraw the menu > > item > > when the favicon callback is called. If the favicon is expired or not in the > > history (never downloaded due to quick navigation canceling the download > > request), it should be downloaded. > > Yes. You'll need to modify menus to support a delegate this is > notified when the icon becomes available and the menu classes will > then update any internal state as appropriate when the icon changes. > > -Scott > > > On 2010/07/12 15:29:18, sky wrote: > >> > >> This bug can also happen if you navigate quick enough. I think the fix > >> should be in the menu model. It should ask history for any favicons > >> that haven't been loaded. > > > > > > > > http://codereview.chromium.org/2928005/show > > >
Sorry for the delay, was on vacation last week. Before the menu is run try: MessageLoop::current()->SetNestableTasksAllowed(true);
Patch Set 2 is up, more for feedback than for a final submission. The main problem I'm having is at line 356 of the patched native_menu_win.cc - I can't figure out a way to invalidate and repaint a single favicon / menu item. I couldn't get InvalidateRect working at all and trying to update a single menu via SetMenuItemInfo also repaints every single menu item. This results in some duplicate requests for missing icons and you get some flickering in the menu as they are redrawn. So what is the way forward? 1) Batch the requests to history so that there is a single callback when all favicons are retrieved & then redraw the menu. 2) Stay with current approach and block duplicate favicon requests (will still get a menu redraw for every favicon retrieved) 3) Hopefully I'm missing something and there is a way to redraw a single menu item?
http://codereview.chromium.org/2928005/diff/14001/15001 File app/menus/menu_model.h (right): http://codereview.chromium.org/2928005/diff/14001/15001#newcode11 app/menus/menu_model.h:11: #include "chrome/browser/fav_icon_delegate.h" app should not depend upon chrome http://codereview.chromium.org/2928005/diff/14001/15004 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/14001/15004#newcode129 chrome/browser/back_forward_menu_model.cc:129: entry->url(), &load_consumer_, spacing is wrong http://codereview.chromium.org/2928005/diff/14001/15004#newcode152 chrome/browser/back_forward_menu_model.cc:152: entry->favicon().set_is_valid(true); spacing is all wrong in this block. http://codereview.chromium.org/2928005/diff/14001/15004#newcode217 chrome/browser/back_forward_menu_model.cc:217: MessageLoop::current()->SetNestableTasksAllowed(true); You need to unset this appropriately and you need a comment as to why it's here. http://codereview.chromium.org/2928005/diff/14001/15005 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/14001/15005#newcode167 chrome/browser/back_forward_menu_model.h:167: virtual void SetFaviconLoadedDelegate(FavIconDelegate* fav_icon_delegate) Shouldn't be inlined. http://codereview.chromium.org/2928005/diff/14001/15005#newcode190 chrome/browser/back_forward_menu_model.h:190: int index; Add comments. http://codereview.chromium.org/2928005/diff/14001/15005#newcode192 chrome/browser/back_forward_menu_model.h:192: private: spacing wrong http://codereview.chromium.org/2928005/diff/14001/15005#newcode193 chrome/browser/back_forward_menu_model.h:193: DISALLOW_COPY_AND_ASSIGN(FavIconData); No point in using DISALLOW_COPY_AND_ASSIGN for a struct like this. http://codereview.chromium.org/2928005/diff/14001/15005#newcode197 chrome/browser/back_forward_menu_model.h:197: CancelableRequestConsumerTSimple<FavIconData*> load_consumer_; Don't use a pointer here. Otherwise you're going to leak. http://codereview.chromium.org/2928005/diff/14001/15006 File chrome/browser/fav_icon_delegate.h (right): http://codereview.chromium.org/2928005/diff/14001/15006#newcode12 chrome/browser/fav_icon_delegate.h:12: public: Spacing is all wrong in this file. http://codereview.chromium.org/2928005/diff/14001/15008 File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/2928005/diff/14001/15008#newcode362 views/controls/menu/native_menu_win.cc:362: // to repaint but WM_DRAWITEM is sent for evey menu item, causing duplicate evey -> every http://codereview.chromium.org/2928005/diff/14001/15008#newcode363 views/controls/menu/native_menu_win.cc:363: // requests for favicons Why would a repaint trigger duplicate requests? http://codereview.chromium.org/2928005/diff/14001/15009 File views/controls/menu/native_menu_win.h (right): http://codereview.chromium.org/2928005/diff/14001/15009#newcode19 views/controls/menu/native_menu_win.h:19: class NativeMenuWin : public MenuWrapper, public FavIconDelegate { NativeMenu should not implement FavIconDelegate.
http://codereview.chromium.org/2928005/diff/14001/15008 File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/2928005/diff/14001/15008#newcode363 views/controls/menu/native_menu_win.cc:363: // requests for favicons In OnDrawItem, data->native_menu_win->model_->GetIconAt will query history for a missing favicon, this does the callback to FaviconLoaded, since I can't redraw a single menu item data->native_menu_win->model_->GetIconAt keeps getting called for every single Favicon. I either have to change the approach (grab all the favicon requests before drawing the menu) or add a flag to prevent duplicate history queries for the favicon.
http://codereview.chromium.org/2928005/diff/14001/15008 File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/2928005/diff/14001/15008#newcode363 views/controls/menu/native_menu_win.cc:363: // requests for favicons On 2010/08/30 17:04:41, dill wrote: > In OnDrawItem, data->native_menu_win->model_->GetIconAt will query history for a > missing favicon, this does the callback to FaviconLoaded, since I can't redraw a > single menu item data->native_menu_win->model_->GetIconAt keeps getting called > for every single Favicon. I either have to change the approach (grab all the > favicon requests before drawing the menu) or add a flag to prevent duplicate > history queries for the favicon. You should track whether you've done the request to the favicon service already. If you haven't already done the request, don't do it again.
http://codereview.chromium.org/2928005/diff/28001/29002 File app/menus/menu_model.h (right): http://codereview.chromium.org/2928005/diff/28001/29002#newcode44 app/menus/menu_model.h:44: virtual void OnFavIconLoaded(int index) = 0; This method name should correspond to that of the MenuModel. So, something like OnIconChanged. http://codereview.chromium.org/2928005/diff/28001/29002#newcode50 app/menus/menu_model.h:50: Delegate* delegate_; The field should only be in subclasses. http://codereview.chromium.org/2928005/diff/28001/29005 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/28001/29005#newcode160 chrome/browser/back_forward_menu_model.cc:160: entry->favicon().set_is_valid(true); You should set this if know_favicon is true. http://codereview.chromium.org/2928005/diff/28001/29005#newcode163 chrome/browser/back_forward_menu_model.cc:163: entry->favicon().set_bitmap(fav_icon); You should also set the favicon url. http://codereview.chromium.org/2928005/diff/28001/29005#newcode165 chrome/browser/back_forward_menu_model.cc:165: delegate_->OnFavIconLoaded(faviconData->index); You're accessing an object you just deleted. http://codereview.chromium.org/2928005/diff/28001/29006 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/28001/29006#newcode177 chrome/browser/back_forward_menu_model.h:177: std::vector<int> requestedFavicons; Description? Also, this should be requested_favicons_ http://codereview.chromium.org/2928005/diff/28001/29006#newcode181 chrome/browser/back_forward_menu_model.h:181: bool nestedAllowed_; naming is wrong. http://codereview.chromium.org/2928005/diff/28001/29006#newcode191 chrome/browser/back_forward_menu_model.h:191: FavIconData() {} No need to declare constructor or destructor on this class. http://codereview.chromium.org/2928005/diff/28001/29006#newcode193 chrome/browser/back_forward_menu_model.h:193: NavigationEntry* entry; Document ownership. http://codereview.chromium.org/2928005/diff/28001/29006#newcode198 chrome/browser/back_forward_menu_model.h:198: CancelableRequestConsumerTSimple<FavIconData*> load_consumer_; Make this <FavIconData> so that you don't have to worry about memory management (current code leaks). http://codereview.chromium.org/2928005/diff/28001/29007 File views/controls/menu/menu_2.cc (right): http://codereview.chromium.org/2928005/diff/28001/29007#newcode33 views/controls/menu/menu_2.cc:33: model_->MenuWillClose(); What happens if the menu closes normally? http://codereview.chromium.org/2928005/diff/28001/29008 File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/2928005/diff/28001/29008#newcode312 views/controls/menu/native_menu_win.cc:312: model_->SetDelegate(this); spacing is off. http://codereview.chromium.org/2928005/diff/28001/29008#newcode357 views/controls/menu/native_menu_win.cc:357: int menu_index = model_index + first_item_index_; spacing is off. http://codereview.chromium.org/2928005/diff/28001/29009 File views/controls/menu/native_menu_win.h (right): http://codereview.chromium.org/2928005/diff/28001/29009#newcode12 views/controls/menu/native_menu_win.h:12: #include "app/menus/menu_model.h" sorting http://codereview.chromium.org/2928005/diff/28001/29009#newcode90 views/controls/menu/native_menu_win.h:90: void OnFavIconLoaded(int model_index); virtual
http://codereview.chromium.org/2928005/diff/28001/29002 File app/menus/menu_model.h (right): http://codereview.chromium.org/2928005/diff/28001/29002#newcode50 app/menus/menu_model.h:50: Delegate* delegate_; Is SetDelegate(Delegate* delegate) OK in this class? This interface is all that is exposed to the native_menu classes so I need something here to allow the delegate to be set. http://codereview.chromium.org/2928005/diff/28001/29005 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/28001/29005#newcode160 chrome/browser/back_forward_menu_model.cc:160: entry->favicon().set_is_valid(true); known_favicon is checked in the if statement above http://codereview.chromium.org/2928005/diff/28001/29006 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/28001/29006#newcode198 chrome/browser/back_forward_menu_model.h:198: CancelableRequestConsumerTSimple<FavIconData*> load_consumer_; I think this is necessary - I need a new FavIconData created for each favicon in the menu to pass to load_consumer_.SetClientData. FavIconData is allocated in FetchFavicon and deleted in OnFavIconDataAvailable. http://codereview.chromium.org/2928005/diff/28001/29007 File views/controls/menu/menu_2.cc (right): http://codereview.chromium.org/2928005/diff/28001/29007#newcode33 views/controls/menu/menu_2.cc:33: model_->MenuWillClose(); Argh. I thought this was called anytime a menu is closed. I guess I have to setting / reseting MessageLoop::current()->SetNestableTasksAllowed in the native menus so I know when it's done drawing the menu.
http://codereview.chromium.org/2928005/diff/28001/29002 File app/menus/menu_model.h (right): http://codereview.chromium.org/2928005/diff/28001/29002#newcode50 app/menus/menu_model.h:50: Delegate* delegate_; On 2010/09/08 20:11:36, dill wrote: > Is SetDelegate(Delegate* delegate) OK in this class? This interface is all that > is exposed to the native_menu classes so I need something here to allow the > delegate to be set. SetDelegate is ok, but not the field for the delegate, also make it pure virtual. http://codereview.chromium.org/2928005/diff/28001/29006 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/28001/29006#newcode198 chrome/browser/back_forward_menu_model.h:198: CancelableRequestConsumerTSimple<FavIconData*> load_consumer_; On 2010/09/08 20:11:36, dill wrote: > I think this is necessary - I need a new FavIconData created for each favicon in > the menu to pass to load_consumer_.SetClientData. FavIconData is allocated in > FetchFavicon and deleted in OnFavIconDataAvailable. FavIconData is necessary, but it's not necessary that you use pointers. As the code stands it leaks if BackForwardMenuModel is deleted and there are outstanding requests for favicons.
I looked at few random things. I'll defer to sky for the rest. http://codereview.chromium.org/2928005/diff/41003/37005 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/41003/37005#newcode149 chrome/browser/back_forward_menu_model.cc:149: int index = I would probably format this more simply as: int index = load_consumer_.GetClientData( browser_->profile()->GetFaviconService(Profile::EXPLICIT_ACCESS), handle); http://codereview.chromium.org/2928005/diff/41003/37005#newcode153 chrome/browser/back_forward_menu_model.cc:153: NavigationEntry* entry = GetNavigationEntry(index); Can this be moved inside the if statement? I would also remove the DCHECK(entry). This will crash in release mode anyway, and the DCHECK doesn't make the null pointer exception any easier to find. http://codereview.chromium.org/2928005/diff/41003/37005#newcode156 chrome/browser/back_forward_menu_model.cc:156: gfx::PNGCodec::Decode(data->front(), data->size(), &fav_icon)) { Align this with "know_favicon" http://codereview.chromium.org/2928005/diff/41003/37009 File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/2928005/diff/41003/37009#newcode313 views/controls/menu/native_menu_win.cc:313: model_->SetDelegate(this); Check indenting. http://codereview.chromium.org/2928005/diff/41003/37009#newcode364 views/controls/menu/native_menu_win.cc:364: int menu_index = model_index + first_item_index_; Check indenting.
Broader topics: It will take me a little while to grock the Cocoa version having never touched Objective C before, hopefully I can figure that out in a couple weeks. Should I download the favicon if it is not history? This will handle the case where you navigate quickly between sites and Chromium never had a chance to download the favicon. http://codereview.chromium.org/2928005/diff/41003/37005 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/41003/37005#newcode149 chrome/browser/back_forward_menu_model.cc:149: int index = On 2010/09/12 16:35:13, brettw wrote: > I would probably format this more simply as: > int index = load_consumer_.GetClientData( > browser_->profile()->GetFaviconService(Profile::EXPLICIT_ACCESS), handle); 81 characters, unfortunately. How about this? int index = load_consumer_.GetClientData( browser_->profile()->GetFaviconService( Profile::EXPLICIT_ACCESS), handle); http://codereview.chromium.org/2928005/diff/41003/37005#newcode153 chrome/browser/back_forward_menu_model.cc:153: NavigationEntry* entry = GetNavigationEntry(index); On 2010/09/12 16:35:13, brettw wrote: > Can this be moved inside the if statement? I would also remove the > DCHECK(entry). This will crash in release mode anyway, and the DCHECK doesn't > make the null pointer exception any easier to find. Should I replace the DCHECK with a null check?
> Should I download the favicon if it is not history? Don't attempt that in this patch. -Scott http://codereview.chromium.org/2928005/diff/41003/37002 File app/menus/menu_model.h (right): http://codereview.chromium.org/2928005/diff/41003/37002#newcode45 app/menus/menu_model.h:45: virtual ~Delegate() {} Make destructor protected. http://codereview.chromium.org/2928005/diff/41003/37004 File app/menus/simple_menu_model.h (right): http://codereview.chromium.org/2928005/diff/41003/37004#newcode115 app/menus/simple_menu_model.h:115: virtual void SetDelegate(menus::MenuModel::Delegate* delegate) {} Don't inline this. http://codereview.chromium.org/2928005/diff/41003/37005 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/41003/37005#newcode149 chrome/browser/back_forward_menu_model.cc:149: int index = Change this to int index = load_consumer_.GetClientDataForCurrentRequest(); http://codereview.chromium.org/2928005/diff/41003/37005#newcode156 chrome/browser/back_forward_menu_model.cc:156: gfx::PNGCodec::Decode(data->front(), data->size(), &fav_icon)) { You should deal with the case where know_favicon is true but data.get() is NULL or data->size() is 0. http://codereview.chromium.org/2928005/diff/41003/37005#newcode223 chrome/browser/back_forward_menu_model.cc:223: requested_favicons_.clear(); You need to cancel all the pending requests when the menu hides, otherwise there's a possibility of setting the favicon for the wrong navigationentry. http://codereview.chromium.org/2928005/diff/41003/37006 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/41003/37006#newcode59 chrome/browser/back_forward_menu_model.h:59: virtual void FetchFavicon(NavigationEntry* entry, int index); This should not be virtual and they should be private (and you don't want them grouped with MenuModel overrides, but when you move to private that'll be fixed). http://codereview.chromium.org/2928005/diff/41003/37006#newcode167 chrome/browser/back_forward_menu_model.h:167: virtual void SetDelegate(Delegate* delegate); Put this at the same location as other MenuModel methods. http://codereview.chromium.org/2928005/diff/41003/37006#newcode179 chrome/browser/back_forward_menu_model.h:179: // each favicon retrived from history. This keeps track of which favicons The first sentence doesn't make sense and it's needed. http://codereview.chromium.org/2928005/diff/41003/37006#newcode197 chrome/browser/back_forward_menu_model.h:197: CancelableRequestConsumerTSimple<size_t> load_consumer_; The type here does not match that of how use it. http://codereview.chromium.org/2928005/diff/41003/37008 File chrome/browser/gtk/menu_gtk.h (right): http://codereview.chromium.org/2928005/diff/41003/37008#newcode99 chrome/browser/gtk/menu_gtk.h:99: void OnIconChanged(int model_index); virtual and public. http://codereview.chromium.org/2928005/diff/41003/37010 File views/controls/menu/native_menu_win.h (right): http://codereview.chromium.org/2928005/diff/41003/37010#newcode89 views/controls/menu/native_menu_win.h:89: void OnIconChanged(int model_index); virtual and public.
Addressed code review comments. Still trying to find time for cocoa version.
Should be good to go now, Cocoa version is done.
Reviews generally go faster if you don't try to do to much at once. I would suggest you remove the changes to the various menu implementations from this patch. http://codereview.chromium.org/2928005/diff/71001/72004 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/71001/72004#newcode124 chrome/browser/back_forward_menu_model.cc:124: void BackForwardMenuModel::FetchFavicon(NavigationEntry* entry, int index) { Order of definition must match declaration. http://codereview.chromium.org/2928005/diff/71001/72004#newcode127 chrome/browser/back_forward_menu_model.cc:127: entry->unique_id()); align with '(' on previous line. http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = GetNavigationEntry(index); Don't rely on the index, it might have changed between when you asked for the request and when you got the data back. http://codereview.chromium.org/2928005/diff/71001/72005 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/71001/72005#newcode74 chrome/browser/back_forward_menu_model.h:74: // Set the delegate for triggering OnIconChanged end sentence with '.'. http://codereview.chromium.org/2928005/diff/71001/72005#newcode78 chrome/browser/back_forward_menu_model.h:78: void FetchFavicon(NavigationEntry* entry, int index); Add descriptions. http://codereview.chromium.org/2928005/diff/71001/72005#newcode180 chrome/browser/back_forward_menu_model.h:180: std::vector<int> requested_favicons_; Why is this a vector and not a set?
http://codereview.chromium.org/2928005/diff/71001/72004 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = GetNavigationEntry(index); Should I pass the NavigationEntry* instead of the index as the ClientData or use the NavigationEntry.unique_id? http://codereview.chromium.org/2928005/diff/71001/72005 File chrome/browser/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/71001/72005#newcode180 chrome/browser/back_forward_menu_model.h:180: std::vector<int> requested_favicons_; Agreed, will use set.
http://codereview.chromium.org/2928005/diff/71001/72004 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = GetNavigationEntry(index); On 2010/10/31 13:23:36, dill wrote: > Should I pass the NavigationEntry* instead of the index as the ClientData or use > the NavigationEntry.unique_id? Using the NavigationEntry* is equally perilous as it is possible for some one to delete it. Use the unique id. Also, write a unit test for the changes.
http://codereview.chromium.org/2928005/diff/71001/72004 File chrome/browser/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = GetNavigationEntry(index); On 2010/11/01 16:00:41, sky wrote: > On 2010/10/31 13:23:36, dill wrote: > > Should I pass the NavigationEntry* instead of the index as the ClientData or > use > > the NavigationEntry.unique_id? > > Using the NavigationEntry* is equally perilous as it is possible for some one to > delete it. Use the unique id. Also, write a unit test for the changes. Will do. Would it be appropriate then to add a GetEntryWithUniqueId to NavigationController?
On Mon, Nov 1, 2010 at 10:07 AM, <dill.sellars@gmail.com> wrote: > > http://codereview.chromium.org/2928005/diff/71001/72004 > File chrome/browser/back_forward_menu_model.cc (right): > > http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 > chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = > GetNavigationEntry(index); > On 2010/11/01 16:00:41, sky wrote: >> >> On 2010/10/31 13:23:36, dill wrote: >> > Should I pass the NavigationEntry* instead of the index as the > > ClientData or >> >> use >> > the NavigationEntry.unique_id? > >> Using the NavigationEntry* is equally perilous as it is possible for > > some one to >> >> delete it. Use the unique id. Also, write a unit test for the changes. > > Will do. Would it be appropriate then to add a GetEntryWithUniqueId to > NavigationController? Yes, that is fine. -Scott
On 2010/11/01 17:17:49, sky wrote: > On Mon, Nov 1, 2010 at 10:07 AM, <mailto:dill.sellars@gmail.com> wrote: > > > > http://codereview.chromium.org/2928005/diff/71001/72004 > > File chrome/browser/back_forward_menu_model.cc (right): > > > > http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 > > chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = > > GetNavigationEntry(index); > > On 2010/11/01 16:00:41, sky wrote: > >> > >> On 2010/10/31 13:23:36, dill wrote: > >> > Should I pass the NavigationEntry* instead of the index as the > > > > ClientData or > >> > >> use > >> > the NavigationEntry.unique_id? > > > >> Using the NavigationEntry* is equally perilous as it is possible for > > > > some one to > >> > >> delete it. Use the unique id. Also, write a unit test for the changes. > > > > Will do. Would it be appropriate then to add a GetEntryWithUniqueId to > > NavigationController? > > Yes, that is fine. > > -Scott On the topic of unit testing - in order to test FetchFavicon I need to use the browser and this is not currently initialized in BackFwdMenuModelTest. Should I change BackFwdMenuModelTest to extend BrowserWithTestWindowTest instead of RenderViewHostTestHarness or perhaps take a different approach to mock the browser / profile / history service?
On Sun, Nov 21, 2010 at 10:42 PM, <dill.sellars@gmail.com> wrote: > On 2010/11/01 17:17:49, sky wrote: >> >> On Mon, Nov 1, 2010 at 10:07 AM, <mailto:dill.sellars@gmail.com> wrote: >> > >> > http://codereview.chromium.org/2928005/diff/71001/72004 >> > File chrome/browser/back_forward_menu_model.cc (right): >> > >> > http://codereview.chromium.org/2928005/diff/71001/72004#newcode152 >> > chrome/browser/back_forward_menu_model.cc:152: NavigationEntry* entry = >> > GetNavigationEntry(index); >> > On 2010/11/01 16:00:41, sky wrote: >> >> >> >> On 2010/10/31 13:23:36, dill wrote: >> >> > Should I pass the NavigationEntry* instead of the index as the >> > >> > ClientData or >> >> >> >> use >> >> > the NavigationEntry.unique_id? >> > >> >> Using the NavigationEntry* is equally perilous as it is possible for >> > >> > some one to >> >> >> >> delete it. Use the unique id. Also, write a unit test for the changes. >> > >> > Will do. Would it be appropriate then to add a GetEntryWithUniqueId to >> > NavigationController? > >> Yes, that is fine. > >> -Scott > > On the topic of unit testing - in order to test FetchFavicon I need to use > the > browser and this is not currently initialized in BackFwdMenuModelTest. > Should I > change BackFwdMenuModelTest to extend BrowserWithTestWindowTest instead of > RenderViewHostTestHarness or perhaps take a different approach to mock the > browser / profile / history service? Why do you need the browser? RenderViewHostTestHarness has a TestingProfile which you can make have a faviconservice/history. -Scott
Updated code based on review comments. Removed native UI implementations to concentrate on core model / app changes. Added unit test. The unit test has been added as a new file because I could not get the message loop working with BackFwdMenuModelTest / RenderViewHostTestHarness, the tests wouldn't work or would pass but then hang at the end of the test.
On 2010/12/26 16:44:05, dill wrote: > Updated code based on review comments. Removed native UI implementations to > concentrate on core model / app changes. Added unit test. The unit test has been > added as a new file because I could not get the message loop working with > BackFwdMenuModelTest / RenderViewHostTestHarness, the tests wouldn't work or > would pass but then hang at the end of the test. Did you try debugging to see what the failure is? -Scott
http://codereview.chromium.org/2928005/diff/93001/app/menus/menu_model.h File app/menus/menu_model.h (right): http://codereview.chromium.org/2928005/diff/93001/app/menus/menu_model.h#newc... app/menus/menu_model.h:44: virtual void OnIconChanged(int model_index) {} model_index -> index. No where else does the API talk in terms of model indices. http://codereview.chromium.org/2928005/diff/93001/chrome/browser/ui/toolbar/b... File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/93001/chrome/browser/ui/toolbar/b... chrome/browser/ui/toolbar/back_forward_menu_model.cc:238: int unique_id = load_consumer_.GetClientDataForCurrentRequest(); Finding the navigation entry is cheap, but decoding the image isn't. So, find the navigation entry first. http://codereview.chromium.org/2928005/diff/93001/chrome/browser/ui/toolbar/b... chrome/browser/ui/toolbar/back_forward_menu_model.cc:252: if (!entry) Notice that even if you didn't find the matching NavigationEntry, entry is always non-null when you exit the for loop (well, as long as there is at least one navigation entry). http://codereview.chromium.org/2928005/diff/93001/chrome/browser/ui/toolbar/b... File chrome/browser/ui/toolbar/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/93001/chrome/browser/ui/toolbar/b... chrome/browser/ui/toolbar/back_forward_menu_model.h:77: // Callback for the favicon service. for -> from
On 2011/01/04 21:51:04, sky wrote:
> On 2010/12/26 16:44:05, dill wrote:
> > Updated code based on review comments. Removed native UI implementations to
> > concentrate on core model / app changes. Added unit test. The unit test has
> been
> > added as a new file because I could not get the message loop working with
> > BackFwdMenuModelTest / RenderViewHostTestHarness, the tests wouldn't work or
> > would pass but then hang at the end of the test.
>
> Did you try debugging to see what the failure is?
>
> -Scott
Stack trace below. On TearDown, TestingProfile is deleted by the message loop
and in DestroyHistoryService it posts another task to the message loop and calls
MessageLoop::Run() again. Enabling nested tasks from the unit test doesn't help.
I am now calling DestroyHistoryService as the last line in my unit test so that
the TestingProfile deconstructor is happy.
unit_tests.exe!base::MessagePumpForUI::ProcessNextWindowsMessage() Line
327 C++
unit_tests.exe!base::MessagePumpForUI::DoRunLoop() Line 197 + 0x8 bytes C++
unit_tests.exe!base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate
* delegate=0x0ae69010, base::MessagePumpWin::Dispatcher * dispatcher=0x00000000)
Line 49 + 0xf bytes C++
unit_tests.exe!base::MessagePumpWin::Run(base::MessagePump::Delegate *
delegate=0x0ae69010) Line 79 + 0x1c bytes C++
unit_tests.exe!MessageLoop::RunInternal() Line 269 + 0x2a bytes C++
unit_tests.exe!MessageLoop::RunHandler() Line 242 C++
unit_tests.exe!MessageLoop::Run() Line 220 C++
unit_tests.exe!TestingProfile::DestroyHistoryService() Line 227 C++
unit_tests.exe!TestingProfile::~TestingProfile() Line 184 C++
unit_tests.exe!TestingProfile::`scalar deleting destructor'() + 0x16
bytes C++
unit_tests.exe!DeleteTask<TestingProfile>::Run() Line 186 + 0x25 bytes C++
unit_tests.exe!MessageLoop::RunTask(Task * task=0x0ae852a0) Line 421 + 0xf
bytes C++
unit_tests.exe!MessageLoop::DeferOrRunPendingTask(const
MessageLoop::PendingTask & pending_task={...}) Line 433 C++
unit_tests.exe!MessageLoop::DoWork() Line 537 + 0xc bytes C++
unit_tests.exe!base::MessagePumpForUI::DoRunLoop() Line 201 + 0x1d bytes C++
unit_tests.exe!base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate
* delegate=0x0ae69010, base::MessagePumpWin::Dispatcher * dispatcher=0x00000000)
Line 49 + 0xf bytes C++
unit_tests.exe!base::MessagePumpWin::Run(base::MessagePump::Delegate *
delegate=0x0ae69010) Line 79 + 0x1c bytes C++
unit_tests.exe!MessageLoop::RunInternal() Line 269 + 0x2a bytes C++
unit_tests.exe!MessageLoop::RunHandler() Line 242 C++
unit_tests.exe!MessageLoop::RunAllPending() Line 226 C++
> unit_tests.exe!RenderViewHostTestHarness::TearDown() Line 307 C++
unit_tests.exe!testing::HandleExceptionsInMethodIfSupported<testing::Test,void>(testing::Test
* object=0x0ae69000, void (void)* method=0x021f49b0, const char *
location=0x05526568) Line 2130 + 0x8 bytes C++
unit_tests.exe!testing::Test::Run() Line 2153 + 0x13 bytes C++
unit_tests.exe!testing::TestInfo::Run() Line 2322 C++
unit_tests.exe!testing::TestCase::Run() Line 2424 C++
unit_tests.exe!testing::internal::UnitTestImpl::RunAllTests() Line 4200 C++
unit_tests.exe!testing::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>(testing::internal::UnitTestImpl
* object=0x09df5000, bool (void)* method=0x021dcc90, const char *
location=0x05526db8) Line 2130 + 0x8 bytes C++
unit_tests.exe!testing::UnitTest::Run() Line 3838 + 0x18 bytes C++
unit_tests.exe!base::TestSuite::Run() Line 125 + 0xc bytes C++
Addressed code review findings, moved unit test from it's own class into the existing BackFwdMenuModelTest.
http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc (right): http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:30: void MakeTestSkBitmap(int w, int h, SkBitmap* bmp) { See the code in top_sites_unittest CreateBitmap that does this. It's simpler. http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:42: class FaviconDelegate : public menus::MenuModel::Delegate { Keep this in the anonymous namespace. http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:44: explicit FaviconDelegate(MessageLoop* loop) : loop_(loop) { Does this really need to cache the loop? Can't it just quit the current loop? http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:462: TEST_F(BackFwdMenuModelTest, FaviconLoadTest) { Description? http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:465: Browser browser_(Browser::TYPE_NORMAL, profile()); browser_ -> browser http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:491: MessageLoop::current()->RunAllPending(); Why the RunAllPending here? http://codereview.chromium.org/2928005/diff/132001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:500: MessageLoop::current()->Run(); Shouldn't have an EXPECT that the delegate was called?
Unit test code review issues resolved.
The patch has been updated to work with the ui namespace refactor.
Patch updated to compile against changes in trunk.
Sorry for the long delay in responding. -Scott http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:214: void BackForwardMenuModel::FetchFavicon(NavigationEntry* entry, int index) { It doesn't look like you use index here, can it be nuked? http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:238: if (know_favicon && data.get() && data->size() && data->size() > 0) { How come there is data->size() && data->size() > 0 ? Isn't one enough? Also, make this an early return if false. http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:240: // We need both the NavigationEntry and the model_index so we search for This comment isn't too helpful. The useful comment to add is between 253 and 254 explaining why entry may be null. http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.h:77: // Callback from the favicon service. newline between 76 and 77. http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.h:184: friend class BackFwdMenuModelTest; Move all the friends to right after private: http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc (right): http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:28: SkBitmap CreateBitmap(SkColor color) { This shouldn't be indented. http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:466: scoped_ptr<BackForwardMenuModel> back_model(new BackForwardMenuModel( How come you put this in a scoped_ptr instead of on the stack? http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.h File ui/base/models/menu_model.h (right): http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.... ui/base/models/menu_model.h:39: class Delegate { Move this into its own file and call it MenuModelDelegate. We've been going with this rather than inlining to avoid extra includes. http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.... ui/base/models/menu_model.h:127: // Set the Delegate End with period, and mention not-owned. http://codereview.chromium.org/2928005/diff/159002/ui/base/models/simple_menu... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/159002/ui/base/models/simple_menu... ui/base/models/simple_menu_model.cc:308: } SimpleMenuModel should maintain the delegate as a field and provide a protected accessor to it that is inlined.
http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:214: void BackForwardMenuModel::FetchFavicon(NavigationEntry* entry, int index) { Yes, that was a leftover from when unique_id wasn't used. On 2011/02/22 18:28:28, sky wrote: > It doesn't look like you use index here, can it be nuked? http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:238: if (know_favicon && data.get() && data->size() && data->size() > 0) { data->size() > 0 was from an earlier code review, didn't make sense to me but I added it. I should have pushed back. Early return is already there, this if statement covers the rest of the function. On 2011/02/22 18:28:28, sky wrote: > How come there is data->size() && data->size() > 0 ? > Isn't one enough? > Also, make this an early return if false. http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:240: // We need both the NavigationEntry and the model_index so we search for How about here I say "Find the current model_index for the unique_id." and on line 253 I say "The NavigationEntry wasn't found, this can happen if the user navigates to another page and a NavigatationEntry falls out of the range of kMaxHistoryItems." On 2011/02/22 18:28:28, sky wrote: > This comment isn't too helpful. The useful comment to add is between 253 and 254 > explaining why entry may be null. http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc (right): http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:466: scoped_ptr<BackForwardMenuModel> back_model(new BackForwardMenuModel( Leftover from experimentation, back to stack. On 2011/02/22 18:28:28, sky wrote: > How come you put this in a scoped_ptr instead of on the stack? http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.h File ui/base/models/menu_model.h (right): http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.... ui/base/models/menu_model.h:39: class Delegate { This should go in ui\base\models, correct? On 2011/02/22 18:28:28, sky wrote: > Move this into its own file and call it MenuModelDelegate. We've been going with > this rather than inlining to avoid extra includes.
http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/159002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:240: // We need both the NavigationEntry and the model_index so we search for On 2011/02/24 15:00:28, dill wrote: > How about here I say "Find the current model_index for the unique_id." and on > line 253 I say "The NavigationEntry wasn't found, this can happen if the user > navigates to another page and a NavigatationEntry falls out of the range of > kMaxHistoryItems." > > On 2011/02/22 18:28:28, sky wrote: > > This comment isn't too helpful. The useful comment to add is between 253 and > 254 > > explaining why entry may be null. > Sounds good. http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.h File ui/base/models/menu_model.h (right): http://codereview.chromium.org/2928005/diff/159002/ui/base/models/menu_model.... ui/base/models/menu_model.h:39: class Delegate { On 2011/02/24 15:00:28, dill wrote: > This should go in ui\base\models, correct? > > On 2011/02/22 18:28:28, sky wrote: > > Move this into its own file and call it MenuModelDelegate. We've been going > with > > this rather than inlining to avoid extra includes. > yes.
http://codereview.chromium.org/2928005/diff/159002/ui/base/models/simple_menu... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/159002/ui/base/models/simple_menu... ui/base/models/simple_menu_model.cc:308: } On 2011/02/22 18:28:28, sky wrote: > SimpleMenuModel should maintain the delegate as a field and provide a protected > accessor to it that is inlined. BackForwardMenuModel extends ui::MenuModel, not ui::SimpleMenuModel so it can't use the field.
http://codereview.chromium.org/2928005/diff/159002/ui/base/models/simple_menu... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/159002/ui/base/models/simple_menu... ui/base/models/simple_menu_model.cc:308: } On 2011/02/25 15:16:43, dill wrote: > On 2011/02/22 18:28:28, sky wrote: > > SimpleMenuModel should maintain the delegate as a field and provide a > protected > > accessor to it that is inlined. > > BackForwardMenuModel extends ui::MenuModel, not ui::SimpleMenuModel so it can't > use the field. I don't realize that. Have the implementation in both then.
Patch updated.
On 2011/03/01 23:12:37, dill wrote: > Patch updated. ping
GAH! Sorry for being lame again. Can you sync up with trunk, make sure it works again and I'll take it over? Thanks, -Scott
Updated to compile with latest trunk.
All nits. I made these changes locally and will land it for you if it passes the bots. But you need to complete http://code.google.com/legal/individual-cla-v1.0.html before I can do that. -Scott http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.cc:238: if (favicon.known_icon && favicon.image_data.get() && This should be is_valid() http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model.h (right): http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model.h:194: ui::MenuModelDelegate* menu_model_delegate_; This isn't initialized in the constructor. http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc (right): http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:10: #include "content/browser/browser_thread.h" Should be around line 16. http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:38: explicit FaviconDelegate() : was_called_(false) {} no explicit http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:45: bool was_called() { return was_called_; } const http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:49: DISALLOW_COPY_AND_ASSIGN(FaviconDelegate); newline between 48 and 49. http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:497: &browser, BackForwardMenuModel::BACKWARD_MENU); indented 4 spaces. http://codereview.chromium.org/2928005/diff/172001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc:517: url1_favicon, icon_data, history::FAVICON); indented 4. http://codereview.chromium.org/2928005/diff/172001/ui/base/models/simple_menu... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/2928005/diff/172001/ui/base/models/simple_menu... ui/base/models/simple_menu_model.cc:249: bool SimpleMenuModel::GetIconAt(int index, SkBitmap* icon) { Position doesn't match header.
Thanks for finally getting this in. CLA submitted. I still need to submit the native menu implementations - do you want me to add those to this code review or start a new code review?
Do that in a separate cl. I haven't submitted the patch yet, will do so as soon as try bots find it agreeable. -Scott On Fri, Mar 18, 2011 at 11:56 AM, <dill.sellars@gmail.com> wrote: > Thanks for finally getting this in. CLA submitted. I still need to submit > the > native menu implementations - do you want me to add those to this code > review or > start a new code review? > > http://codereview.chromium.org/2928005/ > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
