telemetry: Add a method to access tabs by key in TabList.

telemetry: Add a method to access tabs by key in TabList.

Previously, the only public method to access tabs from TabList was by index
into an array. The problem with this method is that the array is dynamic, so
there's always the possibility of a race condition causing the index
dereference to explode.

The plan is to migrate the interface entirely over to accessing tabs by key.
For now, add a public method that allows access to tabs by key. Also, make tabs
return the same key as their id property.

BUG=398467, 442546
Committed: https://crrev.com/85a6b66385c44f257355a9829cb12805dd81e909
Cr-Commit-Position: refs/heads/master@{#316530}

[erikchen, 2015-02-13 04:12:56]

erikchen@chromium.org changed reviewers:
+ nednguyen@google.com

[erikchen, 2015-02-13 04:12:56]

nednguyen: Please review.

This seemed like the right approach given your previous comments on the bug. Let
me know if you want to discuss different approaches.

[nednguyen, 2015-02-13 16:46:06]

web_content.WebContent API has an id field, you can modify the implementation so
that id return the context id.

https://codereview.chromium.org/915173012/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/tab_list.py (right):

https://codereview.chromium.org/915173012/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/tab_list.py:20: def GetTabFromContextId(self,
context_id):
GetTabById(...)

ContextId is an implementation detail of chrome browser. We want the tab API to
be as generic as possible.

[erikchen, 2015-02-13 23:16:08]

Patchset #2 (id:20001) has been deleted

[erikchen, 2015-02-13 23:16:13]

Patchset #2 (id:40001) has been deleted

[erikchen, 2015-02-13 23:17:15]

nednguyen: PTAL

https://codereview.chromium.org/915173012/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/tab_list.py (right):

https://codereview.chromium.org/915173012/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/tab_list.py:20: def GetTabFromContextId(self,
context_id):
On 2015/02/13 16:46:06, nednguyen wrote:
> GetTabById(...)
> 
> ContextId is an implementation detail of chrome browser. We want the tab API
to
> be as generic as possible.

Done.

[nednguyen, 2015-02-13 23:27:21]

lgtm with nits

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend_list.py
(right):

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/inspector_backend_list.py:70:
def GetTabById(self, identifier):
ditto

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/tab_list.py (right):

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/tab_list.py:20: def GetTabById(self, identifier):
s/identifier/id

[nednguyen, 2015-02-13 23:30:28]

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/tab_list.py (right):

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/tab_list.py:20: def GetTabById(self, identifier):
On 2015/02/13 23:27:21, nednguyen wrote:
> s/identifier/id

Also maybe add documentation to mention that this id is the same as tab.id

[erikchen, 2015-02-13 23:37:01]

New patchsets have been uploaded after l-g-t-m from nednguyen@google.com

[erikchen, 2015-02-13 23:37:32]

nednguyen: PTAL

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/tab_list.py (right):

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/tab_list.py:20: def GetTabById(self, identifier):
On 2015/02/13 23:27:21, nednguyen wrote:
> s/identifier/id

id is the name of a python built-in function, which gives a pylint warning.

https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/tab_list.py:20: def GetTabById(self, identifier):
On 2015/02/13 23:30:28, nednguyen wrote:
> On 2015/02/13 23:27:21, nednguyen wrote:
> > s/identifier/id
> 
> Also maybe add documentation to mention that this id is the same as tab.id

Done.

[nednguyen, 2015-02-13 23:41:17]

On 2015/02/13 23:37:32, erikchen wrote:
> nednguyen: PTAL
> 
>
https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/tab_list.py (right):
> 
>
https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/tab_list.py:20: def GetTabById(self,
identifier):
> On 2015/02/13 23:27:21, nednguyen wrote:
> > s/identifier/id
> 
> id is the name of a python built-in function, which gives a pylint warning.
Ah, bummer!
> 
>
https://codereview.chromium.org/915173012/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/tab_list.py:20: def GetTabById(self,
identifier):
> On 2015/02/13 23:30:28, nednguyen wrote:
> > On 2015/02/13 23:27:21, nednguyen wrote:
> > > s/identifier/id
> > 
> > Also maybe add documentation to mention that this id is the same as tab.id
> 
> Done.

lgtm again

[erikchen, 2015-02-13 23:53:41]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-02-13 23:54:10]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-02-13 23:54:50]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-02-13 23:56:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/915173012/80001

[commit-bot: I haz the power, 2015-02-14 00:58:30]

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

[commit-bot: I haz the power, 2015-02-14 00:58:30]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nednguyen, 2015-02-14 01:35:43]

Err, I forgot. Can you add unittest for the new method? You can either do it in
this patch or in a subsequent patch.

[erikchen, 2015-02-17 04:44:28]

On 2015/02/14 01:35:43, nednguyen wrote:
> Err, I forgot. Can you add unittest for the new method? You can either do it
in
> this patch or in a subsequent patch.

The method I added is an accessor into an existing data structure. A unit test
for it would just be a change detector test, which is counter productive. Let me
know if you disagree, and we can discuss further.

[erikchen, 2015-02-17 04:44:34]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-02-17 04:44:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/915173012/80001

[nednguyen, 2015-02-17 04:47:00]

On 2015/02/17 04:44:28, erikchen wrote:
> On 2015/02/14 01:35:43, nednguyen wrote:
> > Err, I forgot. Can you add unittest for the new method? You can either do it
> in
> > this patch or in a subsequent patch.
> 
> The method I added is an accessor into an existing data structure. A unit test
> for it would just be a change detector test, which is counter productive. Let
me
> know if you disagree, and we can discuss further.

I disagree. If the implementation change so that the tab's id is different from
the id used for indexing the tab, or the tab's id is no longer stable, we need
to unittest to catch it. If the test can detect behavior change instead of
interface change, I don't see what is the problem.

[commit-bot: I haz the power, 2015-02-17 05:09:20]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-02-17 05:10:09]

Patchset 3 (id:??) landed as
https://crrev.com/85a6b66385c44f257355a9829cb12805dd81e909
Cr-Commit-Position: refs/heads/master@{#316530}