Telemetry support for extensions.

Telemetry support for extensions.

* Browser has new properties supports_extensions and extensions (of type ExtensionDict)
* BrowserBackend ctor takes additional arg supports_extensions. 
* BrowserBackend has new properties supports_extensions and extension_dict_backend (of type ExtensionDictBackend)
* BrowserBackend._WaitForBrowserToComeUp waits for all extensions to load and be interactive.
* BrowserBackend.ExtensionsNotSupportedException is raised for android browser and content shell.
* Add class ExtensionToLoad which is a wrapper for an extension_path.
* Add extension_to_load (of type ExtensionToLoad) to BrowserOptions.
* Add SupportsOptions to PossibleBrowser, to test extensions support. Returns False for android and content_shell when BrowserOptions.extensions_to_load is not empty 
* Add ExtensionPage which derives from WebContents
* Add ExtensionDictBackend which is a dictionary of ExtensionPage instances with extension ids as keys.
* Add ExtensionDict which passes through to ExtensionDictBackend, except it uses ExtensionToLoad instances as keys.
* Import crx_id for determining extension id from extension path.
* Simple extension manifest.json and background.js for unit tests.
* Unit tests for single and multiple extensions.

BUG=169954
TEST=browser test.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182316

[achuithb, 2013-01-15 00:53:50]

Still working on making sense of it all.

Please let me know if a further split is desirable.

[nduca, 2013-01-15 05:31:05]

So glad to see you picking this up. Thank you, and good job picking this up. Its
a lot of code to learn.

Lots of feedback, please reach out on gchat if anything confuses you.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/examples/tele...
File tools/telemetry/examples/telemetry_extension_test.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/examples/tele...
tools/telemetry/examples/telemetry_extension_test.py:15: parser =
options.CreateParser('telemetry_extension_test.py')
This file is missing a command to add an extension to the extensions_to_load.
Right now, its assuming (Requiring) that the user pass them in via the command
line.

Something like

   extension_to_load = ExtensionToLoad(os.path.join(os.path.dirname(__file__),
 unittestdatadir, 'extension_test')))

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/examples/tele...
tools/telemetry/examples/telemetry_extension_test.py:22:
'chrome-extension://aapnijgdinlhnhlmodcfapnahmbfebeb/' +
this part bugs me --- we're hand-coding in the extension id. We should figure
out a way to get the extension id before we load it. Ideally, we'd add an id
getter property on extension and then do extension_to_load.id?

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
File tools/telemetry/telemetry/browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_backend.py:27: class ContextControler(object):
I think we should leave TabController alone and create a new class just for
extensions that hangs off browser.

I think its better to say
  browser.tabs
and
  browser.extensions[extension_id]

then make people understand the concept of a context.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_backend.py:91: def FindByUrl(self, url):
I'd like to avoid this.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_backend.py:96: return None
Should ExtensionToLoad() should have an id? The user shouldn't have to hard-code
the id of their extension, right? How does the user find their extension?

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
File tools/telemetry/telemetry/browser_options.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_options.py:14: def __init__(self, path,
is_component):
should we assert that the extension exists?

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_options.py:16: self.is_component =
is_component
we need to find some common area that validates that the extension path exists
and contains a manifest that parses, and throws if not. Possibly here when its
constructed?

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_options.py:164: if
self.component_extensions_to_load_as_string: # pylint: disable=E1101
i dont think we need to add support for loading extensions from the command line
yet. Lets do that if we need it. All we need in this file is the ExtensionToLoad
and the addition of extensions_to_load to browser_options.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
File tools/telemetry/telemetry/browser_unittest.py (left):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_unittest.py:34:
t.WaitForDocumentReadyStateToBeInteractiveOrBetter()
missing tests for plain old extensions. you should test that first.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
File tools/telemetry/telemetry/browser_unittest.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_unittest.py:44: options =
options_for_unittests.GetCopy()
automation extension means nothing in telemetry.

