Add tabs.onReplaced event to notify listeners of tabs being swapped due to prerendering or instant.

Add tabs.onReplaced event to notify listeners of tabs being replaced due to prerendering or instant.
Don't send onCreated/onRemoved notificatiosn anymore for those cases.

BUG=85584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175391

[justinlin, 2012-12-18 02:05:59]

Hi Matt, please take a look. This is to fix the bug metioned which is also
blocking us for tab capture (since the capture will prematurely stop due to
this). If this seems reasonable, I'll add tests soon. Thanks.

[Matt Perry, 2012-12-18 18:04:02]

https://codereview.chromium.org/11607012/diff/1/chrome/browser/extensions/bro...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11607012/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:488:
TabInsertedAt(new_contents, index, tab_strip_model->active_index() == index);
Can we remove these two calls? The downside is that extensions may have been
depending on them before, but maybe they were broken anyway.

https://codereview.chromium.org/11607012/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/11607012/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/tabs.json:923: "name": "onSwapped",
nit: I have a slight preference for onReplaced.

[justinlin, 2012-12-19 23:42:42]

Added tests and addressed comments. PTAL.

2 Questions:
- I realized there is already an API that does the (almost?) same thing:
http://developer.chrome.com/extensions/webNavigation.html#event-onTabReplaced
Do you think it's still worth adding this event? I think we should probably
remove the onCreated/onRemoved events due to replacements anyway though.

- Should this API be like onRemoved and accessible without the tabs permission?
It's kind of weird that onCreated is not like onRemoved and requires the
permission, so I'm not which one onReplaced is more similar to.

https://codereview.chromium.org/11607012/diff/1/chrome/browser/extensions/bro...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11607012/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:488:
TabInsertedAt(new_contents, index, tab_strip_model->active_index() == index);
On 2012/12/18 18:04:02, Matt Perry wrote:
> Can we remove these two calls? The downside is that extensions may have been
> depending on them before, but maybe they were broken anyway.

Yea, I thought about removing at first, but wanted to avoid breaking
functionality. The docs do say that removed events are fired when a tab is
closed, so you're probably right that it was broken and would fire when the tab
wasn't actually closed but replaced instead. Removed.

https://codereview.chromium.org/11607012/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/11607012/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/tabs.json:923: "name": "onSwapped",
On 2012/12/18 18:04:02, Matt Perry wrote:
> nit: I have a slight preference for onReplaced.

Done. Yea, sounds better.

[justinlin, 2012-12-21 03:25:19]

mpmcomplete@: ping?

On 2012/12/19 23:42:42, justinlin wrote:
> Added tests and addressed comments. PTAL.
> 
> 2 Questions:
> - I realized there is already an API that does the (almost?) same thing:
> http://developer.chrome.com/extensions/webNavigation.html#event-onTabReplaced
> Do you think it's still worth adding this event? I think we should probably
> remove the onCreated/onRemoved events due to replacements anyway though.
> 
> - Should this API be like onRemoved and accessible without the tabs
permission?
> It's kind of weird that onCreated is not like onRemoved and requires the
> permission, so I'm not which one onReplaced is more similar to.
> 
>
https://codereview.chromium.org/11607012/diff/1/chrome/browser/extensions/bro...
> File chrome/browser/extensions/browser_event_router.cc (right):
> 
>
https://codereview.chromium.org/11607012/diff/1/chrome/browser/extensions/bro...
> chrome/browser/extensions/browser_event_router.cc:488:
> TabInsertedAt(new_contents, index, tab_strip_model->active_index() == index);
> On 2012/12/18 18:04:02, Matt Perry wrote:
> > Can we remove these two calls? The downside is that extensions may have been
> > depending on them before, but maybe they were broken anyway.
> 
> Yea, I thought about removing at first, but wanted to avoid breaking
> functionality. The docs do say that removed events are fired when a tab is
> closed, so you're probably right that it was broken and would fire when the
tab
> wasn't actually closed but replaced instead. Removed.
> 
>
https://codereview.chromium.org/11607012/diff/1/chrome/common/extensions/api/...
> File chrome/common/extensions/api/tabs.json (right):
> 
>
https://codereview.chromium.org/11607012/diff/1/chrome/common/extensions/api/...
> chrome/common/extensions/api/tabs.json:923: "name": "onSwapped",
> On 2012/12/18 18:04:02, Matt Perry wrote:
> > nit: I have a slight preference for onReplaced.
> 
> Done. Yea, sounds better.

[Matt Perry, 2013-01-04 00:15:32]

On 2012/12/19 23:42:42, justinlin wrote:
> Added tests and addressed comments. PTAL.
> 
> 2 Questions:
> - I realized there is already an API that does the (almost?) same thing:
> http://developer.chrome.com/extensions/webNavigation.html#event-onTabReplaced
> Do you think it's still worth adding this event? I think we should probably
> remove the onCreated/onRemoved events due to replacements anyway though.

Hmm... I don't feel real strongly about it. webNavigation is largely better than
tabs, but we haven't deprecated tabs, so we should probably make it work. So I
lean towards adding tabs.onReplaced still.

