tabId support to chrome.extensions.getViews()

tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418
Committed: https://crrev.com/15fd52bd9893d7b72fda22e24d74c145b29221d1
Cr-Commit-Position: refs/heads/master@{#405631}

[catmullings, 2016-06-28 23:16:41]

Description was changed from

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418
==========

to

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418
==========

[catmullings, 2016-06-28 23:16:41]

catmullings@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-07-01 00:35:28]

Same here - no glaring issues, so once a test is up, I'll take a closer look. 
But overall looks like we're on the right track!

https://codereview.chromium.org/2105033003/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:16: // printf("Popup
Callback");
Don't forget to remove debugging code. :)

[catmullings, 2016-07-08 17:25:55]

Patchset #3 (id:40001) has been deleted

[Devlin, 2016-07-11 17:23:45]

Looking good!

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (left):

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:32: function getViews() {
this indentation was probably more correct before.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:14: // Updated and used
later to check getViews() tabId tag for options tab
We're sticklers for grammar in comments, so there shouldn't be any missing words
(so this should be "for *the* options tab"), and each should have proper
punctuation (including the ending period).

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:24: // Check if tab
returned is defined
indentation for callback functions is always kind of wonky, but in this case I
would recommend:
chrome.tabs.create(...
    function(tab) {
  ...
});

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:24: // Check if tab
returned is defined
this comment just describes what the code is doing right here, so I don't think
it really adds value.  I'd remove it.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:35:
chrome.tabs.query({"windowId": window.id}, function(tabs) {
I know you're just following the style from the rest of this test, but in JS,
prefer to not have quotes around the key in objects.  So just
{windowId: window.id}
rather than
{"windowId": window.id}

If you wanted to update the rest of this test too, that'd be good, but don't
feel like you have to.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:36: // Assert 3 to
account for new tab page, web popup, & new tab options.html
2-space indent here.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:39: // Check that only 2
tabs belong to the extension
Here, use a comment to describe why, not what.  That is, rather than saying you
are checking that only two tabs belong to the extension (which is clear from the
code), mention why there are two tabs (i.e., which two tabs?).

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:62: //printf("Other fn");
Don't forget to remove debugging code.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:76: "type": "popup"},
function(window) {
here, too, indentation was probably right before.

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:36: int match_tab_id) {
ditto re parameter ordering.

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.h (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.h:46: int tab_id);
for my OCD, I'd rather group similar parameter types together - so in this case,
have tab_id follow browser_window_id, and come before view_type.

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
File extensions/renderer/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
extensions/renderer/runtime_custom_bindings.cc:113: CHECK(args[2]->IsInt32());
ditto re parameter ordering.

[catmullings, 2016-07-12 18:13:48]

Addressed the suggestions made in the previous review.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:14: // Updated and used
later to check getViews() tabId tag for options tab
On 2016/07/11 17:23:45, Devlin wrote:
> We're sticklers for grammar in comments, so there shouldn't be any missing
words
> (so this should be "for *the* options tab"), and each should have proper
> punctuation (including the ending period).

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:24: // Check if tab
returned is defined
On 2016/07/11 17:23:45, Devlin wrote:
> indentation for callback functions is always kind of wonky, but in this case I
> would recommend:
> chrome.tabs.create(...
>     function(tab) {
>   ...
> });

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:24: // Check if tab
returned is defined
On 2016/07/11 17:23:45, Devlin wrote:
> this comment just describes what the code is doing right here, so I don't
think
> it really adds value.  I'd remove it.

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:35:
chrome.tabs.query({"windowId": window.id}, function(tabs) {
On 2016/07/11 17:23:45, Devlin wrote:
> I know you're just following the style from the rest of this test, but in JS,
> prefer to not have quotes around the key in objects.  So just
> {windowId: window.id}
> rather than
> {"windowId": window.id}
> 
> If you wanted to update the rest of this test too, that'd be good, but don't
> feel like you have to.

I'll go ahead and update the rest of the test with this. It would feel
inconsistent otherwise.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:35:
chrome.tabs.query({"windowId": window.id}, function(tabs) {
On 2016/07/11 17:23:45, Devlin wrote:
> I know you're just following the style from the rest of this test, but in JS,
> prefer to not have quotes around the key in objects.  So just
> {windowId: window.id}
> rather than
> {"windowId": window.id}
> 
> If you wanted to update the rest of this test too, that'd be good, but don't
> feel like you have to.

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:36: // Assert 3 to
account for new tab page, web popup, & new tab options.html
On 2016/07/11 17:23:45, Devlin wrote:
> 2-space indent here.

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:39: // Check that only 2
tabs belong to the extension
On 2016/07/11 17:23:45, Devlin wrote:
> Here, use a comment to describe why, not what.  That is, rather than saying
you
> are checking that only two tabs belong to the extension (which is clear from
the
> code), mention why there are two tabs (i.e., which two tabs?).

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:62: //printf("Other fn");
On 2016/07/11 17:23:45, Devlin wrote:
> Don't forget to remove debugging code.

Done.

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:76: "type": "popup"},
function(window) {
On 2016/07/11 17:23:45, Devlin wrote:
> here, too, indentation was probably right before.

Done.

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:36: int match_tab_id) {
On 2016/07/11 17:23:45, Devlin wrote:
> ditto re parameter ordering.

Done.

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.h (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.h:46: int tab_id);
On 2016/07/11 17:23:45, Devlin wrote:
> for my OCD, I'd rather group similar parameter types together - so in this
case,
> have tab_id follow browser_window_id, and come before view_type.

Done.

[Devlin, 2016-07-12 18:35:58]

Made it part of the way through, but looks like some changes weren't correctly
uploaded?

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:39: // Check that only 2
tabs belong to the extension
On 2016/07/12 18:13:47, catmullings wrote:
> On 2016/07/11 17:23:45, Devlin wrote:
> > Here, use a comment to describe why, not what.  That is, rather than saying
> you
> > are checking that only two tabs belong to the extension (which is clear from
> the
> > code), mention why there are two tabs (i.e., which two tabs?).
> 
> Done.

Was this actually done?

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:62: //printf("Other fn");
On 2016/07/12 18:13:47, catmullings wrote:
> On 2016/07/11 17:23:45, Devlin wrote:
> > Don't forget to remove debugging code.
> 
> Done.

Ditto

https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:76: "type": "popup"},
function(window) {
On 2016/07/12 18:13:47, catmullings wrote:
> On 2016/07/11 17:23:45, Devlin wrote:
> > here, too, indentation was probably right before.
> 
> Done.

Ditto

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
File extensions/renderer/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
extensions/renderer/runtime_custom_bindings.cc:113: CHECK(args[2]->IsInt32());
On 2016/07/11 17:23:45, Devlin wrote:
> ditto re parameter ordering.

Looks like this one wasn't addressed?

https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension.json (right):

https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension.json:98: "tabId": {
Looks like there are tabs lurking in here, but chromium uses only spaces (due to
tabs having variable lengths).

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:22:
chrome.tabs.create({"url": chrome.extension.getURL("options.html")},
s/"url"/url

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:32: {type: "tab",
windowId: window.id}).length);
indentation seems off. Prefer:
assertEq(2,
         chrome.extension.getViews({type: "tab", windowId: window.id}).length);

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:35: // Assert 3 to
account for new tab page, web popup, & new tab options.html
indentation: only 2-space increase for function scope

[catmullings, 2016-07-12 22:27:11]

On 2016/07/12 18:35:58, Devlin wrote:
> Made it part of the way through, but looks like some changes weren't correctly
> uploaded?
> 
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> File chrome/test/data/extensions/api_test/get_views/test.js (right):
> 
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/get_views/test.js:39: // Check that only
2
> tabs belong to the extension
> On 2016/07/12 18:13:47, catmullings wrote:
> > On 2016/07/11 17:23:45, Devlin wrote:
> > > Here, use a comment to describe why, not what.  That is, rather than
saying
> > you
> > > are checking that only two tabs belong to the extension (which is clear
from
> > the
> > > code), mention why there are two tabs (i.e., which two tabs?).
> > 
> > Done.
> 
> Was this actually done?
> 
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/get_views/test.js:62: //printf("Other
fn");
> On 2016/07/12 18:13:47, catmullings wrote:
> > On 2016/07/11 17:23:45, Devlin wrote:
> > > Don't forget to remove debugging code.
> > 
> > Done.
> 
> Ditto
> 
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/get_views/test.js:76: "type": "popup"},
> function(window) {
> On 2016/07/12 18:13:47, catmullings wrote:
> > On 2016/07/11 17:23:45, Devlin wrote:
> > > here, too, indentation was probably right before.
> > 
> > Done.
> 
> Ditto
> 
>
https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
> File extensions/renderer/runtime_custom_bindings.cc (right):
> 
>
https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
> extensions/renderer/runtime_custom_bindings.cc:113: CHECK(args[2]->IsInt32());
> On 2016/07/11 17:23:45, Devlin wrote:
> > ditto re parameter ordering.
> 
> Looks like this one wasn't addressed?
> 
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
> File chrome/common/extensions/api/extension.json (right):
> 
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
> chrome/common/extensions/api/extension.json:98: "tabId": {
> Looks like there are tabs lurking in here, but chromium uses only spaces (due
to
> tabs having variable lengths).
> 
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> File chrome/test/data/extensions/api_test/get_views/test.js (right):
> 
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/get_views/test.js:22:
> chrome.tabs.create({"url": chrome.extension.getURL("options.html")},
> s/"url"/url
> 
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/get_views/test.js:32: {type: "tab",
> windowId: window.id}).length);
> indentation seems off. Prefer:
> assertEq(2,
>          chrome.extension.getViews({type: "tab", windowId:
window.id}).length);
> 
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/get_views/test.js:35: // Assert 3 to
> account for new tab page, web popup, & new tab options.html
> indentation: only 2-space increase for function scope

Are you look at the 3rd patch below?
https://codereview.chromium.org/2105033003/diff2/60001:80001/chrome/test/data...

[catmullings, 2016-07-12 22:27:32]

On 2016/07/12 22:27:11, catmullings wrote:
> On 2016/07/12 18:35:58, Devlin wrote:
> > Made it part of the way through, but looks like some changes weren't
correctly
> > uploaded?
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> > File chrome/test/data/extensions/api_test/get_views/test.js (right):
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> > chrome/test/data/extensions/api_test/get_views/test.js:39: // Check that
only
> 2
> > tabs belong to the extension
> > On 2016/07/12 18:13:47, catmullings wrote:
> > > On 2016/07/11 17:23:45, Devlin wrote:
> > > > Here, use a comment to describe why, not what.  That is, rather than
> saying
> > > you
> > > > are checking that only two tabs belong to the extension (which is clear
> from
> > > the
> > > > code), mention why there are two tabs (i.e., which two tabs?).
> > > 
> > > Done.
> > 
> > Was this actually done?
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> > chrome/test/data/extensions/api_test/get_views/test.js:62: //printf("Other
> fn");
> > On 2016/07/12 18:13:47, catmullings wrote:
> > > On 2016/07/11 17:23:45, Devlin wrote:
> > > > Don't forget to remove debugging code.
> > > 
> > > Done.
> > 
> > Ditto
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
> > chrome/test/data/extensions/api_test/get_views/test.js:76: "type": "popup"},
> > function(window) {
> > On 2016/07/12 18:13:47, catmullings wrote:
> > > On 2016/07/11 17:23:45, Devlin wrote:
> > > > here, too, indentation was probably right before.
> > > 
> > > Done.
> > 
> > Ditto
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
> > File extensions/renderer/runtime_custom_bindings.cc (right):
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
> > extensions/renderer/runtime_custom_bindings.cc:113:
CHECK(args[2]->IsInt32());
> > On 2016/07/11 17:23:45, Devlin wrote:
> > > ditto re parameter ordering.
> > 
> > Looks like this one wasn't addressed?
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
> > File chrome/common/extensions/api/extension.json (right):
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
> > chrome/common/extensions/api/extension.json:98: "tabId": {
> > Looks like there are tabs lurking in here, but chromium uses only spaces
(due
> to
> > tabs having variable lengths).
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> > File chrome/test/data/extensions/api_test/get_views/test.js (right):
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> > chrome/test/data/extensions/api_test/get_views/test.js:22:
> > chrome.tabs.create({"url": chrome.extension.getURL("options.html")},
> > s/"url"/url
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> > chrome/test/data/extensions/api_test/get_views/test.js:32: {type: "tab",
> > windowId: window.id}).length);
> > indentation seems off. Prefer:
> > assertEq(2,
> >          chrome.extension.getViews({type: "tab", windowId:
> window.id}).length);
> > 
> >
>
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
> > chrome/test/data/extensions/api_test/get_views/test.js:35: // Assert 3 to
> > account for new tab page, web popup, & new tab options.html
> > indentation: only 2-space increase for function scope
> 
> Are you look at the 3rd patch below?
>
https://codereview.chromium.org/2105033003/diff2/60001:80001/chrome/test/data...

That one has the changes.

[Devlin, 2016-07-12 23:22:42]

On 2016/07/12 22:27:11, catmullings wrote:
> On 2016/07/12 18:35:58, Devlin wrote:
> > Made it part of the way through, but looks like some changes weren't
correctly
> > uploaded?
> Are you look at the 3rd patch below?
>
https://codereview.chromium.org/2105033003/diff2/60001:80001/chrome/test/data...

There seem to be many fixes that were marked "done", but weren't in patch set 4.
 e.g.

Patch Set 3:
https://codereview.chromium.org/2105033003/diff/60001/chrome/test/data/extens...
Comment is marked as done.

Patch Set 4:
https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
Offending line is still there.

Maybe a git issue?

[catmullings, 2016-07-13 00:01:29]

Patchset #5 (id:100001) has been deleted

[catmullings, 2016-07-13 00:04:30]

This patch should have the fixes that slipped from the patch 4 upload.

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:22:
chrome.tabs.create({"url": chrome.extension.getURL("options.html")},
On 2016/07/12 18:35:58, Devlin wrote:
> s/"url"/url

Done.

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:32: {type: "tab",
windowId: window.id}).length);
On 2016/07/12 18:35:57, Devlin wrote:
> indentation seems off. Prefer:
> assertEq(2,
>          chrome.extension.getViews({type: "tab", windowId:
window.id}).length);

Done.

https://codereview.chromium.org/2105033003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/get_views/test.js:35: // Assert 3 to
account for new tab page, web popup, & new tab options.html
On 2016/07/12 18:35:58, Devlin wrote:
> indentation: only 2-space increase for function scope

Done.

[Devlin, 2016-07-13 16:22:59]

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
File extensions/renderer/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
extensions/renderer/runtime_custom_bindings.cc:113: CHECK(args[2]->IsInt32());
On 2016/07/12 18:35:57, Devlin wrote:
> On 2016/07/11 17:23:45, Devlin wrote:
> > ditto re parameter ordering.
> 
> Looks like this one wasn't addressed?

This one still wasn't done. :)

https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension.json (right):

https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension.json:98: "tabId": {
On 2016/07/12 18:35:57, Devlin wrote:
> Looks like there are tabs lurking in here, but chromium uses only spaces (due
to
> tabs having variable lengths).

Missed this comment.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:22:
chrome.tabs.create({url: chrome.extension.getURL("options.html")},
nitty nit: argument parameters should line up with one another or all be on the
same line, so this should be something like:

chrome.tabs.create({url: chrome.extension.getURL("options.html")},
                   function(tab) {
  optionsTabId = tab.id;
});

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:32:
chrome.extension.getViews({type: "tab", windowId: window.id}).length);
ditto re parameter line-up:
assertEq(
    2, chrome.extension.getViews({type: "tab", windowId: window.id}).length);

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:35: // Assert 3 to
account for new tab page, web popup, & new tab options.html
need to end this comment in a '.'

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:38: // Check that only 2
tabs belong to the extension
ditto, though here this comment just describes the exactly what the line
immediately below does, so it should probably be rephrased.  Something like:
// getViews() only returns views that run in the extension process, so
// this should only include the popup and options.html pages.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:44: // Test tabId tag
with other parameters
end in a '.'

https://codereview.chromium.org/2105033003/diff/120001/extensions/common/cons...
File extensions/common/constants.h (right):

https://codereview.chromium.org/2105033003/diff/120001/extensions/common/cons...
extensions/common/constants.h:173: // Matches chrome.tabs.TAB_ID_NONE
'.'

https://codereview.chromium.org/2105033003/diff/120001/extensions/renderer/re...
File extensions/renderer/resources/runtime_custom_bindings.js (right):

https://codereview.chromium.org/2105033003/diff/120001/extensions/renderer/re...
extensions/renderer/resources/runtime_custom_bindings.js:180: var bg =
runtimeNatives.GetExtensionViews(-1, 'BACKGROUND', -1)[0] ||
wrap the expression to the next line:
var bg =
    runtimeNatives.GetExtensionViews(-1, 'BACKGROUND', -1)[0] || null;

[catmullings, 2016-07-13 22:07:06]

Addressed comments from patch 5.
Let me know if I missed any comments.

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
File extensions/renderer/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/2105033003/diff/60001/extensions/renderer/run...
extensions/renderer/runtime_custom_bindings.cc:113: CHECK(args[2]->IsInt32());
On 2016/07/13 16:22:58, Devlin wrote:
> On 2016/07/12 18:35:57, Devlin wrote:
> > On 2016/07/11 17:23:45, Devlin wrote:
> > > ditto re parameter ordering.
> > 
> > Looks like this one wasn't addressed?
> 
> This one still wasn't done. :)

Done.

https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension.json (right):

https://codereview.chromium.org/2105033003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension.json:98: "tabId": {
On 2016/07/13 16:22:59, Devlin wrote:
> On 2016/07/12 18:35:57, Devlin wrote:
> > Looks like there are tabs lurking in here, but chromium uses only spaces
(due
> to
> > tabs having variable lengths).
> 
> Missed this comment.

Done.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/get_views/test.js (right):

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:22:
chrome.tabs.create({url: chrome.extension.getURL("options.html")},
On 2016/07/13 16:22:59, Devlin wrote:
> nitty nit: argument parameters should line up with one another or all be on
the
> same line, so this should be something like:
> 
> chrome.tabs.create({url: chrome.extension.getURL("options.html")},
>                    function(tab) {
>   optionsTabId = tab.id;
> });

Done.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:32:
chrome.extension.getViews({type: "tab", windowId: window.id}).length);
On 2016/07/13 16:22:59, Devlin wrote:
> ditto re parameter line-up:
> assertEq(
>     2, chrome.extension.getViews({type: "tab", windowId: window.id}).length);

Done.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:35: // Assert 3 to
account for new tab page, web popup, & new tab options.html
On 2016/07/13 16:22:59, Devlin wrote:
> need to end this comment in a '.'

Done.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:38: // Check that only 2
tabs belong to the extension
On 2016/07/13 16:22:59, Devlin wrote:
> ditto, though here this comment just describes the exactly what the line
> immediately below does, so it should probably be rephrased.  Something like:
> // getViews() only returns views that run in the extension process, so
> // this should only include the popup and options.html pages.

Yeah, I did address this before, but it didn't get committed, for some reason.
I had written the following:

// Two tabs represent the extension's window popup and options.html

But I will use your comment since it gives more context.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:38: // Check that only 2
tabs belong to the extension
On 2016/07/13 16:22:59, Devlin wrote:
> ditto, though here this comment just describes the exactly what the line
> immediately below does, so it should probably be rephrased.  Something like:
> // getViews() only returns views that run in the extension process, so
> // this should only include the popup and options.html pages.

Done.

https://codereview.chromium.org/2105033003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/get_views/test.js:44: // Test tabId tag
with other parameters
On 2016/07/13 16:22:59, Devlin wrote:
> end in a '.'

Done.

https://codereview.chromium.org/2105033003/diff/120001/extensions/common/cons...
File extensions/common/constants.h (right):

https://codereview.chromium.org/2105033003/diff/120001/extensions/common/cons...
extensions/common/constants.h:173: // Matches chrome.tabs.TAB_ID_NONE
On 2016/07/13 16:22:59, Devlin wrote:
> '.'

Done.

https://codereview.chromium.org/2105033003/diff/120001/extensions/renderer/re...
File extensions/renderer/resources/runtime_custom_bindings.js (right):

https://codereview.chromium.org/2105033003/diff/120001/extensions/renderer/re...
extensions/renderer/resources/runtime_custom_bindings.js:180: var bg =
runtimeNatives.GetExtensionViews(-1, 'BACKGROUND', -1)[0] ||
On 2016/07/13 16:22:59, Devlin wrote:
> wrap the expression to the next line:
> var bg =
>     runtimeNatives.GetExtensionViews(-1, 'BACKGROUND', -1)[0] || null;

Done.

[Devlin, 2016-07-14 00:06:17]

lgtm, looks great!

[catmullings, 2016-07-14 17:19:36]

The CQ bit was checked by catmullings@chromium.org

[catmullings, 2016-07-14 17:19:39]

The CQ bit was unchecked by catmullings@chromium.org

[catmullings, 2016-07-14 22:32:08]

The CQ bit was checked by catmullings@chromium.org

[commit-bot: I haz the power, 2016-07-14 22:33:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 23:47:12]

Description was changed from

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418
==========

to

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418
==========

[commit-bot: I haz the power, 2016-07-14 23:47:13]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-07-14 23:47:36]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 23:48:55]

Description was changed from

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418
==========

to

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418

Committed: https://crrev.com/15fd52bd9893d7b72fda22e24d74c145b29221d1
Cr-Commit-Position: refs/heads/master@{#405631}
==========

[commit-bot: I haz the power, 2016-07-14 23:48:56]

Patchset 7 (id:??) landed as
https://crrev.com/15fd52bd9893d7b72fda22e24d74c145b29221d1
Cr-Commit-Position: refs/heads/master@{#405631}

[catmullings, 2016-08-03 21:42:28]

Description was changed from

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418

Committed: https://crrev.com/15fd52bd9893d7b72fda22e24d74c145b29221d1
Cr-Commit-Position: refs/heads/master@{#405631}
==========

to

==========
tabId support to chrome.extensions.getViews()

Modified chrome.extensions.getViews() to support filtering by tabId

BUG=72418

Committed: https://crrev.com/15fd52bd9893d7b72fda22e24d74c145b29221d1
Cr-Commit-Position: refs/heads/master@{#405631}
==========