Move topSites API out of experimental

Move topSites API out of experimental

topSites API is now available for developers.

BUG=27759
TEST=*TopSites*

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

[cduvall, 2012-03-19 19:25:15]

Added a function to the chrome.history API. Thanks for the review!

[cduvall, 2012-03-19 20:07:04]

+brettw for history OWNERS

[brettw, 2012-03-19 21:09:27]

owners LGTM, I didn't look at the details.

[Aaron Boodman, 2012-03-20 02:24:24]

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/browser/histo...
File chrome/browser/history/history_extension_api.cc (right):

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/browser/histo...
chrome/browser/history/history_extension_api.cc:434: kDaysOfHistory,
Why not allow developers to specify this too. You could default it to 90 if they
don't specify.

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/browser/histo...
chrome/browser/history/history_extension_api.cc:447: ++iterator) {
This can go on previous line.

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/browser/histo...
File chrome/browser/history/history_extension_apitest.cc (right):

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/browser/histo...
chrome/browser/history/history_extension_apitest.cc:33:
GURL(extension->GetResourceURL("test1.html")));
It looks like these files don't do anything other than call
chrome.test.succeed(). In their only purpose is to create history, they can just
be empty files, and you can remove catcher.GetNextResult().

Also, can you express this as a nested loop instead of the duplicated code?

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/test/data/ext...
File chrome/test/data/extensions/api_test/history/get_most_visited.js (right):

https://chromiumcodereview.appspot.com/9721013/diff/3037/chrome/test/data/ext...
chrome/test/data/extensions/api_test/history/get_most_visited.js:32:
assertEq(items[i].redirects.length, 1);
In unit testing asserts that have two terms, the expected value usually goes
first. This is used for the error messages "expected <foo>, got <bar>".

[brettw, 2012-03-20 16:30:10]

The TopSite API that Aaron pointed out does the same thing that the new tab page
does. This patch doesn't: it queries history, bypassing the cache of top sites
(so it will be much slower), pinning, but allowing you to get more than 8 URLs.
What is the intended usage of these APIs?

[cduvall, 2012-03-20 17:46:07]

On 2012/03/20 16:30:10, brettw wrote:
> The TopSite API that Aaron pointed out does the same thing that the new tab
page
> does. This patch doesn't: it queries history, bypassing the cache of top sites
> (so it will be much slower), pinning, but allowing you to get more than 8
URLs.
> What is the intended usage of these APIs?

Hm you're right, I hadn't seen the TopSite API before I created this patch. I
could make getMostVisited return a list of HistoryItems instead, which may be
more useful than MostVisitedURLs to some developers, and would also make it
different than TopSite. Or we could at least expose the TopSite API to
developers, since right now it's not even in the docs.

[brettw, 2012-03-20 17:49:24]

The TopSites system just manages the data for the new tab page. It doesn't have
the full history info.

[cduvall, 2012-03-20 17:53:27]

On 2012/03/20 17:49:24, brettw wrote:
> The TopSites system just manages the data for the new tab page. It doesn't
have
> the full history info.

Yes, but getMostVisited could be implemented so that it retrieves the history
for X number of days, sorts by visits, and then returns the N most visited
items. It wouldn't have to use anything from TopSites.

[brettw, 2012-03-20 18:01:44]

That depends what the goal of this API is? I don't think we should be adding
random APIs we'll have to support unless there's a strong use case.

[cduvall, 2012-03-20 18:07:22]

Yep, I definitely agree. You think it would be best to scrap this one for now?

[brettw, 2012-03-20 18:21:07]

On 2012/03/20 18:07:22, cduvall wrote:
> Yep, I definitely agree. You think it would be best to scrap this one for now?

Unless we have somebody pushing for it, yeah.

[cduvall, 2012-03-20 18:27:12]

Ok, sounds good. Thanks for bringing that up.

[Aaron Boodman, 2012-03-20 20:18:06]

The original request was actually for the same functionality the NTP has. See
the bug report:

chrome.experimental.history.getMostVisited(8);

Should return an array of the 8 sites which appear in the NTP, in the same 
order (including pinned tabs).

Note: Obviously, when the history API is finalized, it should be:

chrome.history.getMostVisited(8);

There has not been a ton of interest on the bug, but I do think this would be a
useful addition. Replacing the new tab page is a popular use for extensions, and
they frequently want to do what the NTP does.

Brett, would you be comfortable de-experimentalifying the experimental top sites
API?

[brettw, 2012-03-20 20:25:32]

That's fine with me.

[cduvall, 2012-03-21 01:18:13]

