Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7538022:
Jumplist Bug (Closed)
|
Created:
9 years, 4 months ago by Cait (Slow) Modified:
9 years, 4 months ago CC:
chromium-reviews, estade+watch_chromium.org, brettw-cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionExposed the Notification Observer interface in jumplist_win, so that it could receive notifications
and query TopSites for updates.
Also, refactored jumplist (and a bit of browser_view) so that jumplist would properly
handle receiving notifications from both TabService and TopSites.
BUG=77756
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97672
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 26
Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 25
Patch Set 10 : '' #
Total comments: 2
Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 7
Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 8
Patch Set 16 : '' #
Messages
Total messages: 32 (0 generated)
JumpList should just hook into topsites. Jumplist is not a specialization of a most visited handler, so it doesn't make sense that it's a subclass.
I don't understand... Aren't the most visited the top sites? What we were trying to do is avoid duplication of code since to get the top sites, we need to do what the most visited handler needs to do, right? This is why we created the base class... We could rename it to top sites instead of most visited if it makes more sense... Otherwise, I think I'm missing something here... Can you shed some light here? Thanks! On Fri, Aug 5, 2011 at 2:39 PM, <estade@chromium.org> wrote: > JumpList should just hook into topsites. Jumplist is not a specialization > of a > most visited handler, so it doesn't make sense that it's a subclass. > > > http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... > BYE MAD
TopSites is a class in chrome/browser/history which handles the logic of picking top sites from history, as well as blacklisting, pinning, etc. This is the location for logic that should be shared by the UI surfaces that expose the user's top sites. Most Visited is just the name of the NTP section.
OK, so it might just be a naming thing (though the top sites object seems to have some most visited terminology in it too TopSites::GetMostVisitedURLs() for example... So what we were trying to do is to create a base class for consumers of the topsites list... to share code between the different UI surfaces like the NTP and the jumplist for example... Maybe we can simply rework the terminology, or do you have concerns about the actual code we are trying to share in this base class? On Fri, Aug 5, 2011 at 2:53 PM, <estade@chromium.org> wrote: > TopSites is a class in chrome/browser/history which handles the logic of > picking > top sites from history, as well as blacklisting, pinning, etc. This is the > location for logic that should be shared by the UI surfaces that expose the > user's top sites. Most Visited is just the name of the NTP section. > > > http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >
Why can't the code you want to share live in TopSites?
We'll take a look, but it's all these notification hooks and parsing what top sites returns that we want to share, so I'm doubtful... But we'll take a second look... Thanks! On Fri, Aug 5, 2011 at 3:14 PM, <estade@chromium.org> wrote: > Why can't the code you want to share live in TopSites? > > > http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >
topsites is already a layer on top of history, we don't need an additional layer
on top of that. Look at ui/gtk/global_history_menu.{h,cc} for the Linux analog
to what you're doing.
I think if you do not want another layer then the best option is either to
replicate the notification hooks in MostVisitedHandler and JumpList. Most of
this code deals with communication with TopSites (requesting data after
receiving a notification, etc.), so certainly putting it in TopSites wouldn't
work...
Unless I hear otherwise, I'll start refactoring things to revert the changes to
the MostVisitedhandler.
On 2011/08/05 19:22:21, Evan Stade wrote:
> topsites is already a layer on top of history, we don't need an additional
layer
> on top of that. Look at ui/gtk/global_history_menu.{h,cc} for the Linux analog
> to what you're doing.
Wait, I want to take a closer look first, let's see how they do it on Linux first... On Fri, Aug 5, 2011 at 3:51 PM, <caitkp@chromium.org> wrote: > I think if you do not want another layer then the best option is either to > replicate the notification hooks in MostVisitedHandler and JumpList. Most > of > this code deals with communication with TopSites (requesting data after > receiving a notification, etc.), so certainly putting it in TopSites > wouldn't > work... > > Unless I hear otherwise, I'll start refactoring things to revert the > changes to > the MostVisitedhandler. > > > On 2011/08/05 19:22:21, Evan Stade wrote: > >> topsites is already a layer on top of history, we don't need an additional >> > layer > >> on top of that. Look at ui/gtk/global_history_menu.{h,**cc} for the Linux >> analog >> to what you're doing. >> > > > > http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >
OK, so it seems like the code we were trying to share is simply duplicated in the Linux version, and the sad part is that there seems to be a lot of similarities between the Windows and Linux versions of the jump list/global history menu... But I guess their also different enough... Ho Well... :-( So OK Cait, forget about trying to share code and simply duplicate what you need for interacting with the top sites... Sorry to have steered you in the wrong direction... On Fri, Aug 5, 2011 at 4:02 PM, Marc-Andre Decoste <mad@chromium.org> wrote: > Wait, I want to take a closer look first, let's see how they do it on Linux > first... > > > On Fri, Aug 5, 2011 at 3:51 PM, <caitkp@chromium.org> wrote: > >> I think if you do not want another layer then the best option is either to >> replicate the notification hooks in MostVisitedHandler and JumpList. Most >> of >> this code deals with communication with TopSites (requesting data after >> receiving a notification, etc.), so certainly putting it in TopSites >> wouldn't >> work... >> >> Unless I hear otherwise, I'll start refactoring things to revert the >> changes to >> the MostVisitedhandler. >> >> >> On 2011/08/05 19:22:21, Evan Stade wrote: >> >>> topsites is already a layer on top of history, we don't need an >>> additional >>> >> layer >> >>> on top of that. Look at ui/gtk/global_history_menu.{h,**cc} for the >>> Linux analog >>> to what you're doing. >>> >> >> >> >> http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >> > >
No worries. It looks like a pretty straightforward change.
On 2011/08/05 20:12:08, MAD wrote:
> OK, so it seems like the code we were trying to share is simply duplicated
> in the Linux version, and the sad part is that there seems to be a lot of
> similarities between the Windows and Linux versions of the jump list/global
> history menu... But I guess their also different enough... Ho Well... :-(
>
> So OK Cait, forget about trying to share code and simply duplicate what you
> need for interacting with the top sites... Sorry to have steered you in the
> wrong direction...
>
> On Fri, Aug 5, 2011 at 4:02 PM, Marc-Andre Decoste <mailto:mad@chromium.org>
wrote:
>
> > Wait, I want to take a closer look first, let's see how they do it on Linux
> > first...
> >
> >
> > On Fri, Aug 5, 2011 at 3:51 PM, <mailto:caitkp@chromium.org> wrote:
> >
> >> I think if you do not want another layer then the best option is either to
> >> replicate the notification hooks in MostVisitedHandler and JumpList. Most
> >> of
> >> this code deals with communication with TopSites (requesting data after
> >> receiving a notification, etc.), so certainly putting it in TopSites
> >> wouldn't
> >> work...
> >>
> >> Unless I hear otherwise, I'll start refactoring things to revert the
> >> changes to
> >> the MostVisitedhandler.
> >>
> >>
> >> On 2011/08/05 19:22:21, Evan Stade wrote:
> >>
> >>> topsites is already a layer on top of history, we don't need an
> >>> additional
> >>>
> >> layer
> >>
> >>> on top of that. Look at ui/gtk/global_history_menu.{h,**cc} for the
> >>> Linux analog
> >>> to what you're doing.
> >>>
> >>
> >>
> >>
> >>
>
http://codereview.chromium.**org/7538022/%3Chttp://codereview.chromium.org/75...>
> >>
> >
> >
See Patch set 7 for refactored version: Reverted MostVisitedHandler and moved
TopSites hooks to JumpList.
On 2011/08/05 20:21:15, Cait Phillips wrote:
> No worries. It looks like a pretty straightforward change.
>
> On 2011/08/05 20:12:08, MAD wrote:
> > OK, so it seems like the code we were trying to share is simply duplicated
> > in the Linux version, and the sad part is that there seems to be a lot of
> > similarities between the Windows and Linux versions of the jump list/global
> > history menu... But I guess their also different enough... Ho Well... :-(
> >
> > So OK Cait, forget about trying to share code and simply duplicate what you
> > need for interacting with the top sites... Sorry to have steered you in the
> > wrong direction...
> >
> > On Fri, Aug 5, 2011 at 4:02 PM, Marc-Andre Decoste <mailto:mad@chromium.org>
> wrote:
> >
> > > Wait, I want to take a closer look first, let's see how they do it on
Linux
> > > first...
> > >
> > >
> > > On Fri, Aug 5, 2011 at 3:51 PM, <mailto:caitkp@chromium.org> wrote:
> > >
> > >> I think if you do not want another layer then the best option is either
to
> > >> replicate the notification hooks in MostVisitedHandler and JumpList. Most
> > >> of
> > >> this code deals with communication with TopSites (requesting data after
> > >> receiving a notification, etc.), so certainly putting it in TopSites
> > >> wouldn't
> > >> work...
> > >>
> > >> Unless I hear otherwise, I'll start refactoring things to revert the
> > >> changes to
> > >> the MostVisitedhandler.
> > >>
> > >>
> > >> On 2011/08/05 19:22:21, Evan Stade wrote:
> > >>
> > >>> topsites is already a layer on top of history, we don't need an
> > >>> additional
> > >>>
> > >> layer
> > >>
> > >>> on top of that. Look at ui/gtk/global_history_menu.{h,**cc} for the
> > >>> Linux analog
> > >>> to what you're doing.
> > >>>
> > >>
> > >>
> > >>
> > >>
> >
>
http://codereview.chromium.**org/7538022/%253Chttp://codereview.chromium.org/...>
> > >>
> > >
> > >
thank you http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:555: &JumpList::OnMostVisitedURLsAvailable)); too much indent. Can this fit on the line above? http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:705: // This data will be decoded by JumpListUpdateTask. comment out of date http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:729: ShellLinkItemList local_recently_closed_pages; please sprinkle in some newlines for readability http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:752: for (ShellLinkItemList::const_iterator item = local_most_visited_pages.begin(); 80 http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:767: for (ShellLinkItemList::const_iterator item = local_recently_closed_pages.begin(); 80 http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:776: (*item)->SetIcon(icon_path.value(), 0, true); refactor this loop into helper function (it seems it's identical to the one above less the vector)
done. On 2011/08/05 20:39:03, Evan Stade wrote: > thank you > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win.cc > File chrome/browser/jumplist_win.cc (right): > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:555: &JumpList::OnMostVisitedURLsAvailable)); > too much indent. Can this fit on the line above? > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:705: // This data will be decoded by > JumpListUpdateTask. > comment out of date > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:729: ShellLinkItemList > local_recently_closed_pages; > please sprinkle in some newlines for readability > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:752: for (ShellLinkItemList::const_iterator item > = local_most_visited_pages.begin(); > 80 > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:767: for (ShellLinkItemList::const_iterator item > = local_recently_closed_pages.begin(); > 80 > > http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:776: (*item)->SetIcon(icon_path.value(), 0, > true); > refactor this loop into helper function (it seems it's identical to the one > above less the vector)
Some more style and other comments... Well done! BYE MAD http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:485: :handle_(NULL) { Single line... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:507: profile_ = profile; No need to move that up anymore, might as well put it back where it was to minimize the change... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:515: history::TopSites* ts = profile_->GetTopSites(); I would prefer a more meaningful variable name than just ts, I would use top_sites instead. http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:535: // Most visited urls changed, query again. Most visited -> Top sites http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:550: history::TopSites* ts = profile_->GetTopSites(); Again, ts -> top_sites, please... :-) http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:565: profile_->GetFaviconService(Profile::EXPLICIT_ACCESS); Need 4 more spaces of indent... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:592: profile_->GetFaviconService(Profile::EXPLICIT_ACCESS); Again, 4 more indent spaces please... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:622: &recently_closed_pages_, kRecentlyClosedCount); I'm not comfortable passing the pointers to protected lists in there and then having these methods protect access to a list passed as an argument... I would rather change these methods to not receive a list, and work directly with the data member, it's the only usage we have anyway... It would the code much clearer... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:679: // Ask FaviconService if it has a favicon of a URL. I would bring back this comment where it was... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:681: if (icon_urls_.empty()) Even though this is atomic, I would prefer we did it within the lock. http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:700: handle_ = NULL; Maybe add a comment to specify why we do this? http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:721: this->AddRef(); Please add a comment stating who/where will release this ref. http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:737: ShellLinkItemList temp_most_visited_pages(most_visited_pages_); Why do we need these temps? Can't we simply do: local_most_visited_pages = most_visited_pages_; local_recently_closed_pages = recently_closed_pages_; And maybe we could clear the data member lists here? http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (right): http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:21: #include "chrome/browser/ui/webui/ntp/base_most_visited_handler.h" This doesn't exist anymore... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:103: // This class also implements BaseMostVisitedHandler. So by calling Update this comment, base class doesn't exist anymore... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:183: // Callback for TopSites. BaseMostVisitedHandler that notifies when the "Most Mention of removed base class again... http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:188: const history::MostVisitedURLList& data); Bad indent, should be only 4 more spaces than previous line. http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:225: // as they may be used by up to 3 threads. We should also add comments to the data members protected by this lock to mention that they must only be accessed within this lock.
Revised according to MAD's comments http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:721: this->AddRef(); On 2011/08/05 21:21:43, MAD wrote: > Please add a comment stating who/where will release this ref. Actually..this AddRef is not supposed to be here..my bad. http://codereview.chromium.org/7538022/diff/13006/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:737: ShellLinkItemList temp_most_visited_pages(most_visited_pages_); The data member lists only get cleared immediately before they are updated. Otherwise in the notification , which ever list *wasn't* updated will remain empty and overwrite the current list On 2011/08/05 21:21:43, MAD wrote: > Why do we need these temps? Can't we simply do: > local_most_visited_pages = most_visited_pages_; > local_recently_closed_pages = recently_closed_pages_; > > And maybe we could clear the data member lists here?
LGTM
A few more comments... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (left): http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:577: JumpList::JumpList() : profile_(NULL) { We should still set the profile to NULL here... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:549: void JumpList::StartQueryForTopSites() { I think this code is small enough and unlikely to be reused, that we can actually have it directly within the Observe implementation... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:590: FaviconService* favicon_service = Maybe extract this common code to a method... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:598: recently_closed_pages_.clear(); I'm a it uncomfortable with the isolated locks here, and also the full lock within AddTab below (more details in there)... I'm afraid we might get calls in between in the other thread when the list will be in the process of being filled... Maybe we should try to fill a local list instead (sorry, this would mean to bring back the extra argument for the list that I asked you to remove, I should have thought about this at first... :-( ) This way, we could reset the list in one shot, within a single lock/unlock section... That would be safer I think... Unless I'm missing something... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:640: if (tab->navigations.empty() || recently_closed_pages_.size() >= max_items) If we would keep this within a lock (which we might not based on a previous comment above), we should add a comment that accessing tab->navigations is currently safe to do within a lock, but if this changes to a method call on the TabRestoreService object, it might become unsafe to do so... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:649: std::string url = current_navigation.virtual_url().spec(); Not sure if it is safe to call virtual_url on the TabNavigation object within a lock... Most likely is sure... But... maybe not... Should at least have a comment saying it if we keep the lock in this method... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:699: // If there is currently an favicon request in progress, it is now outdated, an favicon -> a favicon http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:720: // We create a RunnableMethod that creates icon files, and we post it to We don't actually "create" a runnable method... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:722: this->AddRef(); Do we need this??? If so, please add a comment... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:725: NewRunnableMethod(this, &JumpList::RunUpdate)); Alignment should be kept as it was, i.e., at the same level as previous line... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:769: item != item_list.end(); ++item) { indent two more spaces please... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (right): http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:18: #include "chrome/browser/history/top_sites.h" Do we need this include here? http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:21: #include "chrome/browser/ui/webui/ntp/base_most_visited_handler.h" Please remove deprecated most visited base class inclusion and mentions in comments... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:105: // JumpList is updated when a top site is added or blacklisted. I think this whole paragraph can go away... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:109: // task when it actually updates a JumpList. (This task is implemented in an Please update comment since we now use a runnable method instead of a separate task class. http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:176: // Callback for TopSites. BaseMostVisitedHandler that notifies when the "Most Please update comment. http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:225: // as they may be used by up to 3 threads. Please mention in comments above these data members that they are (and must stay) protected by a the list_lock_
Updated as per MAD's comments, except for the AddRef issue (see comment). http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:722: this->AddRef(); We *shouldn't* need this (I think RunnableMethod handles adding and removing refs from the object on which the method is being called), but when I remove it, I get errors when the jumplist deconstructor is called.... On 2011/08/15 16:16:16, MAD wrote: > Do we need this??? If so, please add a comment...
I'd be curious to see this error... I can't see what we are doing wrong to get to need this extra AddRef()... I'll try your patch and see what I can find... On Mon, Aug 15, 2011 at 3:44 PM, <caitkp@chromium.org> wrote: > Updated as per MAD's comments, except for the AddRef issue (see comment). > > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc> > File chrome/browser/jumplist_win.cc (right): > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.cc#**newcode722<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc#newcode722> > chrome/browser/jumplist_win.**cc:722: this->AddRef(); > We *shouldn't* need this (I think RunnableMethod handles adding and > removing refs from the object on which the method is being called), but > when I remove it, I get errors when the jumplist deconstructor is > called.... > > > On 2011/08/15 16:16:16, MAD wrote: > >> Do we need this??? If so, please add a comment... >> > > http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >
OK, I just tried it, and the problem is not the missing AddRef(), it's the fact that we may have a pending notification when we shutdown, and thus we get to handle it and be destroyed after the profile got destroyed... Adding an AddRef() kind of hides the problem and causes a leak... I think the proper way to fix this is to add a terminate method to the jump list class and call it before the scope_refptr releases the main ref held by the browser... In this terminate method, we should cancel all pending tasks to avoid these tasks being executed to late in the browser shutdown process... On Mon, Aug 15, 2011 at 4:18 PM, Marc-Andre Decoste <mad@chromium.org>wrote: > I'd be curious to see this error... I can't see what we are doing wrong to > get to need this extra AddRef()... I'll try your patch and see what I can > find... > > > On Mon, Aug 15, 2011 at 3:44 PM, <caitkp@chromium.org> wrote: > >> Updated as per MAD's comments, except for the AddRef issue (see comment). >> >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc> >> File chrome/browser/jumplist_win.cc (right): >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.cc#**newcode722<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc#newcode722> >> chrome/browser/jumplist_win.**cc:722: this->AddRef(); >> We *shouldn't* need this (I think RunnableMethod handles adding and >> removing refs from the object on which the method is being called), but >> when I remove it, I get errors when the jumplist deconstructor is >> called.... >> >> >> On 2011/08/15 16:16:16, MAD wrote: >> >>> Do we need this??? If so, please add a comment... >>> >> >> http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >> > >
I think you missed the series of comments I made in the jumplist header file... And one at the end of the cc file... I also have a couple more in the cc file... Thanks! BYE MAD ... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:769: item != item_list.end(); ++item) { On 2011/08/15 16:16:16, MAD wrote: > indent two more spaces please... You missed this one too... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (right): http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:18: #include "chrome/browser/history/top_sites.h" On 2011/08/15 16:16:16, MAD wrote: > Do we need this include here? Here... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:21: #include "chrome/browser/ui/webui/ntp/base_most_visited_handler.h" On 2011/08/15 16:16:16, MAD wrote: > Please remove deprecated most visited base class inclusion and mentions in > comments... Here... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:105: // JumpList is updated when a top site is added or blacklisted. On 2011/08/15 16:16:16, MAD wrote: > I think this whole paragraph can go away... Here... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:109: // task when it actually updates a JumpList. (This task is implemented in an On 2011/08/15 16:16:16, MAD wrote: > Please update comment since we now use a runnable method instead of a separate > task class. Here... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:176: // Callback for TopSites. BaseMostVisitedHandler that notifies when the "Most On 2011/08/15 16:16:16, MAD wrote: > Please update comment. Here... http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:225: // as they may be used by up to 3 threads. On 2011/08/15 16:16:16, MAD wrote: > Please mention in comments above these data members that they are (and must > stay) protected by a the list_lock_ Here... http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:485: , handle_(NULL) { fits on a single line... otherwise, we prefer this alignment: JumpList::JumpList() : profile_(NULL), handle_(NULL) { when things don't fit on a single line... http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:680: } I think we can merge these two locked blocks...
Hi MAD, Thanks for the suggestions. I tried adding a terminate() method in JumpList (currently it gets called from BrowserView deconstructor), however, I'm still getting errors (about invalid threads now). It seems like the pain point is the RemoveObserver() method of JumpList. Can we take a look at this together tomorrow, as I am quite stumped by it... Thanks, Cait On Mon, Aug 15, 2011 at 5:52 PM, <mad@chromium.org> wrote: > I think you missed the series of comments I made in the jumplist header > file... > And one at the end of the cc file... > > I also have a couple more in the cc file... > > Thanks! > > BYE > MAD > ... > > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc> > File chrome/browser/jumplist_win.cc (right): > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.cc#**newcode769<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc#newcode769> > chrome/browser/jumplist_win.**cc:769: item != item_list.end(); ++item) { > On 2011/08/15 16:16:16, MAD wrote: > >> indent two more spaces please... >> > > You missed this one too... > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h> > File chrome/browser/jumplist_win.h (right): > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h#**newcode18<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode18> > chrome/browser/jumplist_win.h:**18: #include > "chrome/browser/history/top_**sites.h" > On 2011/08/15 16:16:16, MAD wrote: > >> Do we need this include here? >> > > Here... > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h#**newcode21<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode21> > chrome/browser/jumplist_win.h:**21: #include > "chrome/browser/ui/webui/ntp/**base_most_visited_handler.h" > On 2011/08/15 16:16:16, MAD wrote: > >> Please remove deprecated most visited base class inclusion and >> > mentions in > >> comments... >> > > Here... > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h#**newcode105<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode105> > chrome/browser/jumplist_win.h:**105: // JumpList is updated when a top > site is added or blacklisted. > On 2011/08/15 16:16:16, MAD wrote: > >> I think this whole paragraph can go away... >> > > Here... > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h#**newcode109<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode109> > chrome/browser/jumplist_win.h:**109: // task when it actually updates a > JumpList. (This task is implemented in an > On 2011/08/15 16:16:16, MAD wrote: > >> Please update comment since we now use a runnable method instead of a >> > separate > >> task class. >> > > Here... > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h#**newcode176<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode176> > chrome/browser/jumplist_win.h:**176: // Callback for TopSites. > BaseMostVisitedHandler that notifies when the "Most > On 2011/08/15 16:16:16, MAD wrote: > >> Please update comment. >> > > Here... > > > http://codereview.chromium.**org/7538022/diff/22001/chrome/** > browser/jumplist_win.h#**newcode225<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode225> > chrome/browser/jumplist_win.h:**225: // as they may be used by up to 3 > threads. > On 2011/08/15 16:16:16, MAD wrote: > >> Please mention in comments above these data members that they are (and >> > must > >> stay) protected by a the list_lock_ >> > > Here... > > http://codereview.chromium.**org/7538022/diff/30001/chrome/** > browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc> > > File chrome/browser/jumplist_win.cc (right): > > http://codereview.chromium.**org/7538022/diff/30001/chrome/** > browser/jumplist_win.cc#**newcode485<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc#newcode485> > chrome/browser/jumplist_win.**cc:485: , handle_(NULL) { > fits on a single line... otherwise, we prefer this alignment: > > JumpList::JumpList() > : profile_(NULL), > handle_(NULL) { > > when things don't fit on a single line... > http://google-styleguide.**googlecode.com/svn/trunk/** > cppguide.xml?showone=**Constructor_Initializer_Lists#** > Constructor_Initializer_Lists<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists> > > http://codereview.chromium.**org/7538022/diff/30001/chrome/** > browser/jumplist_win.cc#**newcode680<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc#newcode680> > chrome/browser/jumplist_win.**cc:680: } > I think we can merge these two locked blocks... > > > http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >
Yeah, I think we need to find how to make sure we don't remove observers after the profile has been destroyed... Maybe there is a notification sent before destroying profile... We should look for that and see if we can use it... I'm currently busy with something else I broke yesterday and need to fix ASAP... I'll get back to this right after... On Mon, Aug 15, 2011 at 8:08 PM, Cait Phillips <caitkp@chromium.org> wrote: > Hi MAD, > > Thanks for the suggestions. I tried adding a terminate() method in JumpList > (currently it gets called from BrowserView deconstructor), however, I'm > still getting errors (about invalid threads now). It seems like the pain > point is the RemoveObserver() method of JumpList. Can we take a look at this > together tomorrow, as I am quite stumped by it... > > Thanks, > Cait > > > On Mon, Aug 15, 2011 at 5:52 PM, <mad@chromium.org> wrote: > >> I think you missed the series of comments I made in the jumplist header >> file... >> And one at the end of the cc file... >> >> I also have a couple more in the cc file... >> >> Thanks! >> >> BYE >> MAD >> ... >> >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc> >> File chrome/browser/jumplist_win.cc (right): >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.cc#**newcode769<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc#newcode769> >> chrome/browser/jumplist_win.**cc:769: item != item_list.end(); ++item) { >> On 2011/08/15 16:16:16, MAD wrote: >> >>> indent two more spaces please... >>> >> >> You missed this one too... >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h> >> File chrome/browser/jumplist_win.h (right): >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h#**newcode18<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode18> >> chrome/browser/jumplist_win.h:**18: #include >> "chrome/browser/history/top_**sites.h" >> On 2011/08/15 16:16:16, MAD wrote: >> >>> Do we need this include here? >>> >> >> Here... >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h#**newcode21<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode21> >> chrome/browser/jumplist_win.h:**21: #include >> "chrome/browser/ui/webui/ntp/**base_most_visited_handler.h" >> On 2011/08/15 16:16:16, MAD wrote: >> >>> Please remove deprecated most visited base class inclusion and >>> >> mentions in >> >>> comments... >>> >> >> Here... >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h#**newcode105<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode105> >> chrome/browser/jumplist_win.h:**105: // JumpList is updated when a top >> site is added or blacklisted. >> On 2011/08/15 16:16:16, MAD wrote: >> >>> I think this whole paragraph can go away... >>> >> >> Here... >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h#**newcode109<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode109> >> chrome/browser/jumplist_win.h:**109: // task when it actually updates a >> JumpList. (This task is implemented in an >> On 2011/08/15 16:16:16, MAD wrote: >> >>> Please update comment since we now use a runnable method instead of a >>> >> separate >> >>> task class. >>> >> >> Here... >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h#**newcode176<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode176> >> chrome/browser/jumplist_win.h:**176: // Callback for TopSites. >> BaseMostVisitedHandler that notifies when the "Most >> On 2011/08/15 16:16:16, MAD wrote: >> >>> Please update comment. >>> >> >> Here... >> >> >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** >> browser/jumplist_win.h#**newcode225<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode225> >> chrome/browser/jumplist_win.h:**225: // as they may be used by up to 3 >> threads. >> On 2011/08/15 16:16:16, MAD wrote: >> >>> Please mention in comments above these data members that they are (and >>> >> must >> >>> stay) protected by a the list_lock_ >>> >> >> Here... >> >> http://codereview.chromium.**org/7538022/diff/30001/chrome/** >> browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc> >> >> File chrome/browser/jumplist_win.cc (right): >> >> http://codereview.chromium.**org/7538022/diff/30001/chrome/** >> browser/jumplist_win.cc#**newcode485<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc#newcode485> >> chrome/browser/jumplist_win.**cc:485: , handle_(NULL) { >> fits on a single line... otherwise, we prefer this alignment: >> >> JumpList::JumpList() >> : profile_(NULL), >> handle_(NULL) { >> >> when things don't fit on a single line... >> http://google-styleguide.**googlecode.com/svn/trunk/** >> cppguide.xml?showone=**Constructor_Initializer_Lists#** >> Constructor_Initializer_Lists<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists> >> >> http://codereview.chromium.**org/7538022/diff/30001/chrome/** >> browser/jumplist_win.cc#**newcode680<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc#newcode680> >> chrome/browser/jumplist_win.**cc:680: } >> I think we can merge these two locked blocks... >> >> >> http://codereview.chromium.**org/7538022/<http://codereview.chromium.org/7538... >> > >
So turns out that I forgot to build the last fix (the terminate method) I tried last night. Which would explain why it didn't work...anyways, it is working now (Patch set 12). On 2011/08/16 14:36:14, MAD wrote: > Yeah, I think we need to find how to make sure we don't remove observers > after the profile has been destroyed... > > Maybe there is a notification sent before destroying profile... We should > look for that and see if we can use it... > > I'm currently busy with something else I broke yesterday and need to fix > ASAP... I'll get back to this right after... > > On Mon, Aug 15, 2011 at 8:08 PM, Cait Phillips <mailto:caitkp@chromium.org> wrote: > > > Hi MAD, > > > > Thanks for the suggestions. I tried adding a terminate() method in JumpList > > (currently it gets called from BrowserView deconstructor), however, I'm > > still getting errors (about invalid threads now). It seems like the pain > > point is the RemoveObserver() method of JumpList. Can we take a look at this > > together tomorrow, as I am quite stumped by it... > > > > Thanks, > > Cait > > > > > > On Mon, Aug 15, 2011 at 5:52 PM, <mailto:mad@chromium.org> wrote: > > > >> I think you missed the series of comments I made in the jumplist header > >> file... > >> And one at the end of the cc file... > >> > >> I also have a couple more in the cc file... > >> > >> Thanks! > >> > >> BYE > >> MAD > >> ... > >> > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc> > >> File chrome/browser/jumplist_win.cc (right): > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.cc#**newcode769<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.cc#newcode769> > >> chrome/browser/jumplist_win.**cc:769: item != item_list.end(); ++item) { > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> indent two more spaces please... > >>> > >> > >> You missed this one too... > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h> > >> File chrome/browser/jumplist_win.h (right): > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h#**newcode18<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode18> > >> chrome/browser/jumplist_win.h:**18: #include > >> "chrome/browser/history/top_**sites.h" > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> Do we need this include here? > >>> > >> > >> Here... > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h#**newcode21<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode21> > >> chrome/browser/jumplist_win.h:**21: #include > >> "chrome/browser/ui/webui/ntp/**base_most_visited_handler.h" > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> Please remove deprecated most visited base class inclusion and > >>> > >> mentions in > >> > >>> comments... > >>> > >> > >> Here... > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h#**newcode105<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode105> > >> chrome/browser/jumplist_win.h:**105: // JumpList is updated when a top > >> site is added or blacklisted. > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> I think this whole paragraph can go away... > >>> > >> > >> Here... > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h#**newcode109<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode109> > >> chrome/browser/jumplist_win.h:**109: // task when it actually updates a > >> JumpList. (This task is implemented in an > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> Please update comment since we now use a runnable method instead of a > >>> > >> separate > >> > >>> task class. > >>> > >> > >> Here... > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h#**newcode176<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode176> > >> chrome/browser/jumplist_win.h:**176: // Callback for TopSites. > >> BaseMostVisitedHandler that notifies when the "Most > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> Please update comment. > >>> > >> > >> Here... > >> > >> > >> http://codereview.chromium.**org/7538022/diff/22001/chrome/** > >> > browser/jumplist_win.h#**newcode225<http://codereview.chromium.org/7538022/diff/22001/chrome/browser/jumplist_win.h#newcode225> > >> chrome/browser/jumplist_win.h:**225: // as they may be used by up to 3 > >> threads. > >> On 2011/08/15 16:16:16, MAD wrote: > >> > >>> Please mention in comments above these data members that they are (and > >>> > >> must > >> > >>> stay) protected by a the list_lock_ > >>> > >> > >> Here... > >> > >> http://codereview.chromium.**org/7538022/diff/30001/chrome/** > >> > browser/jumplist_win.cc<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc> > >> > >> File chrome/browser/jumplist_win.cc (right): > >> > >> http://codereview.chromium.**org/7538022/diff/30001/chrome/** > >> > browser/jumplist_win.cc#**newcode485<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc#newcode485> > >> chrome/browser/jumplist_win.**cc:485: , handle_(NULL) { > >> fits on a single line... otherwise, we prefer this alignment: > >> > >> JumpList::JumpList() > >> : profile_(NULL), > >> handle_(NULL) { > >> > >> when things don't fit on a single line... > >> http://google-styleguide.**googlecode.com/svn/trunk/** > >> cppguide.xml?showone=**Constructor_Initializer_Lists#** > >> > Constructor_Initializer_Lists<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists> > >> > >> http://codereview.chromium.**org/7538022/diff/30001/chrome/** > >> > browser/jumplist_win.cc#**newcode680<http://codereview.chromium.org/7538022/diff/30001/chrome/browser/jumplist_win.cc#newcode680> > >> chrome/browser/jumplist_win.**cc:680: } > >> I think we can merge these two locked blocks... > >> > >> > >> > http://codereview.chromium.**org/7538022/%3Chttp://codereview.chromium.org/75...> > >> > > > >
Almost there... Just a few more style nit picks... Thanks! BYE MAD http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (left): http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:581: RemoveObserver(); I would add a call to terminate here, just to be on the safe side... It's a noop when we are already terminated anyway... http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (right): http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:135: // Terminate. Maybe augment the comment to specify that this must be called before the death of the profile we were given in AddObserver. http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:181: const history::MostVisitedURLList& data); Should only 4 chars indent from where void starts. http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:185: void JumpList::RunUpdate(); No need for JumpList:: http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:188: // loaded icons missing a '.' at the end... http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:213: // is called), they are and must remain, protected by the list_lock_. You could simply add "Protected by list_lock_" on a separate comment line, but it should be added to all the data members that are protected. http://codereview.chromium.org/7538022/diff/28004/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/7538022/diff/28004/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:357: if (JumpList::Enabled()) { Here, you should rely on jumplist_ not being NULL instead... And maybe add a comment stating that this must be called before we destroy the profile used by the jumplist...
Fixed remaining style issues and added code to handle profile destruction issues. If all looks good, please submit a try (as I am unable to...) -Cait On 2011/08/16 17:02:30, MAD wrote: > Almost there... > > Just a few more style nit picks... > > Thanks! > > BYE > MAD > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win.cc > File chrome/browser/jumplist_win.cc (left): > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:581: RemoveObserver(); > I would add a call to terminate here, just to be on the safe side... It's a noop > when we are already terminated anyway... > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win.h > File chrome/browser/jumplist_win.h (right): > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:135: // Terminate. > Maybe augment the comment to specify that this must be called before the death > of the profile we were given in AddObserver. > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:181: const history::MostVisitedURLList& data); > Should only 4 chars indent from where void starts. > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:185: void JumpList::RunUpdate(); > No need for JumpList:: > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:188: // loaded icons > missing a '.' at the end... > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:213: // is called), they are and must remain, > protected by the list_lock_. > You could simply add "Protected by list_lock_" on a separate comment line, but > it should be added to all the data members that are protected. > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > http://codereview.chromium.org/7538022/diff/28004/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:357: if (JumpList::Enabled()) { > Here, you should rely on jumplist_ not being NULL instead... > > And maybe add a comment stating that this must be called before we destroy the > profile used by the jumplist...
A few more style nits to fix, but no real code change needed, so I will use this version for an initial set of try bots... Thanks! BYE MAD http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:521: // show the new tab page. I don't think we are about to show the new tab page :-) http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:530: Source<Profile>(profile_)); Bad alignment, should be aligned to this, right after parenthesis... http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.cc:784: local_recently_closed_pages); Wrong alignment, should be aligned to app_id, right after parenthesis... http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (left): http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:170: // The Profile object used for listening its events. Bring back this comment. http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win.h File chrome/browser/jumplist_win.h (right): http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:102: // update it in a UI thread. To solve this problem, this class posts a posts TO a runnable... http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:153: bool AddTab(const TabRestoreService::Tab* tab, ShellLinkItemList* list, Maybe we could bring those back to 3 lines to minimize the diff... http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:183: const history::MostVisitedURLList& data); Bad alignment, should only be 4 more indent space from the beginning of 'void' http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... chrome/browser/jumplist_win.h:185: // Runnable method that updates the jumplist, once have all the data once all the data... (remove the word have)
done. On 2011/08/19 14:46:03, MAD wrote: > A few more style nits to fix, but no real code change needed, so I will use this > version for an initial set of try bots... > > Thanks! > > BYE > MAD > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win.cc > File chrome/browser/jumplist_win.cc (right): > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:521: // show the new tab page. > I don't think we are about to show the new tab page :-) > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:530: Source<Profile>(profile_)); > Bad alignment, should be aligned to this, right after parenthesis... > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.cc:784: local_recently_closed_pages); > Wrong alignment, should be aligned to app_id, right after parenthesis... > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win.h > File chrome/browser/jumplist_win.h (left): > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:170: // The Profile object used for listening its > events. > Bring back this comment. > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win.h > File chrome/browser/jumplist_win.h (right): > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:102: // update it in a UI thread. To solve this > problem, this class posts a > posts TO a runnable... > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:153: bool AddTab(const TabRestoreService::Tab* > tab, ShellLinkItemList* list, > Maybe we could bring those back to 3 lines to minimize the diff... > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:183: const history::MostVisitedURLList& data); > Bad alignment, should only be 4 more indent space from the beginning of 'void' > > http://codereview.chromium.org/7538022/diff/44002/chrome/browser/jumplist_win... > chrome/browser/jumplist_win.h:185: // Runnable method that updates the jumplist, > once have all the data > once all the data... (remove the word have)
LGTM... I hit the commit box even though not try bots have run for this issue, I ran them on the side for Cait since she's not a committer yet... BYE MAD
Change committed as 97672 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
