Fix navigation of chrome://oobe/login.

Fix navigation of chrome://oobe/login.

* Introduce MiscWebContentsBackend for chrome://oobe/login.
* Fix navigation of chrome://oobe/login by using misc_web_contents_backend instead of tab_list_backend.

BUG=228970
TEST=unit tests.

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

[achuithb, 2013-04-09 00:14:05]

Please take a look, Dave and Nat. Most of it is just refactoring common code
from ExtensionDictBackend and TabListBackend into WebContentsBackend. Let me
know if you have any questions.

[dtu, 2013-04-09 22:41:54]

https://codereview.chromium.org/13836002/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/chrome/web_contents_backend.py (right):

https://codereview.chromium.org/13836002/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/chrome/web_contents_backend.py:14: class
WebContentsBackend(object):
It's not the backend for WebContents in the same way that BrowserBackend is the
backend for Browser. WebContentsDictBackend?

https://codereview.chromium.org/13836002/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/chrome/web_contents_backend.py:19:
self._web_contents_list = []
None of this stuff (storing the list of webcontents, accessing the items like a
list) is needed for chrome://oobe or extension_dict_backend, right? I think this
class should have only the minimal amount of code that's shared between all the
classes.

[achuithb, 2013-04-09 22:50:12]

https://codereview.chromium.org/13836002/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/chrome/web_contents_backend.py (right):

https://codereview.chromium.org/13836002/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/chrome/web_contents_backend.py:14: class
WebContentsBackend(object):
On 2013/04/09 22:41:54, Dave Tu wrote:
> It's not the backend for WebContents in the same way that BrowserBackend is
the
> backend for Browser. WebContentsDictBackend?

WebContentsListBackend? Since the primary access is through Get + index?

https://codereview.chromium.org/13836002/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/chrome/web_contents_backend.py:19:
self._web_contents_list = []
On 2013/04/09 22:41:54, Dave Tu wrote:
> None of this stuff (storing the list of webcontents, accessing the items like
a
> list) is needed for chrome://oobe or extension_dict_backend, right? I think
this
> class should have only the minimal amount of code that's shared between all
the
> classes.

Line 10 here:
https://codereview.chromium.org/13866022/diff/1001/tools/telemetry/telemetry/...

I don't see how we can avoid having an iterator of some kind to find the
web_contents we care about. For extensions, we could access by id, but for
tabs/web_contents, I think we need Get.

[achuithb, 2013-04-10 18:58:10]

Any luck with this CL?

[dtu, 2013-04-10 22:42:46]

Sorry, I forgot to publish one of my comments.

One particular reason is that the list caching code is not great, and is mostly
there to work around a limitation in DevTools in older versions of Chrome. It's
fairly complex and prone to bugs, especially the WeakValueDictionary behavior.
Fetching the list of tabs every time you want to look something up is much
simpler.

The other thing is I think the WebContents thing and the ExtensionDict should
have semantics like a Python dict. They are unordered collections of objects
that can be looked up by an ID, and are iterable. (Python dicts are iterable,
but not indexable.)

Tabs have a specific order to them (the display order), but it doesn't make
sense to look them up by any ID, so they have semantics like a Python list. They
are also iterable, so the iterator could go in the base WebContentsDict class.

[dtu, 2013-04-10 22:43:52]

If it has dict semantics, you can do that Get() check for chrome://oobe using a
for loop, or something like
http://stackoverflow.com/questions/3097866/python-access-to-first-element-in-...

[nduca, 2013-04-11 16:52:17]

This feels a bit complicated to me. Whats the "value" of the common code we're
trying to reuse?

[achuithb, 2013-04-11 18:45:30]

On 2013/04/11 16:52:17, nduca wrote:
> This feels a bit complicated to me. Whats the "value" of the common code we're
> trying to reuse?

I need to access chrome://oobe which is a WebContents, but is neither a Tab nor
an Extension. It used to be a Tab, but this was recently fixed in chrome.

[nduca, 2013-04-11 18:48:29]

How about making TabListBackend have a secondary list of privateTabs? Then,
since cros browser backend itself has access to the tab list backend, it can get
to the privateTabs?