if this is a test of component extension, thats fine. But then you need to call
it that, and name it component_extension_test

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/bro...
tools/telemetry/telemetry/browser_unittest.py:54:
'chrome-extension://aapnijgdinlhnhlmodcfapnahmbfebeb/' +
more hard coded extension ids. No good.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/cro...
File tools/telemetry/telemetry/cros_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/cro...
tools/telemetry/telemetry/cros_browser_backend.py:56: def
_InstallExtension(self, local_path, device_path):
who calls this?

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/ext...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/ext...
tools/telemetry/telemetry/extension_page.py:22: class ExtensionPage(object):
I'd like to focus on getting just a plain old extension working with no methods
other than runtime.Evaluate('1+1') before we start adding features for calling
methosd that take callbacks and stuff like that. Those can be follwoups.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/ext...
tools/telemetry/telemetry/extension_page.py:24: 
The tab_backend vs tab rewrite should really help a lot here. You want this to
be equivalent to tab.py --- and contain a tab_backend -- - and possibly rename
tab_backend in dtu's patch to make i tcleaner.

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/pos...
File tools/telemetry/telemetry/possible_browser.py (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/telemetry/pos...
tools/telemetry/telemetry/possible_browser.py:10: def __init__(self,
browser_type, options, supports_extensions):
is there any way to not pass this in on the arg list? Can't we have cros and
desktop browser backend return true if not content shell, and android always
return false?

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/unittest_data...
File tools/telemetry/unittest_data/auto_provider/background.js (right):

https://codereview.chromium.org/11882033/diff/1/tools/telemetry/unittest_data...
tools/telemetry/unittest_data/auto_provider/background.js:3: // found in the
LICENSE file.
lets call this test_extension instead of auto provider and reduce its manifest
to the minimum required

[nduca, 2013-01-22 23:00:01]

This looks really good. We're still missing the logic to figure out extension
ids up front, and tests. I also want @dtu's feedback on the context object.

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser.py (right):

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:31: if not
browser_backend.is_content_shell:
always initialize the extensions object. just make it empty and not crash if the
browser doesn't support it.

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/context_list_backend.py (right):

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
tools/telemetry/telemetry/context_list_backend.py:1: # Copyright (c) 2012 The
Chromium Authors. All rights reserved.
maybe dtu can advise on this. The word context is kinda messing with my head,
but I understand we need a name for the common code. What does inspector use for
this? Eg chrome://inspect/ and its backends?

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:7: class ExtensionEvent(object):
unused?

[achuithb, 2013-01-22 23:08:58]

On 2013/01/22 23:00:01, nduca wrote:
> This looks really good. We're still missing the logic to figure out extension
> ids up front, and tests. I also want @dtu's feedback on the context object.

I've added extension_unittest.py - let me know what you think?

I can't think of any other way to get to the extension you care about if you
don't know the extension id. Extensions don't have unique names or properties
outside of the extension id.

Thanks for wading through this. I'm in the process of updating the CL
description to describe what I'm trying to do with this patch.

[nduca, 2013-01-22 23:13:59]

Please talk to zel, who can set you up with folks from the extensions team who
can tell you how the extension id is generated. We need it to be predictable.

[nduca, 2013-01-22 23:14:24]

(also, take a peek at tools/crx_id maybe its helpful?!)

[dtu, 2013-01-23 00:55:17]

The only thing they have in common is that they're inspectable, so
inspectable_element? tabstension works too!!

[nduca, 2013-01-23 01:01:28]

I think we should leave tab_list alone, and see if we can winnow down the bits
of tab_list we need for extensions. It sounds like extensions needs considerably
lest than tabs.

[achuithb, 2013-01-23 01:20:21]

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser.py (right):

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:31: if not
browser_backend.is_content_shell:
On 2013/01/22 23:00:01, nduca wrote:
> always initialize the extensions object. just make it empty and not crash if
the
> browser doesn't support it.

Done.

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/context_list_backend.py (right):

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
tools/telemetry/telemetry/context_list_backend.py:1: # Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/22 23:00:01, nduca wrote:
> maybe dtu can advise on this. The word context is kinda messing with my head,
> but I understand we need a name for the common code. What does inspector use
for
> this? Eg chrome://inspect/ and its backends?

WebContent perhaps? I think that's what the C++ code is called now, previously
used to be TabContents.

chrome://inspect has sections like
Pages:
...
Extensions:
...
Shared Workers:
...
Other:
...

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/12002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:7: class ExtensionEvent(object):
On 2013/01/22 23:00:01, nduca wrote:
> unused?

Done.

[achuithb, 2013-01-23 01:25:49]

On 2013/01/23 01:01:28, nduca wrote:
> I think we should leave tab_list alone, and see if we can winnow down the bits
> of tab_list we need for extensions. It sounds like extensions needs
considerably
> lest than tabs.

Ok, let me take a whack at that.

[achuithb, 2013-01-23 21:01:31]

I've gone ahead and copied all the contents of tab_contents_backend.py instead
of sub-classing.

Nat: PTAL

[dtu, 2013-01-23 22:34:56]

On 2013/01/23 21:01:31, achuith.bhandarkar wrote:
> I've gone ahead and copied all the contents of tab_contents_backend.py instead
> of sub-classing.
> 
> Nat: PTAL

The reason why extensions is simpler than tabs is that you don't need to look up
extensions by list index. You can only look them up by ID. So you can remove
everything related to maintaining the extensions list, including __getitem__,
_extension_list, _UpdateExtensionList, _ListExtensions, etc. Then maybe it
should be called extension_dict or something like that and implement Python
dictionary methods.

[nduca, 2013-01-23 23:20:53]

Did you find a solution for getting the extension id?

[achuithb, 2013-01-23 23:26:06]

On 2013/01/23 23:20:53, nduca wrote:
> Did you find a solution for getting the extension id?

Yup, packing an extension in chrome will generate a key + extension id. I need
to check in the key file as well with this CL. I'm working on this right now.

[achuithb, 2013-01-23 23:26:36]

On 2013/01/23 22:34:56, Dave Tu wrote:
> On 2013/01/23 21:01:31, achuith.bhandarkar wrote:
> > I've gone ahead and copied all the contents of tab_contents_backend.py
instead
> > of sub-classing.
> > 
> > Nat: PTAL
> 
> The reason why extensions is simpler than tabs is that you don't need to look
up
> extensions by list index. You can only look them up by ID. So you can remove
> everything related to maintaining the extensions list, including __getitem__,
> _extension_list, _UpdateExtensionList, _ListExtensions, etc. Then maybe it
> should be called extension_dict or something like that and implement Python
> dictionary methods.

ok, I'll look at getting rid of the extension_list.

[nduca, 2013-01-24 00:02:12]

it should be possible for someone to say "load this extension from this folder"
without them having to add the key file or pack it. That is to say, dont force
the user of telemetry to have to generate the keyfile themselves.

[achuithb, 2013-01-25 00:53:54]

So crx_id does the job! 

PTAL, Nat and Dave!

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:19:
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..'))
Is there a better way to do this?

[nduca, 2013-01-25 19:52:29]

Did some of the backend changes disappear from this patch?

[nduca, 2013-01-25 20:01:52]

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:62: 
Thinking out loud: do we need a supports_extensions field on the public browser
object?

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:19:
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..'))
Yep make a file in this folder called crx_id.py  and import that. Then copy the
design pattern of perf_tests_helper and have that file do the rest of the
sys.path manipulation to import for you.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:13: def
testDefaultExtensions(self):
I think your objective in this test is to verify extension listing. But, its not
good that we only verify that using chromeos.

You should have a few extensions in unittest_data, install them all, then verify
that they're there. That way we can verify it anywhere.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:28: options =
options_for_unittests.GetCopy()
will this pass against a content shell browser?

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:39: self.assertEquals(2,
extension.tab.EvaluateJavaScript("1+1"))
extension.tab is very bad. Extensions should not be tabs.

Figure out how we can make it extension.EvaluateJavaScript

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:41:
extension.tab.EvaluateJavaScript("simpleRightTriangle()"))
What are you testing here? That the background js is there? That you can get to
the extension and run script on it? Is there material difference between the
two? If there is, this is two tests. If not, then pick one and only do that one.

Then your method names should be clearly named. E.g. bg.js exports a variable
__did_background_js_run = True

then have this python test assert that that is true.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:44:
extension.RunSyncMethod('hypotenuseOfRightTriangle', 5, 12))
No no no no. RunSymcMethod need to not be part of this patch.

[achuithb, 2013-01-26 01:28:59]

On 2013/01/25 19:52:29, nduca wrote:
> Did some of the backend changes disappear from this patch?

