Move history API out of experimental. Allow extensions to override history page.

Move history API out of experimental.  Allow extensions to override history page.

BUG=32362
TEST=ExtensionApiTest.History, ExtensionApiTest.Overrides, manual testing.

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

[Sam Kerner (Chrome), 2010-03-29 15:47:16]

http://codereview.chromium.org/1530002/diff/2001/3005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/1530002/diff/2001/3005#newcode1820
chrome/common/extensions/api/extension_api.json:1820: "name": "details",
Many parameters were unnamed.  Better ideas for names are welcome.

http://codereview.chromium.org/1530002/diff/6001/7052
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/1530002/diff/6001/7052#newcode1379
chrome/common/extensions/extension.cc:1379: page !=
chrome::kChromeUIHistoryHost) ||
From IRC: 

<skerner> erikkay: Should a single extension be able to override both the
history and new tab page?  

<erikkay> skerner: Hmm.  good question.  on the surface, my take was sure.   but
this could be an extension of our one UI surface argument.

<erikkay> skerner.  the more I'm thinking about it, I think your concern is
probably correct.  We should only allow one page to be overridden per extension.
 It's easy to relax that constraint later, but impossible to add it if it were
missing.

[Erik does not do reviews, 2010-03-29 15:47:23]

http://codereview.chromium.org/1530002/diff/2001/3005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/1530002/diff/2001/3005#newcode1785
chrome/common/extensions/api/extension_api.json:1785: "transition": {"type":
"integer", "minimum": 0, "maximum": 10, "description": "The <a
href='#transition_types'>transition type</a> for this visit from its referrer."}
I hadn't noticed this before, but this should probably be changed to an
enumeration.  I think we hadn't been using enumerations when this was first
implemented.

http://codereview.chromium.org/1530002/diff/2001/3005#newcode1869
chrome/common/extensions/api/extension_api.json:1869: "name": "range",
I'm confused about something.  how did these work before without names?

http://codereview.chromium.org/1530002/diff/2001/3042
File chrome/common/extensions/docs/static/override.html (right):

http://codereview.chromium.org/1530002/diff/2001/3042#newcode92
chrome/common/extensions/docs/static/override.html:92: may take a long time on a
slow network connection.
remove "on a slow network connection".  This is repeated on the next line, which
also includes filesystem performance.

http://codereview.chromium.org/1530002/diff/2001/3052
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/1530002/diff/2001/3052#newcode1376
chrome/common/extensions/extension.cc:1376: // For now, only allow the new tab
page.  Others will work when we remove
Update this comment and remove the TODO.

http://codereview.chromium.org/1530002/diff/2001/3062
File chrome/test/data/extensions/api_test/permissions/enabled/manifest.json
(right):

http://codereview.chromium.org/1530002/diff/2001/3062#newcode6
chrome/test/data/extensions/api_test/permissions/enabled/manifest.json:6:
"permissions": [ "tabs", "bookmarks", "history" ]
I think we added the experimental key here specifically to test it, not history
in particular.  This is because experimental is different in that it requires a
command-line flag.  I guess this makes this test a bit of a moving target since
things move out of experimental like this.

http://codereview.chromium.org/1530002/diff/6001/7042
File chrome/common/extensions/docs/static/override.html (right):

http://codereview.chromium.org/1530002/diff/6001/7042#newcode79
chrome/common/extensions/docs/static/override.html:79: You can override one or
both pages.
per our discussion, I think we should change this to only allow one override per
extension.

[Aaron Boodman, 2010-03-29 16:39:12]

http://codereview.chromium.org/1530002/diff/6001/7005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/1530002/diff/6001/7005#newcode1777
chrome/common/extensions/api/extension_api.json:1777: "id": "VisitItem",
What the heck -- this doesn't seem to be referred to by anything in this file.
How can that be?

[Sam Kerner (Chrome), 2010-03-29 18:36:06]

Still addressing other comments.

http://codereview.chromium.org/1530002/diff/2001/3005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/1530002/diff/2001/3005#newcode1869
chrome/common/extensions/api/extension_api.json:1869: "name": "range",
On 2010/03/29 15:47:23, Erik Kay wrote:
> I'm confused about something.  how did these work before without names?

The generated docs showed that the name was "undefined".  I don't know of
anything else that uses the name.

http://codereview.chromium.org/1530002/diff/6001/7005#newcode1777
chrome/common/extensions/api/extension_api.json:1777: "id": "VisitItem",
On 2010/03/29 16:39:12, Aaron Boodman wrote:
> What the heck -- this doesn't seem to be referred to by anything in this file.
> How can that be?

chrome.history.getVisits() has a callback that takes an array of visits.

http://codereview.chromium.org/1530002/diff/6001/7005#newcode1830
chrome/common/extensions/api/extension_api.json:1830: { "name": "results",
"type": "array", "items": { "$ref": "VisitItem"} }
Here is the reference to VisitItem.

[Sam Kerner (Chrome), 2010-03-31 21:12:23]

Ready for another look.

http://codereview.chromium.org/1530002/diff/2001/3005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/1530002/diff/2001/3005#newcode1785
chrome/common/extensions/api/extension_api.json:1785: "transition": {"type":
"integer", "minimum": 0, "maximum": 10, "description": "The <a
href='#transition_types'>transition type</a> for this visit from its referrer."}
On 2010/03/29 15:47:23, Erik Kay wrote:
> I hadn't noticed this before, but this should probably be changed to an
> enumeration.  I think we hadn't been using enumerations when this was first
> implemented.

Done.

http://codereview.chromium.org/1530002/diff/2001/3042
File chrome/common/extensions/docs/static/override.html (right):

http://codereview.chromium.org/1530002/diff/2001/3042#newcode92
chrome/common/extensions/docs/static/override.html:92: may take a long time on a
slow network connection.
On 2010/03/29 15:47:23, Erik Kay wrote:
> remove "on a slow network connection".  This is repeated on the next line,
which
> also includes filesystem performance.

Done.

http://codereview.chromium.org/1530002/diff/2001/3052
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/1530002/diff/2001/3052#newcode1376
chrome/common/extensions/extension.cc:1376: // For now, only allow the new tab
page.  Others will work when we remove
On 2010/03/29 15:47:23, Erik Kay wrote:
> Update this comment and remove the TODO.

Done.

http://codereview.chromium.org/1530002/diff/2001/3062
File chrome/test/data/extensions/api_test/permissions/enabled/manifest.json
(right):

http://codereview.chromium.org/1530002/diff/2001/3062#newcode6
chrome/test/data/extensions/api_test/permissions/enabled/manifest.json:6:
"permissions": [ "tabs", "bookmarks", "history" ]
On 2010/03/29 15:47:23, Erik Kay wrote:
> I think we added the experimental key here specifically to test it, not
history
> in particular.  This is because experimental is different in that it requires
a
> command-line flag.  I guess this makes this test a bit of a moving target
since
> things move out of experimental like this.

I kept the history test, and added a new one that checks experimental by calling
 chrome.experimental.processes.getProcessForTab().  You asked that I record the
things to be done when taking an API out of experimental, and updating this test
is on that list.

http://codereview.chromium.org/1530002/diff/6001/7042
File chrome/common/extensions/docs/static/override.html (right):

http://codereview.chromium.org/1530002/diff/6001/7042#newcode79
chrome/common/extensions/docs/static/override.html:79: You can override one or
both pages.
On 2010/03/29 15:47:24, Erik Kay wrote:
> per our discussion, I think we should change this to only allow one override
per
> extension.

Done.

http://codereview.chromium.org/1530002/diff/6001/7052
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/1530002/diff/6001/7052#newcode1379
chrome/common/extensions/extension.cc:1379: page !=
chrome::kChromeUIHistoryHost) ||
On 2010/03/29 15:47:16, Sam Kerner (Chrome) wrote:
> From IRC: 
> 
> <skerner> erikkay: Should a single extension be able to override both the
> history and new tab page?  
> 
> <erikkay> skerner: Hmm.  good question.  on the surface, my take was sure.  
but
> this could be an extension of our one UI surface argument.
> 
> <erikkay> skerner.  the more I'm thinking about it, I think your concern is
> probably correct.  We should only allow one page to be overridden per
extension.
>  It's easy to relax that constraint later, but impossible to add it if it were
> missing.

Done.

Should a single extension be able to have a browser action and override a page? 
The "one UI surface" principal seems to forbid this.  I added this limitation. 
Let me know if it goes too far.

[Sam Kerner (Chrome), 2010-03-31 21:14:54]

http://codereview.chromium.org/1530002/diff/6001/7052
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/1530002/diff/6001/7052#newcode1379
chrome/common/extensions/extension.cc:1379: page !=
chrome::kChromeUIHistoryHost) ||
On 2010/03/31 21:12:24, Sam Kerner (Chrome) wrote:
> On 2010/03/29 15:47:16, Sam Kerner (Chrome) wrote:
> > From IRC: 
> > 
> > <skerner> erikkay: Should a single extension be able to override both the
> > history and new tab page?  
> > 
> > <erikkay> skerner: Hmm.  good question.  on the surface, my take was sure.  
> but
> > this could be an extension of our one UI surface argument.
> > 
> > <erikkay> skerner.  the more I'm thinking about it, I think your concern is
> > probably correct.  We should only allow one page to be overridden per
> extension.
> >  It's easy to relax that constraint later, but impossible to add it if it
were
> > missing.
> 
> Done.
> 
> Should a single extension be able to have a browser action and override a
page? 
> The "one UI surface" principal seems to forbid this.  I added this limitation.

