GTK: Add Recently Closed tabs to the History menu in the global menu bar.

GTK: Add Recently Closed tabs to the History menu in the global menu bar.

This is mostly a straight port of the OSX implementation in
history_menu_bridge.mm. This patch only handles the "Recently Closed" portion
of the history menu and leaves accessing the HistoryService for the "Most
Visited" sites to a future patch.

Attempts to port the corresponding unit tests (history_menu_bridge_unittest.mm)
have ended in frustration. There's some very weird timing issues that make
anything dealing with menus hard to not be flaky. *cry*

BUG=30213
TEST=Open some windows, close some windows. There's Recently Closed entries in the menu bar on Natty Narwhal.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83035

[Elliot Glaysher, 2011-04-14 22:51:39]

estade: review
evanm: FYI

[Evan Stade, 2011-04-15 19:00:41]

http://codereview.chromium.org/6840068/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:9262: <!-- NOTE: Some of these exist in
context menus with Title Case support, but we use different IDs in case we need
slightly different strings in some language due to the different context. -->
according to what Tony has told me before, this doesn't actually work unless you
add a sense="foo" field to the message. Otherwise, all messages with the same
text content get collapsed in the translation console.

However, I'm not sure I understand the different contexts you are talking about.
Both are menu items. I have a hard time believing that a language would have a
different translation for "Bookmark All Tabs" in the titlebar context menu
versus the global->history menu. You can ping jshin to be sure.

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:248: 
extra line

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:405: if (it !=
menu_item_history_map_.end()) {
nit: return it != menu_item_history_map_.end() ? it->second : NULL;

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:574: if (submenu) {
nit: no curlies

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:625: it != entries.end() && added_count
< kRecentlyClosedCount; ++it) {
instead of added_count can we track the size of history_menu_

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:629: TabRestoreService::Window*
entry_win = (TabRestoreService::Window*)entry;
C++ style cast

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:631: if (!tabs.size())
empty()

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:680: if (subadd_count) {
use subindex > 2 instead?

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:704:
menu_item_history_map_.erase(restore_item);
can we avoid adding it to the map in the first place? actually, it would be nice
if we made sure we were going to actually use this submenu before we create it
or any of its items

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:706: g_object_unref(submenu);
I think it would be a good idea to gtk_widget_destroy before unreffing

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.h (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.h:125: GdkPixubf* default_favicon_;
pixubf?

[Elliot Glaysher, 2011-04-15 20:29:02]

Most of your comments addressed. Others responded to. Rewrote it so we see if we
have a valid menu before building.

http://codereview.chromium.org/6840068/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:9262: <!-- NOTE: Some of these exist in
context menus with Title Case support, but we use different IDs in case we need
slightly different strings in some language due to the different context. -->
On 2011/04/15 19:00:41, Evan Stade wrote:
> according to what Tony has told me before, this doesn't actually work unless
you
> add a sense="foo" field to the message. Otherwise, all messages with the same
> text content get collapsed in the translation console.
> 
> However, I'm not sure I understand the different contexts you are talking
about.
> Both are menu items. I have a hard time believing that a language would have a
> different translation for "Bookmark All Tabs" in the titlebar context menu
> versus the global->history menu. You can ping jshin to be sure.

So this comment was copied verbatim from the mac section above (though we can't
reuse the mac menu titles because we need them &amp;ed for accelerators because
while the default is to have mnemonics off, there's a toggle switch for that).
All of these additional menu items just mirror the above mac section. I've tried
to reuse strings (notice how fewer ones that I've duplicated compared to the mac
section above), but when the IDS specifically said IDS_CONTEXT_MENU, I shied
away from reuse. I'm not sure if this addresses your comment.

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:625: it != entries.end() && added_count
< kRecentlyClosedCount; ++it) {
On 2011/04/15 19:00:41, Evan Stade wrote:
> instead of added_count can we track the size of history_menu_

We can in this patch, but we won't be able to once I've added the Most Visited
section.

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.h (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.h:125: GdkPixubf* default_favicon_;
On 2011/04/15 19:00:41, Evan Stade wrote:
> pixubf?

Yeap. I can't store the image because the image is a widget and can only have
one parent, but each GtkImage can share the same GdkPixbuf backing.

[Evan Stade, 2011-04-15 20:40:32]

> So this comment was copied verbatim from the mac section above (though we
can't
> reuse the mac menu titles because we need them &ed for accelerators
because
> while the default is to have mnemonics off, there's a toggle switch for that).
> All of these additional menu items just mirror the above mac section. I've
tried
> to reuse strings (notice how fewer ones that I've duplicated compared to the
mac
> section above), but when the IDS specifically said IDS_CONTEXT_MENU, I shied
> away from reuse. I'm not sure if this addresses your comment.

my point is that your new strings are not new strings if the contents are
exactly the same, even though the IDS_ identifiers are different. If you don't
like using IDS_CONTEXT_MENU_* then you can add a new entry (as you have done),
and so long as the actual string is the same it will use the already-existing
translations. I think we want this. So basically, I think it's best to just
delete the comment, which is wrong-headed in intent and wrong in fact (both Mac
and here).

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.h (right):

http://codereview.chromium.org/6840068/diff/2001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.h:125: GdkPixubf* default_favicon_;
comment was about spelling

[Elliot Glaysher, 2011-04-15 21:21:15]

Done, also haha wtf on "GdkPixubf"

[Elliot Glaysher, 2011-04-18 17:25:09]

On 2011/04/15 21:21:15, Elliot Glaysher wrote:
> Done, also haha wtf on "GdkPixubf"

Friendly ping

[Evan Stade, 2011-04-18 17:50:24]

LGTM with TabRestoreServiceDestroyed() change

[Elliot Glaysher, 2011-04-18 17:55:44]

On 2011/04/18 17:50:24, Evan Stade wrote:
> LGTM with TabRestoreServiceDestroyed() change

I don't see any comment. You may need to 'm'.

[Evan Stade, 2011-04-18 18:11:18]

dang it

http://codereview.chromium.org/6840068/diff/9002/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/6840068/diff/9002/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:728: // Intentionally left blank. We
hold a weak reference to the service.
I think observers usually do hold weak refs. The idea is to null out that ref.

[Elliot Glaysher, 2011-04-19 17:57:09]

On 2011/04/18 18:11:18, Evan Stade wrote:
> dang it
> 
>
http://codereview.chromium.org/6840068/diff/9002/chrome/browser/ui/gtk/global...
> File chrome/browser/ui/gtk/global_menu_bar.cc (right):
> 
>
http://codereview.chromium.org/6840068/diff/9002/chrome/browser/ui/gtk/global...
> chrome/browser/ui/gtk/global_menu_bar.cc:728: // Intentionally left blank. We
> hold a weak reference to the service.
> I think observers usually do hold weak refs. The idea is to null out that ref.

Since I can't commit this until tomorrow because I don't want it on the M12
branch, I went ahead and moved all the new code to its own GlobalHistoryMenu
object. That's all I changed. (This probably doesn't need more review, but just
in case...)

[Evan Stade, 2011-04-19 18:17:10]

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_history_menu.cc (right):

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_history_menu.cc:257: 
extra line

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:106: GlobalMenuBarCommand
history_menu[] = {
move into global_history_menu.cc

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:238: GtkWidget*
GlobalMenuBar::BuildGtkMenuFrom(
you know, I'm sorry for not thinking about this before, but why aren't you using
MenuGtk? seems like BuildGtkMenuFrom and BuildMenuItem wind up re-writing a lot
of code from MenuGtk, and that will increase as you have to support things like
items updating (e.g. favicons).

[Elliot Glaysher, 2011-04-19 19:52:07]

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:106: GlobalMenuBarCommand
history_menu[] = {
On 2011/04/19 18:17:10, Evan Stade wrote:
> move into global_history_menu.cc

These definitions are used in this file. GlobalHistoryMenu takes ownership of
the built menu and then modifies it based on state changes.

http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:238: GtkWidget*
GlobalMenuBar::BuildGtkMenuFrom(
On 2011/04/19 18:17:10, Evan Stade wrote:
> you know, I'm sorry for not thinking about this before, but why aren't you
using
> MenuGtk?

I was unsure exactly how robust Ubuntu's global menu code would be to weird
things. The MenuGtk code updates the menu after the "show" signal has been
emitted, in a posted task. I wanted to minimize any risk here by matching how a
"normal" program would keep the menu updated. Poking at the global menu code,
every gtk_menu_* call gets turned into a dbus message. :(

[Evan Stade, 2011-04-19 20:13:44]

On 2011/04/19 19:52:07, Elliot Glaysher wrote:
>
http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
> File chrome/browser/ui/gtk/global_menu_bar.cc (right):
> 
>
http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
> chrome/browser/ui/gtk/global_menu_bar.cc:106: GlobalMenuBarCommand
> history_menu[] = {
> On 2011/04/19 18:17:10, Evan Stade wrote:
> > move into global_history_menu.cc
> 
> These definitions are used in this file. GlobalHistoryMenu takes ownership of
> the built menu and then modifies it based on state changes.

only because of BuildGtkMenu, right? But if you used MenuGtk, you wouldn't have
to have this construction code here forcing you to break encapsulation.

> 
>
http://codereview.chromium.org/6840068/diff/18001/chrome/browser/ui/gtk/globa...
> chrome/browser/ui/gtk/global_menu_bar.cc:238: GtkWidget*
> GlobalMenuBar::BuildGtkMenuFrom(
> On 2011/04/19 18:17:10, Evan Stade wrote:
> > you know, I'm sorry for not thinking about this before, but why aren't you
> using
> > MenuGtk?
> 
> I was unsure exactly how robust Ubuntu's global menu code would be to weird
> things. The MenuGtk code updates the menu after the "show" signal has been
> emitted, in a posted task. I wanted to minimize any risk here by matching how
a
> "normal" program would keep the menu updated. Poking at the global menu code,
> every gtk_menu_* call gets turned into a dbus message. :(

If that's the case, wouldn't it be better to only make gtk_menu_* calls when the
menu is actually shown in order to avoid surplus dbus messages?

If that it actually doesn't work, wouldn't you wish to minimize gtk_menu_* calls
by clobbering the entire history (sub)menu in TabRestoreServiceChanged, and do
just one gtk_menu_set_submenu?

Also, you might be interested in http://codereview.chromium.org/6732007/, which
adds an alternate mechanism for keeping menus up to date (since a menu can
change while it's already showing, OnShow is sometimes not enough). That patch
has kind of died, you could either ping him or take up the reins if it looks
useful.

[Elliot Glaysher, 2011-04-19 20:26:06]

On 2011/04/19 20:13:44, Evan Stade wrote:
> If that's the case, wouldn't it be better to only make gtk_menu_* calls when
the
> menu is actually shown in order to avoid surplus dbus messages?

But that set of dbus calls happens while the menu is hidden. I would assume that
during all tab session events and events that would change the Most Visited list
would happen when the menu isn't displayed. Compare this where we show the menu
and post a task to update things. I am worried about the interval between show
and update.

> Also, you might be interested in http://codereview.chromium.org/6732007/,
which
> adds an alternate mechanism for keeping menus up to date (since a menu can
> change while it's already showing, OnShow is sometimes not enough). That patch
> has kind of died, you could either ping him or take up the reins if it looks
> useful.

That's interesting for the icon change, but that doesn't update the rest of the
model. Out of curiosity, do you know what we post a task during our "show"
handler in the current MenuGtk implementation?

[Evan Stade, 2011-04-19 20:54:14]

On 2011/04/19 20:26:06, Elliot Glaysher wrote:
> On 2011/04/19 20:13:44, Evan Stade wrote:
> > If that's the case, wouldn't it be better to only make gtk_menu_* calls when
> the
> > menu is actually shown in order to avoid surplus dbus messages?
> 
> But that set of dbus calls happens while the menu is hidden. I would assume
that
> during all tab session events and events that would change the Most Visited
list
> would happen when the menu isn't displayed. Compare this where we show the
menu
> and post a task to update things. I am worried about the interval between show
> and update.

ah, I see your concern. Well you can still use MenuGtk, but not use
IsItemDynamicAt to update it.

> 
> > Also, you might be interested in http://codereview.chromium.org/6732007/,
> which
> > adds an alternate mechanism for keeping menus up to date (since a menu can
> > change while it's already showing, OnShow is sometimes not enough). That
patch
> > has kind of died, you could either ping him or take up the reins if it looks
> > useful.
> 
> That's interesting for the icon change, but that doesn't update the rest of
the
> model.

true. If you have a case where you want to update a single item's text or check
state, you could extend on that patch. But when bigger changes happen, you could
just rebuild the menu from scratch (which you basically do in
TabRestoreServiceChanged anyway).

In general, if there is functionality you need that is missing from MenuGtk (and
I don't doubt there is), I'd rather you improve MenuGtk than duplicate
construction code.

> Out of curiosity, do you know what we post a task during our "show"
> handler in the current MenuGtk implementation?

third bullet point in http://codereview.chromium.org/159815 (even I only figured
this out from git blame/log, so I guess I should have added a comment)

[Elliot Glaysher, 2011-04-20 20:13:52]

I've overhauled menu construction to use ui::SimpleMenuModels. I factored out
common menu construction code so now both MenuGtk and GlobalMenuBar are both
MenuCreators.

MenuGtk just has way too much stuff I'd need to circumvent, while this let's us
share the menu construction code.

[Evan Stade, 2011-04-20 22:52:10]

looking better

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.h (right):

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.h:35: public MenuCreator,
do you think it would work to make a separate class called GlobaMenuGtk : public
MenuCreator to encapsulate AppendMenuItemToMenu, BuildGtkMenuFrom, and
BuildMenuItem logic? The new class would be a cleaner parallel to MenuGtk.

It would fix the broken encapsulation of GlobalHistoryMenu, and also the fact
that this class is both a MenuCreator and MenuCreator::Delegate (which seems
very fishy).

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:465: bool connect_to_activate) {
this doesn't need to be passed here; check model->GetTypeAt(i) ==
ui::MenuModel::TYPE_BUTTON_ITEM

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:484:
gtk_menu_shell_append(GTK_MENU_SHELL(menu), menu_item);
is it necessary to do this here? why not do this in menucreator and then change
the function to "MenuItemCreated"

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
File chrome/browser/ui/gtk/menu_gtk.h (right):

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.h:27: class MenuCreator {
can we call it GtkMenuCreator to make it more explicit it's creating the
GtkMenu? Also, it merits its own file(s).

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.h:29: class Delegate {
this should not be called Delegate. I think GtkMenuCreator::ModelExtras perhaps
makes more sense? I'm open to hearing your opinion here.

Previously the functions herein were part of MenuGtk::Delegate, but that's
probably just because it was the easiest place to tack them on.

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.h:40: static GtkWidget*
GetDefaultImageForCommandId(int command_id);
I think this belongs on MenuCreator.

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.h:87: class Delegate : public
MenuCreator::Delegate {
I don't think MenuGtk::Delegate needs to be a subclass of MenuCreator::Delegate
(or GtkMenuCreator::ModelExtras if you take my above suggestions). Any client
that actually wants to implement those things should implement both interfaces.

And yes, this means we need to add another constructor that takes a delegate,
model, and modelextras.

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.h:104: virtual GtkIconSet* GetIconSetForId(int
idr) { return NULL; }
also belongs in ModelExtras

[Elliot Glaysher, 2011-04-25 18:24:47]

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.h (right):

http://codereview.chromium.org/6840068/diff/18003/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.h:35: public MenuCreator,
On 2011/04/20 22:52:10, Evan Stade wrote:
> do you think it would work to make a separate class called GlobaMenuGtk :
public
> MenuCreator to encapsulate AppendMenuItemToMenu, BuildGtkMenuFrom, and
> BuildMenuItem logic? The new class would be a cleaner parallel to MenuGtk.
> 
> It would fix the broken encapsulation of GlobalHistoryMenu, and also the fact
> that this class is both a MenuCreator and MenuCreator::Delegate (which seems
> very fishy).

At this point, we're close to looping back to how I did things in the first
place. I've addressed most of your comments just to show what this would look
like, but I disagree that this approach is better.

I've added a lot of complexity going with this OO approach and what was a very
simple, table driven piece of code is now really complex. :(

[Evan Stade, 2011-04-25 20:08:22]

I don't see this code as more complex. I think it's more extensible and it makes
the relationships between the different moving pieces more obvious. However,
this is your code so I will defer to you on whether you think this will make it
easier moving forward. If you hate it, then thanks for humoring me and I'll LG
the previous approach.

http://codereview.chromium.org/6840068/diff/19001/chrome/browser/ui/gtk/gtk_m...
File chrome/browser/ui/gtk/gtk_menu_creator.h (right):

http://codereview.chromium.org/6840068/diff/19001/chrome/browser/ui/gtk/gtk_m...
chrome/browser/ui/gtk/gtk_menu_creator.h:21: // We have multiple places in the
code that actually build menus and have
can you make this comment more specific? e.g. give at least a high level
description of the different requirements

http://codereview.chromium.org/6840068/diff/19001/chrome/browser/ui/gtk/menu_...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/6840068/diff/19001/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:201: if (model) {
if (!model || model->IsVisibleAt(index))

[Elliot Glaysher, 2011-04-25 22:17:24]

I've re-uploaded a copy of the previous code I intend to commit. (With additions
to make the Most Visited menu part work too).

[Evan Stade, 2011-04-25 23:11:41]

http://codereview.chromium.org/6840068/diff/12032/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_history_menu.cc (right):

http://codereview.chromium.org/6840068/diff/12032/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_history_menu.cc:186:
history_service_->QuerySegmentUsageSince(
why don't you use TopSites?

[Elliot Glaysher, 2011-04-25 23:27:49]

http://codereview.chromium.org/6840068/diff/12032/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_history_menu.cc (right):

http://codereview.chromium.org/6840068/diff/12032/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_history_menu.cc:186:
history_service_->QuerySegmentUsageSince(
On 2011/04/25 23:11:41, Evan Stade wrote:
> why don't you use TopSites?

Because I didn't know that existed. I've pulled that part of this patch out.

[Evan Stade, 2011-04-25 23:33:17]

lgtm