Tabs Extension: Implementation of tabs.duplicate api.

Tabs Extension: Implementation of tabs.duplicate api.

BUG=36007
TEST=Manual: Tested with an extension with/without "tabs" permission.
Automated: unittest: ExtensionTabsTest.DuplicateTabNoPermission, ExtensionPermissionsTest.DefaultFunctionAccess, ExtensionTest.ApiPermissions
apitest: ExtensionApiTest.TabDuplicate

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

[SanjoyPal, 2012-06-28 14:50:21]

Implemented chrome.tabs.duplicate(tabId, callback(newTabObject)). Tried to
provide "newTabId" in the callback as explained in the bug, but it seems
"Callbacks can have at most a single parameter".

[Aaron Boodman, 2012-06-29 07:24:15]

Definitely needs a test. Also:

- I believe that you need to add this name to a list somewhere to make it
actually callable without the 'tabs' permission. Did you try that part?

- I didn't remember that our callbacks can only take a single param. Can you
change it so that it returns an object with just an id property in that case?

http://codereview.chromium.org/10697017/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:1122: int new_index = tab_index + 1;
Bummer that we have to assume it's the next tab. Can you look at modifying
Browser so that it returns the new tabcontents directly?

[SanjoyPal, 2012-07-03 09:48:34]

Tabs Extension: Implementation of tabs.duplicate api.

BUG=36007
TEST=Manual: Tested with an extension with/without "tabs" permission.
Automated: ExtensionTabsTest.DuplicateTabNoPermission,
ExtensionPermissionsTest.DefaultFunctionAccess, ExtensionTest.ApiPermissions

[SanjoyPal, 2012-07-03 10:03:21]

On 2012/06/29 07:24:15, Aaron Boodman wrote:
> Definitely needs a test. Also:

Done. Updated patch with automated tests.

> 
> - I believe that you need to add this name to a list somewhere to make it
> actually callable without the 'tabs' permission. Did you try that part?

Done.

> 
> - I didn't remember that our callbacks can only take a single param. Can you
> change it so that it returns an object with just an id property in that case?

I modified it in the following manner. Let me know if it is fine.

"choices": [
+ {"type": "integer", "name": "tabId"},
+ {"$ref": "Tab", "name": "tab"}
+ ]


Now, we get the Tab object if 'tabs' permission is specified or the ID of the
duplicate tab if the 'tabs' permission has not been requested.



> 
>
http://codereview.chromium.org/10697017/diff/1/chrome/browser/extensions/api/...
> File chrome/browser/extensions/api/tabs/tabs.cc (right):
> 
>
http://codereview.chromium.org/10697017/diff/1/chrome/browser/extensions/api/...
> chrome/browser/extensions/api/tabs/tabs.cc:1122: int new_index = tab_index +
1;
> Bummer that we have to assume it's the next tab. Can you look at modifying
> Browser so that it returns the new tabcontents directly?

http://codereview.chromium.org/10697017/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:1122: int new_index = tab_index + 1;
On 2012/06/29 07:24:15, Aaron Boodman wrote:
> Bummer that we have to assume it's the next tab. Can you look at modifying
> Browser so that it returns the new tabcontents directly?

Done.

[Mihai Parparita -not on Chrome, 2012-07-03 22:57:18]

http://codereview.chromium.org/10697017/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs.cc:1118: NULL, &tab_strip, &contents,
&tab_index, &error_))
You need to pass in &browser instead of NULL here. There's no guarantee that the
tab being duplicated belongs to the current browser, so the tab index could
point do a different tab.

Also, if you use the browser that that is returned for the opening of the
duplicated tab, then you could remove the GetCurrentBrowser check entirely.

Finally, you may want to support an optional windowId parameter for the window
where the duplicated tab should be inserted. Though I suppose chrome.tabs.move
could be used with the return value, so it may not strictly be necessary.

[SanjoyPal, 2012-07-04 06:49:45]

Updated the patch after addressing review comments. PTAL.