This is the extension_list_backend.py and extension_list.py split, right? Do you
want this? I was just copy-pasting tab_list_backend.py, and didn't really
understand what the purpose of this was.

[nduca, 2013-01-26 01:36:01]

Where did the browser_options changes go?

[achuithb, 2013-01-29 21:15:55]

So since the last review:
* Fixed the crx_id import.
* Added the supports_extensions field
* ExtensionListBackend/ExtensionList split.
* browser options support for load-extension and load-component-extension
* unit tests updated to use browser_options.extensions_to_load
* RunSyncMethod is gone
* unit test for multiple extensions to test iteration, etc. I've left the cros
default extension test since this verifies default cros extensions.

I have some questions below

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:62: 
On 2013/01/25 20:01:53, nduca wrote:
> Thinking out loud: do we need a supports_extensions field on the public
browser
> object?

Done.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:19:
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..'))
On 2013/01/25 20:01:53, nduca wrote:
> Yep make a file in this folder called crx_id.py  and import that. Then copy
the
> design pattern of perf_tests_helper and have that file do the rest of the
> sys.path manipulation to import for you.

Done.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:13: def
testDefaultExtensions(self):
On 2013/01/25 20:01:53, nduca wrote:
> I think your objective in this test is to verify extension listing. But, its
not
> good that we only verify that using chromeos.
> 
> You should have a few extensions in unittest_data, install them all, then
verify
> that they're there. That way we can verify it anywhere.

Done.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:28: options =
options_for_unittests.GetCopy()
On 2013/01/25 20:01:53, nduca wrote:
> will this pass against a content shell browser?

Done.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:39: self.assertEquals(2,
extension.tab.EvaluateJavaScript("1+1"))
On 2013/01/25 20:01:53, nduca wrote:
> extension.tab is very bad. Extensions should not be tabs.
> 
> Figure out how we can make it extension.EvaluateJavaScript

I've renamed this to extension.context, and added extension.EvaluateJavascript,
but I don't think this is quite what you want.

So Tab and TabBackend are actually Context and ContextBackend (or WebContent, or
whatever), since they just connect via a debugger url to a context which could
be a tab or an extension. In this case, it's actually an extension. I could
rename Tab/TabBackend to Context or something similar? I don't see any point of
copying all the code of Tab/TabBackend to ExtensionPage.

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:41:
extension.tab.EvaluateJavaScript("simpleRightTriangle()"))
On 2013/01/25 20:01:53, nduca wrote:
> What are you testing here? That the background js is there? That you can get
to
> the extension and run script on it? Is there material difference between the
> two? If there is, this is two tests. If not, then pick one and only do that
one.

I care about the 2nd - run a script in background.js, so I'm testing that.
 
> Then your method names should be clearly named. E.g. bg.js exports a variable
> __did_background_js_run = True
> 
> then have this python test assert that that is true.

I don't follow this? How do you export a variable from js?

https://codereview.chromium.org/11882033/diff/29002/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:44:
extension.RunSyncMethod('hypotenuseOfRightTriangle', 5, 12))
On 2013/01/25 20:01:53, nduca wrote:
> No no no no. RunSymcMethod need to not be part of this patch.

Done.

[dtu, 2013-01-30 00:01:48]

Here's what I think it should look like. Nat, feel free to disagree.

For Tab and ExtensionPage:
Both classes are almost good as-is. They can both use TabBackend as their
backends, since they both require all of the WebSocket/SyncRequest stuff. Rename
it to InspectorBackend.

For ExtensionList:
It should act like a Python dict, with __iter__ and __getitem__.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list.py (right):

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:5: class ExtensionList(object):
ExtensionDict. I'd like if this class pretends to be a Python dict object that
maps IDs to ExtensionPages. Iterating over the dict generates a list of keys
(IDs) and doing extensions[id] returns the extension with the specified id.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:15: def GetExtensionById(self,
extension_id):
Override __getitem__ instead.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:18: def GetExtensionIds(self):
Remove. Users should just use the iterator.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list_backend.py (right):

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:27: self._extension_ids = []
You don't need to store this list. You can look it up on the fly. (Every method
that uses it does this already.)

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:55: def
GetExtensionById(self, extension_id):
__getitem__

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:68:
self._CreateContext(extension_info['webSocketDebuggerUrl']))
Use the TabBackend directly and remove _CreateContext.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:99: def
GetExtensionIds(self):
Remove.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:10: self._context = context
Use a TabBackend instead of a Tab here. Call it self._extension_page_backend.
Rename TabBackend to InspectorBackend.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:12: def __enter__(self):
Don't need enter and exit.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:18: def EvaluateJavaScript(self,
expr, timeout=DEFAULT_EXTENSION_TIMEOUT):
Also include ExecuteJavaScript.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:19:
self._context.WaitForDocumentReadyStateToBeInteractiveOrBetter()
I think Waiting is the responsibility of the caller. This should just be a
simple pass-through to the backend.

[achuithb, 2013-01-30 10:01:56]

My apologies for the rebase + fixes :(

PTAL, Dave and Nat.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list.py (right):

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:5: class ExtensionList(object):
On 2013/01/30 00:01:49, Dave Tu wrote:
> ExtensionDict. I'd like if this class pretends to be a Python dict object that
> maps IDs to ExtensionPages. Iterating over the dict generates a list of keys
> (IDs) and doing extensions[id] returns the extension with the specified id.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:15: def GetExtensionById(self,
extension_id):
On 2013/01/30 00:01:49, Dave Tu wrote:
> Override __getitem__ instead.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list.py:18: def GetExtensionIds(self):
On 2013/01/30 00:01:49, Dave Tu wrote:
> Remove. Users should just use the iterator.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_list_backend.py (right):

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:27: self._extension_ids = []
On 2013/01/30 00:01:49, Dave Tu wrote:
> You don't need to store this list. You can look it up on the fly. (Every
method
> that uses it does this already.)