TopSites API is no longer experimental.

[brettw, 2012-03-21 17:43:22]

http://codereview.chromium.org/9721013/diff/18001/chrome/browser/history/top_...
File chrome/browser/history/top_sites_extension_api.cc (right):

http://codereview.chromium.org/9721013/diff/18001/chrome/browser/history/top_...
chrome/browser/history/top_sites_extension_api.cc:36: if (!url.url.is_empty())
You either messed up your indenting or you need {} here.

[cduvall, 2012-03-21 18:00:34]

http://codereview.chromium.org/9721013/diff/18001/chrome/browser/history/top_...
File chrome/browser/history/top_sites_extension_api.cc (right):

http://codereview.chromium.org/9721013/diff/18001/chrome/browser/history/top_...
chrome/browser/history/top_sites_extension_api.cc:36: if (!url.url.is_empty())
On 2012/03/21 17:43:22, brettw wrote:
> You either messed up your indenting or you need {} here.

Oops, Done.

[brettw, 2012-03-21 18:16:01]

history LGTM

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
File chrome/browser/history/top_sites_extension_api.cc (left):

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
chrome/browser/history/top_sites_extension_api.cc:38:
page_value->SetBoolean("filler", true);
I don't really know what this filler code was doing that you removed, so I'm
assuming you know what you're doing.

[cduvall, 2012-03-21 18:33:49]

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
File chrome/browser/history/top_sites_extension_api.cc (left):

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
chrome/browser/history/top_sites_extension_api.cc:38:
page_value->SetBoolean("filler", true);
On 2012/03/21 18:16:01, brettw wrote:
> I don't really know what this filler code was doing that you removed, so I'm
> assuming you know what you're doing.

The filler was for urls that were empty, to let the caller know that they were
just filler urls. I just made it return a shorter list if there were some empty
urls, so there's no need to let the caller know if a url is a filler.

[Mihai Parparita -not on Chrome, 2012-03-21 21:20:04]

Aaron may be more qualified to make this call, but does this need to be a
separate API, with a separate permission? It seems like it should just be a
History API method.

[Aaron Boodman, 2012-03-21 21:26:15]

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
File chrome/browser/history/top_sites_extension_api.cc (right):

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
chrome/browser/history/top_sites_extension_api.cc:37:
NewTabUI::SetURLTitleAndDirection(page_value,
I don't think that we should call this for extensions.

http://codereview.chromium.org/9721013/diff/22002/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/9721013/diff/22002/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:154: "extension_types":
["extension", "packaged_app", "platform_app"]
Don't expose to platform apps (platform apps are a new experimental feature -- I
think in general, they don't want new extension APIs and want to decide on a
case-by-case basis).

[cduvall, 2012-03-21 22:00:14]

Mihai: I talked with Aaron about it, and he thought it would be best to move
topSites out of experimental rather than adding it to chrome.history.

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
File chrome/browser/history/top_sites_extension_api.cc (right):

http://codereview.chromium.org/9721013/diff/22002/chrome/browser/history/top_...
chrome/browser/history/top_sites_extension_api.cc:37:
NewTabUI::SetURLTitleAndDirection(page_value,
On 2012/03/21 21:26:15, Aaron Boodman wrote:
> I don't think that we should call this for extensions.

Done.

http://codereview.chromium.org/9721013/diff/22002/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/9721013/diff/22002/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:154: "extension_types":
["extension", "packaged_app", "platform_app"]
On 2012/03/21 21:26:15, Aaron Boodman wrote:
> Don't expose to platform apps (platform apps are a new experimental feature --
I
> think in general, they don't want new extension APIs and want to decide on a
> case-by-case basis).

Done.

[Aaron Boodman, 2012-03-21 22:05:23]

http://codereview.chromium.org/9721013/diff/24006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9721013/diff/24006/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:259: kTopSites, "topSites",
0,
Since this API effectively gives access to the same sort of information as the
history API, it should have the same warning as the history API:

IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY

[cduvall, 2012-03-21 22:18:07]

http://codereview.chromium.org/9721013/diff/24006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9721013/diff/24006/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.cc:259: kTopSites, "topSites",
0,
On 2012/03/21 22:05:23, Aaron Boodman wrote:
> Since this API effectively gives access to the same sort of information as the
> history API, it should have the same warning as the history API:
> 
> IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY

Done.

[Aaron Boodman, 2012-03-22 06:32:23]

LGTM

[commit-bot: I haz the power, 2012-03-22 18:16:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/9721013/31001

[commit-bot: I haz the power, 2012-03-22 19:27:37]

Change committed as 128267