http://codereview.chromium.org/10697017/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs.cc:1118: NULL, &tab_strip, &contents,
&tab_index, &error_))
On 2012/07/03 22:57:18, Mihai Parparita wrote:
> You need to pass in &browser instead of NULL here. There's no guarantee that
the
> tab being duplicated belongs to the current browser, so the tab index could
> point do a different tab.
> 
> Also, if you use the browser that that is returned for the opening of the
> duplicated tab, then you could remove the GetCurrentBrowser check entirely.
> 
> Finally, you may want to support an optional windowId parameter for the window
> where the duplicated tab should be inserted. Though I suppose chrome.tabs.move
> could be used with the return value, so it may not strictly be necessary.

Done. Let me know if we decide to support windowId parameter.

[Aaron Boodman, 2012-07-05 18:14:44]

http://codereview.chromium.org/10697017/diff/11003/chrome/common/extensions/a...
File chrome/common/extensions/api/tabs.json (right):

http://codereview.chromium.org/10697017/diff/11003/chrome/common/extensions/a...
chrome/common/extensions/api/tabs.json:281: {"type": "integer", "name":
"tabId"},
On second thought, I would prefer to return a Tab object but omit the URL,
title, and faviconUrl if the extension doesn't have the tabs permission.

[SanjoyPal, 2012-07-06 13:46:52]

Aaron, Shall I modify ExtensionTabUtil::CreateTabValue() to accept a bool
parameter hasPermission and omit url,
title and faviconUrl if hasPermission is false ?

