Flesh out more extensions tab api (added removeTab, updateTab)

Flesh out more extensions tab api (added getTab, removeTab, updateTab)


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

[brettw, 2009-04-10 00:00:25]

drive by

http://codereview.chromium.org/62178/diff/2004/3013
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/62178/diff/2004/3013#newcode17
Line 17: static bool GetIndexOfTabId(const TabStripModel* tab_strip, const int
tab_id,
We don't usually say "const" for things passed by value.

http://codereview.chromium.org/62178/diff/2004/3013#newcode18
Line 18: int& tab_index);
Don't use references for out values. We use pointers instead.

[Aaron Boodman, 2009-04-10 01:37:24]

http://codereview.chromium.org/62178/diff/2002/3009
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/62178/diff/2002/3009#newcode83
Line 83: return true;
Observation: CreateTab() seems to work pretty well with this framework.

http://codereview.chromium.org/62178/diff/2002/3009#newcode101
Line 101: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
General recommendation: Prefer to invert logic like this and return early.

http://codereview.chromium.org/62178/diff/2002/3009#newcode102
Line 102: TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index);
What should we do about the case where the tab index is out of bounds?

http://codereview.chromium.org/62178/diff/2004/3013#newcode95
Line 95: if (!args_->GetAsInteger(&tab_id)) {
You don't really need to re-check this as you just tested it above. Also, braces
aren't necessary for one-liners like this.

http://codereview.chromium.org/62178/diff/2004/3013#newcode101
Line 101: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
Prefer to do the opposite check and return early in cases, like this, to reduce
indentation.

http://codereview.chromium.org/62178/diff/2004/3013#newcode101
Line 101: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
Add a TODO to return an error here?

http://codereview.chromium.org/62178/diff/2004/3013#newcode123
Line 123: if (!(args_hash->GetInteger(L"id", &tab_id))) {
Style nit: our code usually doesn't use braces for one-liners.

http://codereview.chromium.org/62178/diff/2004/3013#newcode129
Line 129: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
Again, would be nicer to reverse this and return early and add a TODO to return
an error.

http://codereview.chromium.org/62178/diff/2004/3013#newcode142
Line 142: if ((args_hash->GetString(L"url", &url)) &&
Don't think we need to compare the URLs. Just re-navigate it if it was
specified, even if it was the same.

http://codereview.chromium.org/62178/diff/2004/3013#newcode155
Line 155: if ((args_hash->GetBoolean(L"selected", &selected)) &&
style nit: lose the extra parens around each expression in the condition.

http://codereview.chromium.org/62178/diff/2004/3013#newcode167
Line 167: 
Setting this here doesn't seem to have any affect.

http://codereview.chromium.org/62178/diff/2004/3013#newcode178
Line 178: if (!browser)
Per our discussion today, I think there should still be a success (and failure,
in the future) handler. This just means that we need to solve the case where the
window goes away while an API is running. Matt had some ideas on how to do this,
but for now can you change this to a TODO?

http://codereview.chromium.org/62178/diff/2004/3014
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/62178/diff/2004/3014#newcode39
Line 39: chromium.tabs.getTab = function(tabId, callback) {
Whee, look at it go! Fly my extension APIs, fly!

[Aaron Boodman, 2009-04-10 02:40:48]

lgtm

http://codereview.chromium.org/62178/diff/4001/4004
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/62178/diff/4001/4004#newcode173
Line 173: // TODO(rafaelw): Do something reasonable if an extension running in a
tab
Actually, can you change this to:

TODO(rafaelw): This should have a callback, but it can't because if it closes
its own tab...

[rafaelw, 2009-04-10 04:25:36]

http://codereview.chromium.org/62178/diff/2004/3013
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/62178/diff/2004/3013#newcode17
Line 17: static bool GetIndexOfTabId(const TabStripModel* tab_strip, const int
tab_id,
On 2009/04/10 00:00:25, brettw wrote:
> We don't usually say "const" for things passed by value.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode18
Line 18: int& tab_index);
On 2009/04/10 00:00:25, brettw wrote:
> Don't use references for out values. We use pointers instead.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode95
Line 95: if (!args_->GetAsInteger(&tab_id)) {
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> You don't really need to re-check this as you just tested it above. Also,
braces
> aren't necessary for one-liners like this.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode101
Line 101: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Prefer to do the opposite check and return early in cases, like this, to
reduce
> indentation.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode101
Line 101: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Add a TODO to return an error here?

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode123
Line 123: if (!(args_hash->GetInteger(L"id", &tab_id))) {
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Style nit: our code usually doesn't use braces for one-liners.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode129
Line 129: if (GetIndexOfTabId(tab_strip, tab_id, tab_index)) {
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Again, would be nicer to reverse this and return early and add a TODO to
return
> an error.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode142
Line 142: if ((args_hash->GetString(L"url", &url)) &&
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Don't think we need to compare the URLs. Just re-navigate it if it was
> specified, even if it was the same.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode155
Line 155: if ((args_hash->GetBoolean(L"selected", &selected)) &&
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> style nit: lose the extra parens around each expression in the condition.

Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode167
Line 167: tab_index = new_index;
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Setting this here doesn't seem to have any affect.

It did when I was returning the tab object in the callback. =-). Done.

http://codereview.chromium.org/62178/diff/2004/3013#newcode178
Line 178: // This call could remove the tab that it originated from (in the case
of
On 2009/04/10 01:37:25, Aaron Boodman wrote:
> Per our discussion today, I think there should still be a success (and
failure,
> in the future) handler. This just means that we need to solve the case where
the
> window goes away while an API is running. Matt had some ideas on how to do
this,
> but for now can you change this to a TODO?

Done.