Allow requesting Automation tree by tabId

Allow requesting Automation tree by tabId

BUG=309681
R=asvitkine@chromium.org, dtseng@chromium.org, kalman@chromium.org

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

[David Tseng, 2014-05-29 22:41:31]

https://codereview.chromium.org/308003003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/308003003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:115:
NULL,
What is this (in-line comments would help out)?

https://codereview.chromium.org/308003003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:137:
}
While we're here and I don't forget to ask, are you planning on handling the
"destroyed" code path to help us cleanup extension-side?

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/automation.idl (left):

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/automation.idl:95: // loaded (the previous root
node reference will stop working at or before
This comment needs updating (e.g. load_complete -> loadComplete, tabID is
optional/defaults to active tab, etc).

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/automation.idl:97: [nocompile] static void
getTree(optional long tabId, RootCallback callback);
Are we still ok with this name (getTab perhaps)?

https://codereview.chromium.org/308003003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js
(right):

https://codereview.chromium.org/308003003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js:15:
chrome.automation.getTree(tab.id, function(tree) {
This almost makes me want named object parameters (e.g.
getTree({tabID: tab.id})

Or the getTree function should be named getTab to make that clear.

https://codereview.chromium.org/308003003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js:16: var
title = tree.root.attributes['docTitle'];
nit: attributes.docTitle

[not at google - send to devlin, 2014-05-29 22:41:37]

I didn't really look at the tests in depth apart from that vague comment at the
end which might not be actionable. C++ lgtm.

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:116:
&tab_strip,
you can pass NULL here since |tab_strip| isn't used

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:118:
&tab_index)) {
likewise

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:119:
return RespondNow(Error(
see https://codereview.chromium.org/307983002/

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:142: }
ok :)

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:288: 
no blank line

https://codereview.chromium.org/308003003/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js
(right):

https://codereview.chromium.org/308003003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js:20: });
impressive

you should really be wrapping these callback (in fact, all callbacks from an
extension function result) in chrome.test.callbackPass, and unfortunately that
would make everything... so much worse. are there functions you can pull some of
this stuff into? I see you have "createTab"; it seems like the "get active tab
and fetch config" stuff could be some kind of initialization?

[aboxhall, 2014-05-30 15:37:10]

https://codereview.chromium.org/308003003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/308003003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:115:
NULL,
On 2014/05/29 22:41:31, David Tseng wrote:
> What is this (in-line comments would help out)?

Added inline comments.

https://codereview.chromium.org/308003003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:137:
}
On 2014/05/29 22:41:31, David Tseng wrote:
> While we're here and I don't forget to ask, are you planning on handling the
> "destroyed" code path to help us cleanup extension-side?

Yes, sorry - that is next on my list :)

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/automation.idl (left):

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/automation.idl:95: // loaded (the previous root
node reference will stop working at or before
On 2014/05/29 22:41:31, David Tseng wrote:
> This comment needs updating (e.g. load_complete -> loadComplete, tabID is
> optional/defaults to active tab, etc).

Done.

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/308003003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/automation.idl:97: [nocompile] static void
getTree(optional long tabId, RootCallback callback);
On 2014/05/29 22:41:31, David Tseng wrote:
> Are we still ok with this name (getTab perhaps)?

I like getTree as I think getTab will be confusing with the tabs API.

https://codereview.chromium.org/308003003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js
(right):