> Let me know if it goes too far.

I wrote this before you answered the same question on IRC.  Ignore.

[Erik does not do reviews, 2010-04-01 16:53:45]

almost there

http://codereview.chromium.org/1530002/diff/6001/7004
File chrome/browser/extensions/extension_override_apitest.cc (right):

http://codereview.chromium.org/1530002/diff/6001/7004#newcode19
chrome/browser/extensions/extension_override_apitest.cc:19: // TODO(erikkay)
load a second override and verify behavior, then unload
Actually, this TODO is still valid.  Please leave it there.  It's talking about
having a second extension overriding the same page and the precedence and
unloading rules that apply.

http://codereview.chromium.org/1530002/diff/29001/30044
File chrome/common/extensions/docs/static/history.html (right):

http://codereview.chromium.org/1530002/diff/29001/30044#newcode31
chrome/common/extensions/docs/static/history.html:31: <tr id="LINK">
should these ids change too?

http://codereview.chromium.org/1530002/diff/29001/30059
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/1530002/diff/29001/30059#newcode255
chrome/common/extensions/extension_unittest.cc:255: // Multiple overrides are
not allowed.
see the comment at the top about not adding more manifest tests here

[Sam Kerner (Chrome), 2010-04-01 18:43:47]

http://codereview.chromium.org/1530002/diff/6001/7004
File chrome/browser/extensions/extension_override_apitest.cc (right):