[achuithb, 2013-04-11 18:53:25]

On 2013/04/11 18:48:29, nduca wrote:
> How about making TabListBackend have a secondary list of privateTabs? Then,
> since cros browser backend itself has access to the tab list backend, it can
get
> to the privateTabs?

These are not really tabs though, so it's a bit ugly to do it this way. 

We already have a WebContents base class for Tabs, Extensions and everything
else that's neither of these. What's the objection to the WebContentsDictBackend
base class? Complexity?

[nduca, 2013-04-11 18:58:48]

Complexity.

[achuithb, 2013-04-11 19:13:25]

On 2013/04/11 18:58:48, nduca wrote:
> Complexity.

Dave: Can you weigh in?

I prefer the WebContentsDictBackend approach since there is common code in
TabListBackend and ExtensionDictBackend, and this is approach seems more correct
architecturally. I agree it's more complex, but having a private list of
WebContents jammed into TabListBackend seems hacky.

That said, I'll implement whatever the consensus is between Dave and Nat.

[dtu, 2013-04-11 20:25:22]

On 2013/04/11 19:13:25, achuith.bhandarkar wrote:
> On 2013/04/11 18:58:48, nduca wrote:
> > Complexity.
> 
> Dave: Can you weigh in?
> 
> I prefer the WebContentsDictBackend approach since there is common code in
> TabListBackend and ExtensionDictBackend, and this is approach seems more
correct
> architecturally. I agree it's more complex, but having a private list of
> WebContents jammed into TabListBackend seems hacky.
> 
> That said, I'll implement whatever the consensus is between Dave and Nat.

If we're maintaining the distinction between tabs and extensions, we need to
ensure that "type: other" are not tabs as well, so they don't belong under
privateTabs.

I think there is some common code, but not that much. "type: other" don't need
to be retrieved by list index, since their order is arbitrary.

[achuithb, 2013-04-11 20:35:13]

On 2013/04/11 20:25:22, Dave Tu wrote:
> On 2013/04/11 19:13:25, achuith.bhandarkar wrote:
> > On 2013/04/11 18:58:48, nduca wrote:
> > > Complexity.
> > 
> > Dave: Can you weigh in?
> > 
> > I prefer the WebContentsDictBackend approach since there is common code in
> > TabListBackend and ExtensionDictBackend, and this is approach seems more
> correct
> > architecturally. I agree it's more complex, but having a private list of
> > WebContents jammed into TabListBackend seems hacky.
> > 
> > That said, I'll implement whatever the consensus is between Dave and Nat.
> 
> If we're maintaining the distinction between tabs and extensions, we need to
> ensure that "type: other" are not tabs as well, so they don't belong under
> privateTabs.
> 
> I think there is some common code, but not that much. "type: other" don't need
> to be retrieved by list index, since their order is arbitrary.

I'm going to upload another CL with Dave's suggestions and hopefully the shared
code/complexity will be tolerable.

[achuithb, 2013-04-11 23:35:40]

I've done another round of re-factoring, which I believe clarifies the
separation of responsibilities between the base class WebContentsDictBackend,
which now owns the _web_contents_dict and the methods that manipulate this. 

ExtensionDictBackend simply uses the base class's map (converting extension-ids
to debugger-urls), and TabListBackend keeps an additional ordered tab list. 

PTAL, Dave and Nat.

[nduca, 2013-04-12 18:51:17]

This is still super hard to grok.

Can we just try special casing oobe somehow?

[achuithb, 2013-04-15 22:40:57]

On 2013/04/12 18:51:17, nduca wrote:
> This is still super hard to grok.
> 
> Can we just try special casing oobe somehow?

Ok, I've removed the changes from tab_list_backend and extension_dict_backend.

PTAL, Nat and Dave.

