DevTools: extract DevToolsWindowBase, leave docking, unload and factory logic in DevToolsWindow.

DevTools: extract DevToolsWindowBase, leave docking, unload and factory logic in DevToolsWindow.

R=dgozman@chromium.org, vsevik@chromium.org
TBR=jam for chrome/chrome.gyp

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

[pfeldman, 2014-04-29 07:46:01]

This is yet crashing on start, so I'm debugging it. But I don't expect it to
change much.

[pfeldman, 2014-04-29 10:33:44]

PTAL!

[Vladislav Kaznacheev, 2014-04-29 11:10:05]

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (left):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:878: if (base_url.SchemeIs("data"))
Why remove this? This has been added in https://codereview.chromium.org/27693002
for a reason.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:733: WebContents* inspected_tab =
this->GetInspectedWebContents();
How does explicit 'this' help us here?

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window_base.h (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window_base.h:34: // Base implementation of
DevTools findings around front-end.
Findings? Cannot really parse the comment.

[pfeldman, 2014-04-29 11:28:10]

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (left):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:878: if (base_url.SchemeIs("data"))
It is good we have you remembering it. How about a test for it? I am sure I'll
remove it with the next refactoring. Added comment.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:733: WebContents* inspected_tab =
this->GetInspectedWebContents();
On 2014/04/29 11:10:05, Vladislav Kaznacheev wrote:
> How does explicit 'this' help us here?

This code just moved - for some reason it was defined among dispatchers.

Done.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window_base.h (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window_base.h:34: // Base implementation of
DevTools findings around front-end.
Done. bindings :)

[dgozman, 2014-04-29 11:32:26]

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:819: // Do not navigate back in
history on Windows (http://crbug.com/74156).
I'm not sure whether this belongs to base.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:856: bool
DevToolsWindow::PreHandleGestureEvent(
This is DevToolsWindowBase functionality.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:865: void
DevToolsWindow::ActivateWindow() {
Having this functionality for in-tab DevTools would be nice too.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window_base.cc (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window_base.cc:345: void
DevToolsWindowBase::SetIsDocked(bool dock_requested) {
I don't like that this class even knows about such methods. Can we split
frontend host dispatching?

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window_base.cc:351: void
DevToolsWindowBase::OpenInNewTab(const std::string& url) {
This method definitely asks for aggregation instead of subclassing.

[pfeldman, 2014-04-29 11:37:12]

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:819: // Do not navigate back in
history on Windows (http://crbug.com/74156).
That's why it is not there.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:856: bool
DevToolsWindow::PreHandleGestureEvent(
On 2014/04/29 11:32:26, dgozman wrote:
> This is DevToolsWindowBase functionality.

It can't be - DevToolsWindowBase is not a WebContentsDelegate, it is displayed
in one.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:865: void
DevToolsWindow::ActivateWindow() {
That's why it is implemented there.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window_base.cc (right):

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window_base.cc:345: void
DevToolsWindowBase::SetIsDocked(bool dock_requested) {
On 2014/04/29 11:32:26, dgozman wrote:
> I don't like that this class even knows about such methods. Can we split
> frontend host dispatching?

Not really, no. As long as front-end is sending this to the embedder, we need to
handle it here.

https://codereview.chromium.org/253823005/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window_base.cc:351: void
DevToolsWindowBase::OpenInNewTab(const std::string& url) {
On 2014/04/29 11:32:26, dgozman wrote:
> This method definitely asks for aggregation instead of subclassing.

Not sure I follow your suggestion. Alternatives look heavy to me.

[vsevik, 2014-04-29 12:20:05]

https://codereview.chromium.org/253823005/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/253823005/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:819: // Do not navigate back in
history on Windows (http://crbug.com/74156).
Seems to be yet another reason to have an option to override default
WebContentsDelegate behaviour in undocked/tab mpdes. :(

I believe this particular issue could be fixed in front-end though.

[vsevik, 2014-04-29 12:20:20]

lgtm

[pfeldman, 2014-04-29 12:33:15]

Alright, lets land this as is since it has no change in behavior. Then I'll
follow up with removal of pinch-disabler and will consume backspace in
front-end.

[pfeldman, 2014-04-29 12:33:19]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/253823005/40001

[dgozman, 2014-04-29 12:34:59]

lgtm

Although I'd rather we don't use subclassing to avoid nasty methods with
different implementations, which may lead to bugs later.

[commit-bot: I haz the power, 2014-04-29 12:35:30]

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

[commit-bot: I haz the power, 2014-04-29 12:35:30]

Try jobs failed on following builders:
  linux_chromium_clang_dbg on tryserver.chromium

[pfeldman, 2014-04-29 12:40:52]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:41:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/253823005/60001

[commit-bot: I haz the power, 2014-04-29 12:54:34]

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

[commit-bot: I haz the power, 2014-04-29 12:54:34]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[pfeldman, 2014-04-29 14:09:30]

Committed patchset #3 manually as r266873 (presubmit successful).