http://codereview.chromium.org/1530002/diff/6001/7004#newcode19
chrome/browser/extensions/extension_override_apitest.cc:19: // TODO(erikkay)
load a second override and verify behavior, then unload
On 2010/04/01 16:53:45, Erik Kay wrote:
> Actually, this TODO is still valid.  Please leave it there.  It's talking
about
> having a second extension overriding the same page and the precedence and
> unloading rules that apply.

Restored, with more detail from this comment.

http://codereview.chromium.org/1530002/diff/29001/30044
File chrome/common/extensions/docs/static/history.html (right):

http://codereview.chromium.org/1530002/diff/29001/30044#newcode31
chrome/common/extensions/docs/static/history.html:31: <tr id="LINK">
On 2010/04/01 16:53:46, Erik Kay wrote:
> should these ids change too?

I made the labels the lowercase, and added a prefix to make it clear that they
refer to a type.

http://codereview.chromium.org/1530002/diff/29001/30059
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/1530002/diff/29001/30059#newcode255
chrome/common/extensions/extension_unittest.cc:255: // Multiple overrides are
not allowed.
On 2010/04/01 16:53:46, Erik Kay wrote:
> see the comment at the top about not adding more manifest tests here

Done.

[Erik does not do reviews, 2010-04-01 19:05:59]

LGTM

thanks!