[nduca, 2013-04-16 19:42:38]

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/browser_backend.py (right):

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/browser_backend.py:55:
self._web_contents_dict_backend = (
How about misc_web_contents_dict  and drop the backend suffix on this and the
class?

MiscWebContentsDict

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/browser_backend.py:77: def
web_contents_dict_backend(self):
docstring to explain what the heck it is and contains

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/browser_backend.py:137:
extension_object.WaitForDocumentReadyStateToBeComplete()
why this edit? would be better to have that as a separate cl no matter what your
rationale, its a change in behavior rather than the core feature addition

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/inspector_backend.py (right):

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/inspector_backend.py:68: return
self._browser_backend.web_contents_dict_backend.GetWebContentsUrl(
I'm confused, why this edit?

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/web_contents_dict_backend.py (right):

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/web_contents_dict_backend.py:15: def
__init__(self, browser_backend):
so this thing bugs me because its listing **EVERYTHIGN** and that feels wrong

How about making this
class MiscWebContentsBackend(object)

   def oobe_tab():
      ... logic to find the oobe or return none

   no other public methods


And PLEASE docstring on the class or comments to explain its role

[achuithb, 2013-04-17 22:54:27]

PTAL Nat and Dave.

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/browser_backend.py (right):

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/browser_backend.py:55:
self._web_contents_dict_backend = (
On 2013/04/16 19:42:39, nduca wrote:
> How about misc_web_contents_dict  and drop the backend suffix on this and the
> class?
> 
> MiscWebContentsDict

This is not consistent with TabListBackend and ExtensionDictBackend. You also
suggest MiscWebContentsBackend later on which is what I'm going with, because
this is still a backend, and this is not a dict.

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/browser_backend.py:77: def
web_contents_dict_backend(self):
On 2013/04/16 19:42:39, nduca wrote:
> docstring to explain what the heck it is and contains

Done.

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/browser_backend.py:137:
extension_object.WaitForDocumentReadyStateToBeComplete()
On 2013/04/16 19:42:39, nduca wrote:
> why this edit? would be better to have that as a separate cl no matter what
your
> rationale, its a change in behavior rather than the core feature addition

Reverted.

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/inspector_backend.py (right):

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/inspector_backend.py:68: return
self._browser_backend.web_contents_dict_backend.GetWebContentsUrl(
On 2013/04/16 19:42:39, nduca wrote:
> I'm confused, why this edit?

Reverted.

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/web_contents_dict_backend.py (right):

https://codereview.chromium.org/13836002/diff/36007/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/web_contents_dict_backend.py:15: def
__init__(self, browser_backend):
On 2013/04/16 19:42:39, nduca wrote:
> so this thing bugs me because its listing **EVERYTHIGN** and that feels wrong
> 
> How about making this
> class MiscWebContentsBackend(object)
> 
>    def oobe_tab():
>       ... logic to find the oobe or return none
> 
>    no other public methods
> 
> 
> And PLEASE docstring on the class or comments to explain its role

Done.

[dtu, 2013-04-18 02:36:09]

I'm okay with this. lgtm. Nat?

[nduca, 2013-04-18 17:08:02]

lgtm but just remove the websocket stuff. do that as another patch. dont slip
stuff like that in patches, yo.

https://codereview.chromium.org/13836002/diff/26002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/cros_util.py (right):

https://codereview.chromium.org/13836002/diff/26002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/cros_util.py:22: except
(exceptions.TabCrashException, websocket.WebSocketException):
why on earth would evaluatejs raise websocket exception? If it is, then its a
bug in the backend. WSE should never be visible to users.

[achuithb, 2013-04-18 19:01:25]

Reverted the web_socket change. It'll go into another patch.

Thanks for the reviews!

https://codereview.chromium.org/13836002/diff/26002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/cros_util.py (right):

https://codereview.chromium.org/13836002/diff/26002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/cros_util.py:22: except
(exceptions.TabCrashException, websocket.WebSocketException):
On 2013/04/18 17:08:02, nduca wrote:
> why on earth would evaluatejs raise websocket exception? If it is, then its a
> bug in the backend. WSE should never be visible to users.

Yup, this needed to be fixed elsewhere. Done.

[nduca, 2013-04-18 19:35:35]

ship it. :) lgtm

[achuithb, 2013-04-18 20:20:56]

Committed patchset #17 manually as r194992 (presubmit successful).