https://codereview.chromium.org/308003003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js:15:
chrome.automation.getTree(tab.id, function(tree) {
On 2014/05/29 22:41:31, David Tseng wrote:
> This almost makes me want named object parameters (e.g.
> getTree({tabID: tab.id})

What other named parameters might we want? I'm definitely up for doing this if
there's more than one, but tabID is the only one I can think of.

> Or the getTree function should be named getTab to make that clear.

I'm still ok with getTree - if you look at it in context in the tests, it's
either

chrome.automation.getTree(function(tree) { ... })

or

chrome.automation.getTree(tab.id, function(tree) { ... })

so particularly in the tabId case it's pretty clear that it's on a per-tab
basis. I think getTab() would be confusing as it's not retrieving a Tab object;
getTreeForTab() would be confusing if we're not requesting it by tabId.

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:116:
&tab_strip,
On 2014/05/29 22:41:37, kalman wrote:
> you can pass NULL here since |tab_strip| isn't used

Done.

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:118:
&tab_index)) {
On 2014/05/29 22:41:37, kalman wrote:
> likewise

Done.

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:119:
return RespondNow(Error(
On 2014/05/29 22:41:37, kalman wrote:
> see https://codereview.chromium.org/307983002/

Done.

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/308003003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:288: 
On 2014/05/29 22:41:37, kalman wrote:
> no blank line

Done.

https://codereview.chromium.org/308003003/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js
(right):

https://codereview.chromium.org/308003003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js:20: });
On 2014/05/29 22:41:37, kalman wrote:
> impressive
> 
> you should really be wrapping these callback (in fact, all callbacks from an
> extension function result) in chrome.test.callbackPass, and unfortunately that
> would make everything... so much worse. are there functions you can pull some
of
> this stuff into? I see you have "createTab"; it seems like the "get active tab
> and fetch config" stuff could be some kind of initialization?

How do you mean functions I could pull this stuff into? They'd be one-offs... I
could pull the functions up into the main function and then call them by
name...?

[aboxhall, 2014-05-30 15:38:21]

+asvitkine@ for extension_function_histogram_value.h

[aboxhall, 2014-05-30 15:38:25]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:38:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/308003003/70001

[aboxhall, 2014-05-30 15:39:01]

The CQ bit was unchecked by aboxhall@chromium.org

[Alexei Svitkine (slow), 2014-05-30 15:53:30]

not lgtm

Please update histograms.xml too.

[not at google - send to devlin, 2014-05-30 15:54:13]

On 2014/05/30 15:53:30, Alexei Svitkine wrote:
> not lgtm
> 
> Please update histograms.xml too.

I swear there was a bug somewhere about making this part of the presubmit...

[aboxhall, 2014-05-30 15:56:01]

On 2014/05/30 15:54:13, kalman wrote:
> On 2014/05/30 15:53:30, Alexei Svitkine wrote:
> > not lgtm
> > 
> > Please update histograms.xml too.
> 
> I swear there was a bug somewhere about making this part of the presubmit...

Which histograms.xml? And how?

% find . -name histograms.xml
./tools/metrics/histograms/histograms.xml
./tools/histograms/histograms.xml

[aboxhall, 2014-05-30 15:57:33]

On 2014/05/30 15:56:01, aboxhall wrote:
> On 2014/05/30 15:54:13, kalman wrote:
> > On 2014/05/30 15:53:30, Alexei Svitkine wrote:
> > > not lgtm
> > > 
> > > Please update histograms.xml too.
> > 
> > I swear there was a bug somewhere about making this part of the presubmit...
> 
> Which histograms.xml? And how?
> 
> % find . -name histograms.xml
> ./tools/metrics/histograms/histograms.xml
> ./tools/histograms/histograms.xml

Never mind, git grep saved me.

[aboxhall, 2014-05-30 16:08:25]

On 2014/05/30 15:53:30, Alexei Svitkine wrote:
> not lgtm
> 
> Please update histograms.xml too.

Done.

[aboxhall, 2014-05-30 16:09:16]

https://codereview.chromium.org/308003003/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js
(right):

https://codereview.chromium.org/308003003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/automation/tests/tabs/tab_id.js:20: });
On 2014/05/30 15:37:11, aboxhall wrote:
> On 2014/05/29 22:41:37, kalman wrote:
> > impressive
> > 
> > you should really be wrapping these callback (in fact, all callbacks from an
> > extension function result) in chrome.test.callbackPass, and unfortunately
that
> > would make everything... so much worse. are there functions you can pull
some
> of
> > this stuff into? I see you have "createTab"; it seems like the "get active
tab
> > and fetch config" stuff could be some kind of initialization?
> 
> How do you mean functions I could pull this stuff into? They'd be one-offs...
I
> could pull the functions up into the main function and then call them by
> name...?

Done (after offline discussion).

[Alexei Svitkine (slow), 2014-05-30 16:26:42]

lgtm

[aboxhall, 2014-05-30 16:45:53]

dtseng@: does this LGTY?

[David Tseng, 2014-05-30 18:31:57]

lgtm

[aboxhall, 2014-05-30 18:35:21]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-05-30 18:38:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/308003003/90001

[commit-bot: I haz the power, 2014-05-31 00:09:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-31 00:18:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-31 00:18:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[aboxhall, 2014-05-31 00:37:15]

Committed patchset #7 manually as r273958 (presubmit successful).