http://codereview.chromium.org/10697017/diff/11003/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/11003/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1122:
result_.reset(ExtensionTabUtil::CreateTabValue(
shall i pass a bool parameter hasPermission to CreateTabValue and omit url,
title and faviconUrl if hasPermission is false ?

[SanjoyPal, 2012-07-10 08:51:09]

Callback function now returns Tab object but omits the url,
title, and faviconUrl if the extension doesn't have the tabs permission. PTAL.
Thanks.

http://codereview.chromium.org/10697017/diff/11003/chrome/common/extensions/a...
File chrome/common/extensions/api/tabs.json (right):

http://codereview.chromium.org/10697017/diff/11003/chrome/common/extensions/a...
chrome/common/extensions/api/tabs.json:281: {"type": "integer", "name":
"tabId"},
On 2012/07/05 18:14:44, Aaron Boodman wrote:
> On second thought, I would prefer to return a Tab object but omit the URL,
> title, and faviconUrl if the extension doesn't have the tabs permission.

Done.

[SanjoyPal, 2012-07-16 16:07:05]

Added apitest: ExtensionApiTest.TabDuplicate. PTAL.

[Aaron Boodman, 2012-07-16 21:37:59]

Nice work.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1123:
result_.reset(ExtensionTabUtil::CreateTabValue(
Can you change CreateTabValue to take the Extension* or maybe the
ExtensionPermissionSet and let it check the permissions itself? That would be a
little more foolproof than having all the callers have to do this check.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:551:
IN_PROC_BROWSER_TEST_F(ExtensionTabsTest, DuplicateTabNoPermission) {
What about a test for when the extension has the permission?

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:584: // tab id of TYPE_INTEGER
in the function result.
Comment is out of date.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:588: EXPECT_EQ(tab_index + 1,
duplicate_tab_index);
Can you test that URL, faviconURL, and title are *not* present?

Also can you factor that bit out into a utility function because it will be
commonly needed: crbug.com/137404.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:590: 
extraneous blank line

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:90: bool hasPermission) {
You're going to change this because of other comments, but remember:
variables_named_like_this.

[SanjoyPal, 2012-07-17 10:39:56]

Updated patch set 6 with review comments addressed. PTAL.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1123:
result_.reset(ExtensionTabUtil::CreateTabValue(
On 2012/07/16 21:37:59, Aaron Boodman wrote:
> Can you change CreateTabValue to take the Extension* or maybe the
> ExtensionPermissionSet and let it check the permissions itself? That would be
a
> little more foolproof than having all the callers have to do this check.

Yes. I ll do that.
Can I make these changes as part of crbug.com/137404?

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:551:
IN_PROC_BROWSER_TEST_F(ExtensionTabsTest, DuplicateTabNoPermission) {
On 2012/07/16 21:37:59, Aaron Boodman wrote:
> What about a test for when the extension has the permission?

Done.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:584: // tab id of TYPE_INTEGER
in the function result.
On 2012/07/16 21:37:59, Aaron Boodman wrote:
> Comment is out of date.

Done.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:588: EXPECT_EQ(tab_index + 1,
duplicate_tab_index);
On 2012/07/16 21:37:59, Aaron Boodman wrote:
> Can you test that URL, faviconURL, and title are *not* present?
> 
> Also can you factor that bit out into a utility function because it will be
> commonly needed: crbug.com/137404.

Done.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:590: 
On 2012/07/16 21:37:59, Aaron Boodman wrote:
> extraneous blank line

Done.

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/10697017/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:90: bool hasPermission) {
On 2012/07/16 21:37:59, Aaron Boodman wrote:
> You're going to change this because of other comments, but remember:
> variables_named_like_this.

Done.

[SanjoyPal, 2012-07-19 14:42:39]

ping.

[Matt Tytel, 2012-07-19 20:33:16]

Multiple parameters to callbacks are supported now :). Could you go with the
originally suggested callback? I'd like to see it in action.
aa: "callback(newTabId, newTabObject)"

[Aaron Boodman, 2012-07-19 20:42:38]

I actually prefer the signature
callback(TabObjectButMaybeMissingPrivacySensitiveFields tab)

So please don't change it Sanjoy. Matt, I'm sure the multiple params in callback
will come in useful elsewhere.

[Matt Tytel, 2012-07-19 21:15:36]

Aye, sorry for bugging you then Sanjoy.

[Yair (progame), 2012-08-06 19:35:02]

where does this review stand at the moment?
it looks like it's just waiting for a 4 lettered abbreviation (+ someone to
commit this for SanjoyPal?)

[Aaron Boodman, 2012-08-07 01:03:24]

Sorry for dropping this. It looks pretty close though, and this will be a really
nice addition.

You'll also need an owner of chrome/browser/ui. I've gone ahead and added sky@.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1113: &browser, &tab_strip,
&contents, &tab_index, &error_))
Nit: curly braces are required here.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1121: bool has_permission =
GetExtension()->HasAPIPermission(
Use HasAPIPermissionForTab. Currently the "ForTab" bit will never add any value,
but this will make it more robust to future changes.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:553: // Without a callback the
function will not generate a result.
Don't need to copy this comment.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:559:
scoped_ptr<base::DictionaryValue> result(utils::ToDictionary(
There's not really any need to use the extension API for this setup. Can you
just use C++ functions to add the tab and get its ID?

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:594:
EXPECT_TRUE(utils::HasPrivateInfo(duplicate_result.get()));
You could also test that the resulting tab is contained within the correct
browser.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_registry.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_registry.cc:107:
RegisterFunction<DuplicateTabFunction>();
As long as you're in here can you alphabetize the functions within this section?

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_test_utils.cc:115:
scoped_refptr<Extension> CreateEmptyExtensionWithValue(
This should also just be called CreateExtension. It's just an overload of the
other CreateExtension.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_test_utils.cc:135: bool
HasPrivateInfo(base::DictionaryValue* val) {
HasPrivacySensitiveFields

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.h (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.h:51: int tab_index, bool
has_permission);
I would prefer an enum rather than the boolean param. Can you add:

enum PrivacyFlags {
  INCLUDE_PRIVACY_SENSITIVE_FIELDS,
  OMIT_PRIVACY_SENSITIVE_FIELDS
}

[SanjoyPal, 2012-08-09 13:29:06]

On 2012/08/07 01:03:24, Aaron Boodman wrote:
> Sorry for dropping this. It looks pretty close though, and this will be a
really
> nice addition.
> 
> You'll also need an owner of chrome/browser/ui. I've gone ahead and added
sky@.
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> File chrome/browser/extensions/api/tabs/tabs.cc (right):
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/api/tabs/tabs.cc:1113: &browser, &tab_strip,
> &contents, &tab_index, &error_))
> Nit: curly braces are required here.
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/api/tabs/tabs.cc:1121: bool has_permission =
> GetExtension()->HasAPIPermission(
> Use HasAPIPermissionForTab. Currently the "ForTab" bit will never add any
value,
> but this will make it more robust to future changes.
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> File chrome/browser/extensions/api/tabs/tabs_test.cc (right):
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/api/tabs/tabs_test.cc:553: // Without a callback the
> function will not generate a result.
> Don't need to copy this comment.
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/api/tabs/tabs_test.cc:559:
> scoped_ptr<base::DictionaryValue> result(utils::ToDictionary(
> There's not really any need to use the extension API for this setup. Can you
> just use C++ functions to add the tab and get its ID?
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/api/tabs/tabs_test.cc:594:
> EXPECT_TRUE(utils::HasPrivateInfo(duplicate_result.get()));
> You could also test that the resulting tab is contained within the correct
> browser.
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_function_registry.cc (right):
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_function_registry.cc:107:
> RegisterFunction<DuplicateTabFunction>();
> As long as you're in here can you alphabetize the functions within this
section?
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_function_test_utils.cc (right):
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_function_test_utils.cc:115:
> scoped_refptr<Extension> CreateEmptyExtensionWithValue(
> This should also just be called CreateExtension. It's just an overload of the
> other CreateExtension.
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_function_test_utils.cc:135: bool
> HasPrivateInfo(base::DictionaryValue* val) {
> HasPrivacySensitiveFields
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_tab_util.h (right):
> 
>
http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_tab_util.h:51: int tab_index, bool
> has_permission);
> I would prefer an enum rather than the boolean param. Can you add:
> 
> enum PrivacyFlags {
>   INCLUDE_PRIVACY_SENSITIVE_FIELDS,
>   OMIT_PRIVACY_SENSITIVE_FIELDS
> }

sky, can please review the changes in chrome/browser/ui?

[sky, 2012-08-09 16:10:09]

c/b/u LGTM

[SanjoyPal, 2012-08-14 11:14:13]

Addressed the review comments. PTAL.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1113: &browser, &tab_strip,
&contents, &tab_index, &error_))
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> Nit: curly braces are required here.

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1121: bool has_permission =
GetExtension()->HasAPIPermission(
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> Use HasAPIPermissionForTab. Currently the "ForTab" bit will never add any
value,
> but this will make it more robust to future changes.

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:553: // Without a callback the
function will not generate a result.
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> Don't need to copy this comment.

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:559:
scoped_ptr<base::DictionaryValue> result(utils::ToDictionary(
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> There's not really any need to use the extension API for this setup. Can you
> just use C++ functions to add the tab and get its ID?

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs_test.cc:594:
EXPECT_TRUE(utils::HasPrivateInfo(duplicate_result.get()));
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> You could also test that the resulting tab is contained within the correct
> browser.

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_registry.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_registry.cc:107:
RegisterFunction<DuplicateTabFunction>();
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> As long as you're in here can you alphabetize the functions within this
section?

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_test_utils.cc:115:
scoped_refptr<Extension> CreateEmptyExtensionWithValue(
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> This should also just be called CreateExtension. It's just an overload of the
> other CreateExtension.

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_test_utils.cc:135: bool
HasPrivateInfo(base::DictionaryValue* val) {
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> HasPrivacySensitiveFields

Done.

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.h (right):

http://codereview.chromium.org/10697017/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.h:51: int tab_index, bool
has_permission);
On 2012/08/07 01:03:25, Aaron Boodman wrote:
> I would prefer an enum rather than the boolean param. Can you add:
> 
> enum PrivacyFlags {
>   INCLUDE_PRIVACY_SENSITIVE_FIELDS,
>   OMIT_PRIVACY_SENSITIVE_FIELDS
> }

Done.

[Aaron Boodman, 2012-08-15 04:09:04]

For future reference: It's polite to avoid rebasing a CL until you get LGTMs
from everyone. If you rebase, it makes it hard for reviewers to see what you
changed.

[Aaron Boodman, 2012-08-15 04:14:05]

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1109: if (has_callback()) {
You can invert this check and return early to reduce indentation.

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1110: bool has_permission =
GetExtension()->HasAPIPermissionForTab(tab_id,
Actually, I think it would be better to just pass Extension* to CreateTabValue
and let it decide whether to include the sensitive fields or not. This way, it
will be harder for callers to mess up.

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1112: if (has_permission)
Curly braces are required here...

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1117: else
... and here ...

[Aaron Boodman, 2012-09-04 21:25:57]

Ping? It would be nice to get this landed soon so that it can be in Chrome 23.

[SanjoyPal, 2012-09-05 08:31:15]

Sorry for being so late. For some reasons i was not able to work on chromium for
past few weeks.

Upadated the patch. PTAL.

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1109: if (has_callback()) {
On 2012/08/15 04:14:05, Aaron Boodman wrote:
> You can invert this check and return early to reduce indentation.

Done.

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1110: bool has_permission =
GetExtension()->HasAPIPermissionForTab(tab_id,
On 2012/08/15 04:14:05, Aaron Boodman wrote:
> Actually, I think it would be better to just pass Extension* to CreateTabValue
> and let it decide whether to include the sensitive fields or not. This way, it
> will be harder for callers to mess up.

Done. We need cleanup other callers of CreateTabValue to pass Extension*. Can we
do it as a separate activity/CL?

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1112: if (has_permission)
On 2012/08/15 04:14:05, Aaron Boodman wrote:
> Curly braces are required here...

Done.

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1117: else
On 2012/08/15 04:14:05, Aaron Boodman wrote:
> ... and here ...

Done.

[Aaron Boodman, 2012-09-05 21:56:21]

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1110: bool has_permission =
GetExtension()->HasAPIPermissionForTab(tab_id,
On 2012/09/05 08:31:15, SanjoyPal wrote:
> On 2012/08/15 04:14:05, Aaron Boodman wrote:
> > Actually, I think it would be better to just pass Extension* to
CreateTabValue
> > and let it decide whether to include the sensitive fields or not. This way,
it
> > will be harder for callers to mess up.
> 
> Done. We need cleanup other callers of CreateTabValue to pass Extension*. Can
we
> do it as a separate activity/CL?

Since I made the original comment, we already modified extension_tab_util to
take Extension* for all CreateTabValue overloads.

If you sync, you'll see it and you can just call that.

[SanjoyPal, 2012-09-06 10:53:16]

On 2012/09/05 21:56:21, Aaron Boodman wrote:
>
http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
> File chrome/browser/extensions/api/tabs/tabs.cc (right):
> 
>
http://codereview.chromium.org/10697017/diff/38002/chrome/browser/extensions/...
> chrome/browser/extensions/api/tabs/tabs.cc:1110: bool has_permission =
> GetExtension()->HasAPIPermissionForTab(tab_id,
> On 2012/09/05 08:31:15, SanjoyPal wrote:
> > On 2012/08/15 04:14:05, Aaron Boodman wrote:
> > > Actually, I think it would be better to just pass Extension* to
> CreateTabValue
> > > and let it decide whether to include the sensitive fields or not. This
way,
> it
> > > will be harder for callers to mess up.
> > 
> > Done. We need cleanup other callers of CreateTabValue to pass Extension*.
Can
> we
> > do it as a separate activity/CL?
> 
> Since I made the original comment, we already modified extension_tab_util to
> take Extension* for all CreateTabValue overloads.
> 
> If you sync, you'll see it and you can just call that.

Updated patch set 9. Please commit the changes.

[commit-bot: I haz the power, 2012-09-07 05:35:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/10697017/59002

[commit-bot: I haz the power, 2012-09-07 05:49:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/10697017/59002

[commit-bot: I haz the power, 2012-09-07 07:34:27]

Try job failure for 10697017-59002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-10 12:57:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/10697017/50021

[commit-bot: I haz the power, 2012-09-10 15:10:00]

Change committed as 155731