I think I need this for __iter__ below, right? I can't return an iterator to a
temporary, right?

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:55: def
GetExtensionById(self, extension_id):
On 2013/01/30 00:01:49, Dave Tu wrote:
> __getitem__

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:68:
self._CreateContext(extension_info['webSocketDebuggerUrl']))
On 2013/01/30 00:01:49, Dave Tu wrote:
> Use the TabBackend directly and remove _CreateContext.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_list_backend.py:99: def
GetExtensionIds(self):
On 2013/01/30 00:01:49, Dave Tu wrote:
> Remove.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:10: self._context = context
On 2013/01/30 00:01:49, Dave Tu wrote:
> Use a TabBackend instead of a Tab here. Call it self._extension_page_backend.
> Rename TabBackend to InspectorBackend.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:12: def __enter__(self):
On 2013/01/30 00:01:49, Dave Tu wrote:
> Don't need enter and exit.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:18: def EvaluateJavaScript(self,
expr, timeout=DEFAULT_EXTENSION_TIMEOUT):
On 2013/01/30 00:01:49, Dave Tu wrote:
> Also include ExecuteJavaScript.

Done.

https://codereview.chromium.org/11882033/diff/33005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:19:
self._context.WaitForDocumentReadyStateToBeInteractiveOrBetter()
On 2013/01/30 00:01:49, Dave Tu wrote:
> I think Waiting is the responsibility of the caller. This should just be a
> simple pass-through to the backend.

Done.

[achuithb, 2013-01-30 10:03:06]

On 2013/01/30 00:01:48, Dave Tu wrote:
> Here's what I think it should look like. Nat, feel free to disagree.
> 
> For Tab and ExtensionPage:
> Both classes are almost good as-is. They can both use TabBackend as their
> backends, since they both require all of the WebSocket/SyncRequest stuff.
Rename
> it to InspectorBackend.

I've done this rename in a separate patch:
https://codereview.chromium.org/12079070/

[dtu, 2013-01-31 01:37:37]

This looks very close to me. nduca, did you want to give this an okay, or are
you too busy?

https://chromiumcodereview.appspot.com/11882033/diff/33005/tools/telemetry/te...
File tools/telemetry/telemetry/extension_list_backend.py (right):

https://chromiumcodereview.appspot.com/11882033/diff/33005/tools/telemetry/te...
tools/telemetry/telemetry/extension_list_backend.py:27: self._extension_ids = []
On 2013/01/30 10:01:56, achuith.bhandarkar wrote:
> On 2013/01/30 00:01:49, Dave Tu wrote:
> > You don't need to store this list. You can look it up on the fly. (Every
> method
> > that uses it does this already.)
> 
> I think I need this for __iter__ below, right? I can't return an iterator to a
> temporary, right?

Tried it just now and you can.  My guess is the iterator keeps a reference to
the object, which keeps it from being garbage collected.

class Foo(object):
  def __iter__(self):
    return [1, 2, 3].__iter__()

for e in Foo():
  print e

https://chromiumcodereview.appspot.com/11882033/diff/38022/tools/telemetry/te...
File tools/telemetry/telemetry/extension_dict.py (right):

https://chromiumcodereview.appspot.com/11882033/diff/38022/tools/telemetry/te...
tools/telemetry/telemetry/extension_dict.py:18: def GetExtensionById(self,
extension_id):
Remove.

https://chromiumcodereview.appspot.com/11882033/diff/38022/tools/telemetry/te...
tools/telemetry/telemetry/extension_dict.py:21: def GetExtensionIds(self):
Remove.

[achuithb, 2013-01-31 02:49:49]

* Renamed _UpdateExtensionIdList to _GetExtensionIds - it now returns a list.
* Got rid of _extension_ids and Init
* I added some util.WaitFor in the unit tests so we test extensions after they
have finished loading the browser.

I'll wait for Nat's review.

https://chromiumcodereview.appspot.com/11882033/diff/33005/tools/telemetry/te...
File tools/telemetry/telemetry/extension_list_backend.py (right):

https://chromiumcodereview.appspot.com/11882033/diff/33005/tools/telemetry/te...
tools/telemetry/telemetry/extension_list_backend.py:27: self._extension_ids = []
On 2013/01/31 01:37:37, Dave Tu wrote:
> On 2013/01/30 10:01:56, achuith.bhandarkar wrote:
> > On 2013/01/30 00:01:49, Dave Tu wrote:
> > > You don't need to store this list. You can look it up on the fly. (Every
> > method
> > > that uses it does this already.)
> > 
> > I think I need this for __iter__ below, right? I can't return an iterator to
a
> > temporary, right?
> 
> Tried it just now and you can.  My guess is the iterator keeps a reference to
> the object, which keeps it from being garbage collected.
> 
> class Foo(object):
>   def __iter__(self):
>     return [1, 2, 3].__iter__()
> 
> for e in Foo():
>   print e

Done.

https://chromiumcodereview.appspot.com/11882033/diff/38022/tools/telemetry/te...
File tools/telemetry/telemetry/extension_dict.py (right):

https://chromiumcodereview.appspot.com/11882033/diff/38022/tools/telemetry/te...
tools/telemetry/telemetry/extension_dict.py:18: def GetExtensionById(self,
extension_id):
On 2013/01/31 01:37:37, Dave Tu wrote:
> Remove.

Done.

https://chromiumcodereview.appspot.com/11882033/diff/38022/tools/telemetry/te...
tools/telemetry/telemetry/extension_dict.py:21: def GetExtensionIds(self):
On 2013/01/31 01:37:37, Dave Tu wrote:
> Remove.

Done.

[achuithb, 2013-01-31 20:22:11]

ping?

[nduca, 2013-01-31 21:23:38]

dtu can you give exhaustive review?

[nduca, 2013-02-01 00:25:50]

I think we're getting close. Still some issues but not too horrifying.

