implement remaining tab events (except for onTabUpdated).

implement remaining tab events (except for onTabUpdated).



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

[rafaelw, 2009-04-21 20:17:29]

more tab events

[Matt Perry, 2009-04-21 21:06:53]

http://codereview.chromium.org/88053/diff/1/4
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/88053/diff/1/4#newcode171
Line 171: GetRequestContext())->DispatchEventToRenderers(kOnTabMoved,
json_args);
Might be nice to add a private helper function that dispatches an event, since
you call this so much.

http://codereview.chromium.org/88053/diff/1/3
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/88053/diff/1/3#newcode52
Line 52: std::set<int> tab_ids_;
need a comment

http://codereview.chromium.org/88053/diff/1/6
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/88053/diff/1/6#newcode215
Line 215: // will *NOT* be followed or preceded by tab-detached.
very helpful comments!

[Aaron Boodman, 2009-04-21 22:50:45]

LGTM

http://codereview.chromium.org/88053/diff/1/4
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/88053/diff/1/4#newcode68
Line 68: // Tab is new, send tab-created event.
Would recommend moving this comment up above int "tab_id = ..." and changing it
so that it describes this whole block.

http://codereview.chromium.org/88053/diff/1/4#newcode116
Line 116: DictionaryValue *object_args = new DictionaryValue();
For this one, I thought we agreed to just have the signature of the callback be:
callback(int tabId) (not an object).

http://codereview.chromium.org/88053/diff/1/6
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/88053/diff/1/6#newcode215
Line 215: // will *NOT* be followed or preceded by tab-detached.
On 2009/04/21 21:06:53, Matt Perry wrote:
> very helpful comments!

Yes, also would be great to have unit tests that verify these details when we
have unit tests.

http://codereview.chromium.org/88053/diff/1/2
File chrome/test/data/extensions/test/TabsAPI/1/tabs_api.html (right):

http://codereview.chromium.org/88053/diff/1/2#newcode88
Line 88: appendToLog('onTabCreated -- window: ' + data.windowId + ' tab: ' +
data.tabId + ' index ' + data.index);
80 cols, multiple occurrences in this file.

[rafaelw, 2009-04-22 04:03:34]

http://codereview.chromium.org/88053/diff/1/4
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/88053/diff/1/4#newcode68
Line 68: // Tab is new, send tab-created event.
On 2009/04/21 22:50:45, Aaron Boodman wrote:
> Would recommend moving this comment up above int "tab_id = ..." and changing
it
> so that it describes this whole block.

Done.

http://codereview.chromium.org/88053/diff/1/4#newcode116
Line 116: DictionaryValue *object_args = new DictionaryValue();
On 2009/04/21 22:50:45, Aaron Boodman wrote:
> For this one, I thought we agreed to just have the signature of the callback
be:
> callback(int tabId) (not an object).

Done.

http://codereview.chromium.org/88053/diff/1/4#newcode171
Line 171: GetRequestContext())->DispatchEventToRenderers(kOnTabMoved,
json_args);
On 2009/04/21 21:06:53, Matt Perry wrote:
> Might be nice to add a private helper function that dispatches an event, since
> you call this so much.

Done.

http://codereview.chromium.org/88053/diff/1/3
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/88053/diff/1/3#newcode52
Line 52: std::set<int> tab_ids_;
On 2009/04/21 21:06:53, Matt Perry wrote:
> need a comment

Done.

http://codereview.chromium.org/88053/diff/1/2
File chrome/test/data/extensions/test/TabsAPI/1/tabs_api.html (right):

http://codereview.chromium.org/88053/diff/1/2#newcode88
Line 88: appendToLog('onTabCreated -- window: ' + data.windowId + ' tab: ' +
data.tabId + ' index ' + data.index);
On 2009/04/21 22:50:45, Aaron Boodman wrote:
> 80 cols, multiple occurrences in this file.

Done.

[Matt Perry, 2009-04-22 18:20:17]

lgtm

http://codereview.chromium.org/88053/diff/2001/2003
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/88053/diff/2001/2003#newcode54
Line 54: // and tab insertion. Also used to not send tab-detached after
tab-removed.
nit: "Also used to avoid sending"