> - Should this API be like onRemoved and accessible without the tabs
permission?
> It's kind of weird that onCreated is not like onRemoved and requires the
> permission, so I'm not which one onReplaced is more similar to.

Good point. onRemoved could safely be sent as long as any sensitive info (like
URL) is only sent if they have the tabs permission.

[Matt Perry, 2013-01-04 00:21:07]

https://codereview.chromium.org/11607012/diff/5002/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11607012/diff/5002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:2658: test_server()->Stop();
This is a little janky. Can you make it an option to auto-start the test server,
and turn it off for this test? Starting the test server is moderately expensive.

[justinlin, 2013-01-04 12:43:50]

https://codereview.chromium.org/11607012/diff/5002/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11607012/diff/5002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:2658: test_server()->Stop();
On 2013/01/04 00:21:07, Matt Perry wrote:
> This is a little janky. Can you make it an option to auto-start the test
server,
> and turn it off for this test? Starting the test server is moderately
expensive.

Done.

[Matt Perry, 2013-01-04 17:57:29]

lgtm

[commit-bot: I haz the power, 2013-01-04 20:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11607012/23001

[commit-bot: I haz the power, 2013-01-04 20:23:27]

Presubmit check for 11607012-23001 failed and returned exit status 1.


*----------------------------------*
| integration_test.py has failures |
*----------------------------------*
testAllPublicTemplates (__main__.IntegrationTest)
Traceback (most recent call last):
  File "docs/server2/integration_test.py", line 103, in testAllPublicTemplates
    self._RunPublicTemplatesTest()
  File "docs/server2/integration_test.py", line 96, in _RunPublicTemplatesTest
    self.fail('\n'.join(failures))
AssertionError: Error rendering extensions/tabs.html: $ref extension.onConnect
could not be resolved.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/prerender

IntegrationTest failed!

Presubmit checks took 2.5s to calculate.

[justinlin, 2013-01-04 20:43:53]

gavinp@: Hi, ptal chrome/browser/prerender/prerender_browsertest.cc. Need owners
approval for that.

cduvall@: Do you know what's the problem that the presubmit is complaining about
in the message above? Is that a bug? This change only adds an API to the
tabs.json file.

Thanks

[cduvall, 2013-01-04 22:21:01]

On 2013/01/04 20:43:53, justinlin wrote:
> gavinp@: Hi, ptal chrome/browser/prerender/prerender_browsertest.cc. Need
owners
> approval for that.
> 
> cduvall@: Do you know what's the problem that the presubmit is complaining
about
> in the message above? Is that a bug? This change only adds an API to the
> tabs.json file.
> 
> Thanks

The problem is in the tabs.connect function it has a $ref to extension.onConnect
in the description. extension.onConnect was (recently?) marked as a nodoc. If
you just change that in the description to not be a $ref, then it should work.
Sorry about that!

[Matt Perry, 2013-01-04 22:23:00]

On 2013/01/04 22:21:01, cduvall wrote:
> On 2013/01/04 20:43:53, justinlin wrote:
> > gavinp@: Hi, ptal chrome/browser/prerender/prerender_browsertest.cc. Need
> owners
> > approval for that.
> > 
> > cduvall@: Do you know what's the problem that the presubmit is complaining
> about
> > in the message above? Is that a bug? This change only adds an API to the
> > tabs.json file.
> > 
> > Thanks
> 
> The problem is in the tabs.connect function it has a $ref to
extension.onConnect
> in the description. extension.onConnect was (recently?) marked as a nodoc. If
> you just change that in the description to not be a $ref, then it should work.
> Sorry about that!

extension.onConnect was deprecated and renamed to runtime.onConnect. $ref to
that and you should be good.

[justinlin, 2013-01-04 23:30:29]

On 2013/01/04 22:23:00, Matt Perry wrote:
> On 2013/01/04 22:21:01, cduvall wrote:
> > On 2013/01/04 20:43:53, justinlin wrote:
> > > gavinp@: Hi, ptal chrome/browser/prerender/prerender_browsertest.cc. Need
> > owners
> > > approval for that.
> > > 
> > > cduvall@: Do you know what's the problem that the presubmit is complaining
> > about
> > > in the message above? Is that a bug? This change only adds an API to the
> > > tabs.json file.
> > > 
> > > Thanks
> > 
> > The problem is in the tabs.connect function it has a $ref to
> extension.onConnect
> > in the description. extension.onConnect was (recently?) marked as a nodoc.
If
> > you just change that in the description to not be a $ref, then it should
work.
> > Sorry about that!
> 
> extension.onConnect was deprecated and renamed to runtime.onConnect. $ref to
> that and you should be good.

Thanks. Had to move:
- extension.onConnect -> runtime.onConnect
- extension.Port -> runtime.Port
- extension.onMessage -> runtime.onMessage

[gavinp, 2013-01-07 20:36:26]

LGTM.

https://codereview.chromium.org/11607012/diff/35001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/11607012/diff/35001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:2675: test_server()->Stop();
Nice change. Thank you! I like making tests faster.

[commit-bot: I haz the power, 2013-01-07 20:40:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11607012/35001

[commit-bot: I haz the power, 2013-01-07 21:57:51]

Change committed as 175391