I'd like a base class for Tab and Extension where some of tab goes. There could
be a refactoring before this that does that split, but i do not want this to
land until that happens. There are reasons [test coverage issues with the
current approach]. We've been unsure about names for forever, so lets use
WebContents.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/android_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/android_browser_backend.py:24: raise Exception('Cannot
load extensions for android browser!')
Android browser does not support extensions.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:32: if not
browser_backend.is_content_shell:
android isn't content shell.... treat these separately --- ask the browser
backend if it supports extensions

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:64: @property
you dont have a supports_extensions property. you should.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:65: def extensions(self):
doc tring should say that this may not be there

accessor should raise  special exception if its not supported

   raise ExtensionsNotSupportedException) or something

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser_options.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:13: class ExtensionToLoad(object):
This should be its own py file

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:14: def __init__(self, path,
is_component):
load_as_component=False

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:16: self.is_component =
is_component
this should have an accessor for crx id

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:98:
group.add_option('--load-component-extension',
Remove all the edits here. I do not want users to be able to load extensions
this way. self.extensions_to_load, yes. Not here.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:166: if
self.component_extensions_to_load_as_string: # pylint: disable=E1101
you dont need this

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_backend.py:57: extensions = ','.join(
we should verify that the extension exists before we pass it in and raise an
exception. Either here, or in the constructor of ExtensionToLoad

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_dict_backend.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict_backend.py:90: return
self.__getitem__(crx_id.GetCRXAppID(abs_path))
No. You shouldn't do this. That assumes that all extensions are files, which is
wrong.

You should only be able to get extensions by id in this class.
ExtensionToLoad.id should return an id that you can use in here.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:7: class ExtensionPage(object):
Do we need a followup crbug for using a base class? I'm concerned that if i
rename EvaluateJavascript in one place, we forget the other places.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:12: def
WaitForDocumentReadyStateToBeInteractiveOrBetter(self,
You dont have docstrings on this. Bad. Followup bug or base class.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:17: def ExecuteJavaScript(self,
expr, timeout=DEFAULT_EXTENSION_TIMEOUT):
yeah i really would like a base class. I'm sorry.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:23: default_extensions =
['hhaomjibdihmijegdhdafkllkbggdgoj',
No!!! I explained my issue with this in a previous codereview. Do not use hard
coded extensions in tests. EVER! Telemetry should not regress when suddenly they
make wallpaper_manager_2!

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:48: extension_dirs =
[tempfile.mkdtemp() for i in range(5)]
why 5?

please, just do 2. no loops. dont copy. just make simple_extension_2 in
unittestdata

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:73: 
why not running setTestVar?

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/possible_browser.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/possible_browser.py:10: def __init__(self,
browser_type, options, supports_extensions):
please dont edit this. Its hard to read. Look at how we do is_content_shell ---
you subclass the possible browser object and override Create to pass in the
right stuff to the BrowserBackend constructor.

[nduca, 2013-02-01 23:16:42]

Hey, can you do one thing for me? Remove the is_component field from this patch
and we can do that as a followup. I want to have a discussion about component
extensions with a slightly broader audience, but that doesn't have to impact the
rest of this patch.

[achuithb, 2013-02-06 00:23:03]

Nat, please take a look.

I've also gotten rid of component extension support.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/android_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/android_browser_backend.py:24: raise Exception('Cannot
load extensions for android browser!')
On 2013/02/01 00:25:50, nduca wrote:
> Android browser does not support extensions.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:32: if not
browser_backend.is_content_shell:
On 2013/02/01 00:25:50, nduca wrote:
> android isn't content shell.... treat these separately --- ask the browser
> backend if it supports extensions

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:64: @property
On 2013/02/01 00:25:50, nduca wrote:
> you dont have a supports_extensions property. you should.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser.py:65: def extensions(self):
On 2013/02/01 00:25:50, nduca wrote:
> doc tring should say that this may not be there
> 
> accessor should raise  special exception if its not supported
> 
>    raise ExtensionsNotSupportedException) or something

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser_options.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:13: class ExtensionToLoad(object):
On 2013/02/01 00:25:50, nduca wrote:
> This should be its own py file

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:14: def __init__(self, path,
is_component):
On 2013/02/01 00:25:50, nduca wrote:
> load_as_component=False

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:16: self.is_component =
is_component
On 2013/02/01 00:25:50, nduca wrote:
> this should have an accessor for crx id

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:98:
group.add_option('--load-component-extension',
On 2013/02/01 00:25:50, nduca wrote:
> Remove all the edits here. I do not want users to be able to load extensions
> this way. self.extensions_to_load, yes. Not here.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_options.py:166: if
self.component_extensions_to_load_as_string: # pylint: disable=E1101
On 2013/02/01 00:25:50, nduca wrote:
> you dont need this

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_backend.py:57: extensions = ','.join(
On 2013/02/01 00:25:50, nduca wrote:
> we should verify that the extension exists before we pass it in and raise an
> exception. Either here, or in the constructor of ExtensionToLoad

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_dict_backend.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict_backend.py:90: return
self.__getitem__(crx_id.GetCRXAppID(abs_path))
On 2013/02/01 00:25:50, nduca wrote:
> No. You shouldn't do this. That assumes that all extensions are files, which
is
> wrong.
> 
> You should only be able to get extensions by id in this class.
> ExtensionToLoad.id should return an id that you can use in here.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_page.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:7: class ExtensionPage(object):
On 2013/02/01 00:25:50, nduca wrote:
> Do we need a followup crbug for using a base class? I'm concerned that if i
> rename EvaluateJavascript in one place, we forget the other places.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:12: def
WaitForDocumentReadyStateToBeInteractiveOrBetter(self,
On 2013/02/01 00:25:50, nduca wrote:
> You dont have docstrings on this. Bad. Followup bug or base class.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_page.py:17: def ExecuteJavaScript(self,
expr, timeout=DEFAULT_EXTENSION_TIMEOUT):
On 2013/02/01 00:25:50, nduca wrote:
> yeah i really would like a base class. I'm sorry.

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:23: default_extensions =
['hhaomjibdihmijegdhdafkllkbggdgoj',
On 2013/02/01 00:25:50, nduca wrote:
> No!!! I explained my issue with this in a previous codereview. Do not use hard
> coded extensions in tests. EVER! Telemetry should not regress when suddenly
they
> make wallpaper_manager_2!

Done.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:48: extension_dirs =
[tempfile.mkdtemp() for i in range(5)]
On 2013/02/01 00:25:50, nduca wrote:
> why 5?

You do see certain failures with 5 that you don't see with 2. For instance, if
you don't WaitFor(AllExtensionsLoaded) below, the 2 case still works, but the 5
case does not, on my z600. I've changed this to 3, but I think 2 is not as
strong a test.

> please, just do 2. no loops. dont copy. just make simple_extension_2 in
> unittestdata

How strongly do you feel about this? I think the solution here is more flexible
(in allowing you to change how many extensions you want to test), and doesn't
require a redundant extension in unittestdata.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:73: 
On 2013/02/01 00:25:50, nduca wrote:
> why not running setTestVar?

It is tested in the first test. I didn't see the point of doing it here again.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/possible_browser.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/possible_browser.py:10: def __init__(self,
browser_type, options, supports_extensions):
On 2013/02/01 00:25:50, nduca wrote:
> please dont edit this. Its hard to read. Look at how we do is_content_shell
---
> you subclass the possible browser object and override Create to pass in the
> right stuff to the BrowserBackend constructor.

I'm doing this, but that's not sufficient. PossibleBrowser needs to know if it
supports extensions in order to do the browser matching in
browser_finder.FindBrowser.

[achuithb, 2013-02-07 01:30:45]

Added PossibleBrowser.SupportsOptions as discussed over chat with Nat; got rid
of PossibleBrowser.supports_extensions.

PTAL, Nat and Dave.

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/possible_browser.py (right):

https://codereview.chromium.org/11882033/diff/57001/tools/telemetry/telemetry...
tools/telemetry/telemetry/possible_browser.py:10: def __init__(self,
browser_type, options, supports_extensions):
On 2013/02/06 00:23:03, achuith.bhandarkar wrote:
> On 2013/02/01 00:25:50, nduca wrote:
> > please dont edit this. Its hard to read. Look at how we do is_content_shell
> ---
> > you subclass the possible browser object and override Create to pass in the
> > right stuff to the BrowserBackend constructor.
> 
> I'm doing this, but that's not sufficient. PossibleBrowser needs to know if it
> supports extensions in order to do the browser matching in
> browser_finder.FindBrowser.

Ok, this is fixed now. I've introduced SupportsOptions to do this.

[nduca, 2013-02-07 02:08:33]

>> why 5?
> You do see certain failures with 5 that you don't see with 2.

Sounds like a bug in WaitForBrowserToComeUp. THe browser shouldnt' become alive
until the extensions you requseted show up in the extensions list.

> For instance, if you don't WaitFor(AllExtensionsLoaded) below, the 2 case
still works, but the 5 case does not, on my z600. I've changed this to 3, but I
think 2 is not as strong a test.

You should have interpreted this as a bug in the infrastructure, not something
to be band aided over in a test.

> please, just do 2. no loops. dont copy. just make simple_extension_2 in
> unittestdata

Since you need 5 extnesions, I udnerstand the need. Please pull out to a helper
function or clean up in some way to make readable. As written, this is very hard
to read. 

Perhaps you should have a base class called MultiExtensionTest whose setUp and
tearDown makes the browser and extensions.

Unclean test code makes for fragility down the road.

[nduca, 2013-02-07 02:21:53]

pretty good. Still not there. I'd be open to you breaking out some of the basic
extensions to load, finder, backend changes if you wanted since its the
unittests and ExtensionsDict stuff that remains mostly controversial. I could
review this faster too, if the patch was smaller. BUt, up to you.

https://codereview.chromium.org/11882033/diff/81002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser_finder.py (right):

https://codereview.chromium.org/11882033/diff/81002/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_finder.py:57: (len(options.extensions_to_load)
== 0 or b.supports_extensions)]
lets move the check to the possible browser, where the possible browser base
class returns true, and the PossibleDesktopBrowser checks.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/android_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/android_browser_backend.py:25: raise
Exception('Android browser does not support extensions.')
Why not throw the ExtensionsNotSupportedException that you made in browser?

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/android_browser_finder.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/android_browser_finder.py:64: return not
options.extensions_to_load
this would probably be cleaner written

if len(options.extensions_to_load):
  return False
return True

easier to hack on later

also, the fact that [] is falsy is, while technically correct, is a bit more
fragile than I care for

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_backend.py:31: self.supports_extensions =
supports_extensions
can you make this self._supports_extensions, then make a @property for the
public one? That way you can put """-string on it

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_browser_backend.py (left):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_browser_backend.py:16: assert not
is_content_shell
wrt this deletion and the removal of is_content_shell above, do we never plan on
supporting a cros content shell build? Does that seem like someting we should
attack in this patch? Seems like another patch for another day.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_browser_backend.py:13: super(CrOSBrowserBackend,
self).__init__(False, True, options)
lets use keyword arguments to make this clearer

__init__(is_content_shell=False,
 is_blah=True,
 options=options)


here and on the other backends

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_backend.py:58: extensions = ','.join(
how come this part is on the desktop backend and not on the base class'
GetBrowserStartupArgs? Its valid on cros too, no?

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_finder.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_finder.py:44: return not
options.extensions_to_load or not self._is_content_shell
rewrite in the same format as I did before...
if len(extnesions) and is_conent_shell:
  return false
return true

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_dict.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:5: class ExtensionDict(object):
doc strings

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:5: class ExtensionDict(object):
doc strings for class and all methods, explaining what type it returns, what the
key is.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:16: return
self._extension_dict_backend.__getitem__(extension_id)
Hmmm, maybe this this key should always require the ExtensionToLoad? I'm betting
a lot of folks will make the same mistake you made earlier and hard code
extension IDs. I'm not a fan of that.

Should you be allowed to talk to extensiosn that you didn't load? I'm
thinking.... no.

[achuithb, 2013-02-08 09:51:52]

https://codereview.chromium.org/11882033/diff/81002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser_finder.py (right):

https://codereview.chromium.org/11882033/diff/81002/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_finder.py:57: (len(options.extensions_to_load)
== 0 or b.supports_extensions)]
On 2013/02/07 02:21:54, nduca wrote:
> lets move the check to the possible browser, where the possible browser base
> class returns true, and the PossibleDesktopBrowser checks.

This is already gone. Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/android_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/android_browser_backend.py:25: raise
Exception('Android browser does not support extensions.')
On 2013/02/07 02:21:54, nduca wrote:
> Why not throw the ExtensionsNotSupportedException that you made in browser?

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/android_browser_finder.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/android_browser_finder.py:64: return not
options.extensions_to_load
On 2013/02/07 02:21:54, nduca wrote:
> this would probably be cleaner written
> 
> if len(options.extensions_to_load):
>   return False
> return True
> 
> easier to hack on later
> 
> also, the fact that [] is falsy is, while technically correct, is a bit more
> fragile than I care for

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/browser_backend.py:31: self.supports_extensions =
supports_extensions
On 2013/02/07 02:21:54, nduca wrote:
> can you make this self._supports_extensions, then make a @property for the
> public one? That way you can put """-string on it

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_browser_backend.py (left):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_browser_backend.py:16: assert not
is_content_shell
On 2013/02/07 02:21:54, nduca wrote:
> wrt this deletion and the removal of is_content_shell above, do we never plan
on
> supporting a cros content shell build? Does that seem like someting we should
> attack in this patch? Seems like another patch for another day.

Does sound like another patch for another day.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_browser_backend.py:13: super(CrOSBrowserBackend,
self).__init__(False, True, options)
On 2013/02/07 02:21:54, nduca wrote:
> lets use keyword arguments to make this clearer
> 
> __init__(is_content_shell=False,
>  is_blah=True,
>  options=options)
> 
> 
> here and on the other backends

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_backend.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_backend.py:58: extensions = ','.join(
On 2013/02/07 02:21:54, nduca wrote:
> how come this part is on the desktop backend and not on the base class'
> GetBrowserStartupArgs? Its valid on cros too, no?

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_finder.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_finder.py:44: return not
options.extensions_to_load or not self._is_content_shell
On 2013/02/07 02:21:54, nduca wrote:
> rewrite in the same format as I did before...
> if len(extnesions) and is_conent_shell:
>   return false
> return true

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_dict.py (right):

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:5: class ExtensionDict(object):
On 2013/02/07 02:21:54, nduca wrote:
> doc strings

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:5: class ExtensionDict(object):
On 2013/02/07 02:21:54, nduca wrote:
> doc strings for class and all methods, explaining what type it returns, what
the
> key is.

Done.

https://codereview.chromium.org/11882033/diff/81005/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:16: return
self._extension_dict_backend.__getitem__(extension_id)
On 2013/02/07 02:21:54, nduca wrote:
> Hmmm, maybe this this key should always require the ExtensionToLoad? I'm
betting
> a lot of folks will make the same mistake you made earlier and hard code
> extension IDs. I'm not a fan of that.
> 
> Should you be allowed to talk to extensiosn that you didn't load? I'm
> thinking.... no.

ExtensionDictBackend has no access to ExtensionToLoad, so it must still use the
id as its key. The iterator here returns ids, and cannot return ExtensionToLoad
instances. We still want to be able to iterate through all loaded extensions,
right?

[achuithb, 2013-02-08 09:53:12]

On 2013/02/07 02:08:33, nduca wrote:
> >> why 5?
> > You do see certain failures with 5 that you don't see with 2.
> 
> Sounds like a bug in WaitForBrowserToComeUp. THe browser shouldnt' become
alive
> until the extensions you requseted show up in the extensions list.
> 
> > For instance, if you don't WaitFor(AllExtensionsLoaded) below, the 2 case
> still works, but the 5 case does not, on my z600. I've changed this to 3, but
I
> think 2 is not as strong a test.
> 
> You should have interpreted this as a bug in the infrastructure, not something
> to be band aided over in a test.
> 
> > please, just do 2. no loops. dont copy. just make simple_extension_2 in
> > unittestdata
> 
> Since you need 5 extnesions, I udnerstand the need. Please pull out to a
helper
> function or clean up in some way to make readable. As written, this is very
hard
> to read. 
> 
> Perhaps you should have a base class called MultiExtensionTest whose setUp and
> tearDown makes the browser and extensions.
> 
> Unclean test code makes for fragility down the road.

I've added MultipleExtensionTest as you suggest, with the directory copying
stuff in setUp, and added a TODO for WaitForBrowserToComeUp.

[achuithb, 2013-02-08 10:04:08]

On 2013/02/07 02:21:53, nduca wrote:
> pretty good. Still not there. I'd be open to you breaking out some of the
basic
> extensions to load, finder, backend changes if you wanted since its the
> unittests and ExtensionsDict stuff that remains mostly controversial. I could
> review this faster too, if the patch was smaller. BUt, up to you.
> 

The browser_backend changes depend on ExtensionDict, so I'd rather have this
stay in one patch.

[achuithb, 2013-02-08 10:04:24]

PTAL, Nat and Dave.

[nduca, 2013-02-12 20:12:33]

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_dict.py (right):

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:11: def __iter__(self):
dont add this yet.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:15: def __len__(self):
i'd also remove this one

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:23: 
i'd add __contains__  so you can say "e_to_load in browser.extensions"

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_to_load.py (right):

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_to_load.py:17: def Id(self):
Id -> id

properties get the naming convention of member variables

[nduca, 2013-02-12 20:18:16]

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:24: if browser_to_create:
# comment explaining why you return
if not browser_to_create:
  return

rest unindented

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:38: self._extension_dirs =
[tempfile.mkdtemp() for i in range(3)]
you need to clean up as well.


rule of thumb:
  if you have a setUp, then you ought to have a tear down


shutil.rmtree should help

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:53: if browser_to_create:
same edit as above

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:55: def AllExtensionsLoaded():
seems like this should be done by setUp. The only thing this test should be
doing is testing len and getitem. It should be at most two or three lines log.
setUp should be creating the extension copiies, creating the browser, getitng
the test going


if not self.browser:
  # test only works in browsers that support extensions
  return

for ext in this.loaded_extensions:
  self.assertEquals(2, b.extensions[ext].EvaluateJavascript("1+1"))

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:69:
self.assertTrue(b.extensions[extension_ids[i]])
this test is meaningless since you've loaded before. I think its better to test
the same thing you test before, e.g. doing the testVar.

[achuithb, 2013-02-12 22:42:54]

Apologies for the botched rebase.

The only files that have changed are extension_dict.py
extension_dict_backend.py, extensions_to_load.py and extension_unittest.py.

PTAL, Nat!

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_dict.py (right):

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:11: def __iter__(self):
On 2013/02/12 20:12:33, nduca wrote:
> dont add this yet.

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:15: def __len__(self):
On 2013/02/12 20:12:33, nduca wrote:
> i'd also remove this one

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_dict.py:23: 
On 2013/02/12 20:12:33, nduca wrote:
> i'd add __contains__  so you can say "e_to_load in browser.extensions"

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_to_load.py (right):

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_to_load.py:17: def Id(self):
On 2013/02/12 20:12:33, nduca wrote:
> Id -> id
> 
> properties get the naming convention of member variables 

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:24: if browser_to_create:
On 2013/02/12 20:18:17, nduca wrote:
> # comment explaining why you return
> if not browser_to_create:
>   return
> 
> rest unindented

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:38: self._extension_dirs =
[tempfile.mkdtemp() for i in range(3)]
On 2013/02/12 20:18:17, nduca wrote:
> you need to clean up as well.
> 
> 
> rule of thumb:
>   if you have a setUp, then you ought to have a tear down
> 
> 
> shutil.rmtree should help

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:53: if browser_to_create:
On 2013/02/12 20:18:17, nduca wrote:
> same edit as above

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:55: def AllExtensionsLoaded():
On 2013/02/12 20:18:17, nduca wrote:
> seems like this should be done by setUp. The only thing this test should be
> doing is testing len and getitem. It should be at most two or three lines log.
> setUp should be creating the extension copiies, creating the browser, getitng
> the test going
> 
> 
> if not self.browser:
>   # test only works in browsers that support extensions
>   return
> 
> for ext in this.loaded_extensions:
>   self.assertEquals(2, b.extensions[ext].EvaluateJavascript("1+1"))

Done.

https://codereview.chromium.org/11882033/diff/80007/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:69:
self.assertTrue(b.extensions[extension_ids[i]])
On 2013/02/12 20:18:17, nduca wrote:
> this test is meaningless since you've loaded before. I think its better to
test
> the same thing you test before, e.g. doing the testVar.

Done.

[nduca, 2013-02-12 23:34:05]

lgtm but please ensure the tests actually run, and pass.

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:29: # doesn't wait for
extensions to finish loading."""
does this even run?

you have """ here that is commenting out the entire file up to line 39

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:33:
extension.WaitForDocumentReadyStateToBeInteractiveOrBetter()
remove this line

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:83:
extension.WaitForDocumentReadyStateToBeInteractiveOrBetter()
remove this line

[achuithb, 2013-02-12 23:36:29]

Thanks for the review!

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:29: # doesn't wait for
extensions to finish loading."""
On 2013/02/12 23:34:05, nduca wrote:
> does this even run?
> 
> you have """ here that is commenting out the entire file up to line 39

It runs because the # comments out the """.

Done. Gone.

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:33:
extension.WaitForDocumentReadyStateToBeInteractiveOrBetter()
On 2013/02/12 23:34:05, nduca wrote:
> remove this line

Done.

https://codereview.chromium.org/11882033/diff/83011/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:83:
extension.WaitForDocumentReadyStateToBeInteractiveOrBetter()
On 2013/02/12 23:34:05, nduca wrote:
> remove this line

Done.

[dtu, 2013-02-13 00:11:22]

https://chromiumcodereview.appspot.com/11882033/diff/83011/tools/telemetry/te...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://chromiumcodereview.appspot.com/11882033/diff/83011/tools/telemetry/te...
tools/telemetry/telemetry/extension_unittest.py:33:
extension.WaitForDocumentReadyStateToBeInteractiveOrBetter()
On 2013/02/12 23:36:29, achuith.bhandarkar wrote:
> On 2013/02/12 23:34:05, nduca wrote:
> > remove this line
> 
> Done.

Why not just move this stuff to WaitForBrowserToComeUp then? So this unit test
doesn't have to worry about it.

[achuithb, 2013-02-13 09:10:21]

You can diff against patchset 41 to see AllExtensionsLoaded moved to
BrowserBackend._WaitForBrowserToComeUp. This resolves the unit test TODOs.

PTAL, Dave and Nat.

https://chromiumcodereview.appspot.com/11882033/diff/83011/tools/telemetry/te...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://chromiumcodereview.appspot.com/11882033/diff/83011/tools/telemetry/te...
tools/telemetry/telemetry/extension_unittest.py:33:
extension.WaitForDocumentReadyStateToBeInteractiveOrBetter()
On 2013/02/13 00:11:22, Dave Tu wrote:
> On 2013/02/12 23:36:29, achuith.bhandarkar wrote:
> > On 2013/02/12 23:34:05, nduca wrote:
> > > remove this line
> > 
> > Done.
> 
> Why not just move this stuff to WaitForBrowserToComeUp then? So this unit test
> doesn't have to worry about it.

Done.

[nduca, 2013-02-13 09:16:15]

lgtm nice work :)

[achuithb, 2013-02-13 09:50:19]

On 2013/02/13 09:16:15, nduca wrote:
> lgtm nice work :)

Thank you! Going to wait for Dave. Have also updated the description of this CL.

[achuithb, 2013-02-13 10:13:51]

https://codereview.chromium.org/11882033/diff/92004/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/92004/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:36: self._extension_dirs =
[tempfile.mkdtemp() for i in range(3)]
I get a pylint error here (Unused variable 'i'). I don't know how to do this
without using i? Any ideas?

[nduca, 2013-02-13 10:20:40]

you can # pylint: disable=errno

there're examples int he code for this

[achuithb, 2013-02-13 10:29:19]

On 2013/02/13 10:20:40, nduca wrote:
> you can # pylint: disable=errno
> 
> there're examples int he code for this

That worked. Thank you!

[achuithb, 2013-02-13 10:49:39]

Added the test for Disconnect as discussed elsewhere.

[nduca, 2013-02-13 10:55:48]

https://codereview.chromium.org/11882033/diff/96004/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/96004/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:32: extension.Disconnect()
Nooo, this is a separate test. you have testExtension. That tests one thing.
This is another test.

if you did the setUp tearDown thing here, then this would beens impler.

[achuithb, 2013-02-13 11:13:48]

https://codereview.chromium.org/11882033/diff/96004/tools/telemetry/telemetry...
File tools/telemetry/telemetry/extension_unittest.py (right):

https://codereview.chromium.org/11882033/diff/96004/tools/telemetry/telemetry...
tools/telemetry/telemetry/extension_unittest.py:32: extension.Disconnect()
On 2013/02/13 10:55:48, nduca wrote:
> Nooo, this is a separate test. you have testExtension. That tests one thing.
> This is another test.
> 
> if you did the setUp tearDown thing here, then this would beens impler.

Ok, I'm going to revert this tomorrow. I'll do the setUp tearDown thing so this
is easier, but in another patch.

[dtu, 2013-02-13 19:38:25]

lgtm