[DevTools] Remove dock side knowledge from browser.

[DevTools] Remove dock side knowledge from browser.

After this change, DevToolsWindow waits for frontend to decide
on dock side, and only then shows it either docked or undocked.

This also cleans up DevToolsWindow public API, so clients don't use
test function, and test clients are explicit with their docking expectations.

Note: follow up patch on frontend side is require to switch
from SetDockSide to SetIsDocked call.

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

[dgozman, 2013-12-27 14:27:55]

Pavel,

Take a look at this change on Chrome side. It works without Blink-side right
now. Previous releases support will follow.

Thanks,
Dmitry

[dgozman, 2013-12-27 14:29:30]

The load process is: Create -> ScheduleShow -> OnDocumentLoad -> SetDockSide ->
Show -> DoAction.

[dgozman, 2013-12-30 13:06:15]

This looks like something we can land. I've tested with M34 and M31 frontends.

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

https://codereview.chromium.org/108213012/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:612: load_state_mask_(can_dock ?
kNotLoaded : kIsDockedSet),
This initialization allows old frontends to work as is. We don't wait for
docking call, but instead immediately show undocked.

Passing (temporarily) "dockSide=undocked" fixes any UI problems in old
frontends.

Also, since can_dock is false, we ignore any docking calls from remote
frontends, so they cannot switch from undocked.

[pfeldman, 2014-01-09 12:52:01]

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_embedder_message_dispatcher.h (right):

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_embedder_message_dispatcher.h:34: virtual void
SetDockSide(const std::string& side) = 0;
Please comment as to-be-removed.

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

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:488: if (action.type() ==
DevToolsToggleAction::kNothing)
kNoop

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:610: load_state_mask_(can_dock ?
kNotLoaded : kIsDockedSet),
Let add a comment here about supporting for the legacy front-end.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:936:
web_contents_->GetRenderViewHost()->FirePageBeforeUnload(false);
Lets reset load_state_mask_ to kClosing here.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:970:
CallClientFunction("InspectorFrontendAPI.setIsDocked", &docked, NULL, NULL);
We don't need this as long as existing tests pass.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:987: if (dock_requested &&
!can_dock_)
Lets add DCHECK and proceed as if dock_requested == false.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:82: // Open or focus existing DevTools
window, and perform the specified action.
Open or reveal DevTools window

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:211: typedef int LoadStateMask;
nuke

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:354: LoadStateMask load_state_mask_;
LoadState

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/view_id_util_browsertest.mm (right):

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/view_id_util_browsertest.mm:53:
devtools->SetIsDockedAndShowImmediatelyForTest(true);
Could you inline the one above into this method to minimize the call-sites
impact?

[pfeldman, 2014-01-09 13:02:02]

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

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:937: }
if load_state_mask_ != IsLoaded, you should call
HandleBeforeUnload(web_contents_, true, foo);

[dgozman, 2014-01-14 15:26:16]

PTAL. I've additionally tested with long-loading DevTools.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_embedder_message_dispatcher.h (right):

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_embedder_message_dispatcher.h:34: virtual void
SetDockSide(const std::string& side) = 0;
On 2014/01/09 12:52:01, pfeldman wrote:
> Please comment as to-be-removed.

Done.

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

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:488: if (action.type() ==
DevToolsToggleAction::kNothing)
On 2014/01/09 12:52:01, pfeldman wrote:
> kNoop

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:610: load_state_mask_(can_dock ?
kNotLoaded : kIsDockedSet),
On 2014/01/09 12:52:01, pfeldman wrote:
> Let add a comment here about supporting for the legacy front-end.

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:936:
web_contents_->GetRenderViewHost()->FirePageBeforeUnload(false);
On 2014/01/09 12:52:01, pfeldman wrote:
> Lets reset load_state_mask_ to kClosing here.

I've set action_on_load to NoOp instead.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:937: }
On 2014/01/09 13:02:03, pfeldman wrote:
> if load_state_mask_ != IsLoaded, you should call
> HandleBeforeUnload(web_contents_, true, foo);

After discussing with lushnikov@, I've added a check to
InterceptPageBeforeUnload.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:970:
CallClientFunction("InspectorFrontendAPI.setIsDocked", &docked, NULL, NULL);
On 2014/01/09 12:52:01, pfeldman wrote:
> We don't need this as long as existing tests pass.

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:987: if (dock_requested &&
!can_dock_)
On 2014/01/09 12:52:01, pfeldman wrote:
> Lets add DCHECK and proceed as if dock_requested == false.

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:82: // Open or focus existing DevTools
window, and perform the specified action.
On 2014/01/09 12:52:01, pfeldman wrote:
> Open or reveal DevTools window

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:211: typedef int LoadStateMask;
On 2014/01/09 12:52:01, pfeldman wrote:
> nuke

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:354: LoadStateMask load_state_mask_;
On 2014/01/09 12:52:01, pfeldman wrote:
> LoadState

Done.

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/view_id_util_browsertest.mm (right):

https://codereview.chromium.org/108213012/diff/270001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/view_id_util_browsertest.mm:53:
devtools->SetIsDockedAndShowImmediatelyForTest(true);
On 2014/01/09 12:52:01, pfeldman wrote:
> Could you inline the one above into this method to minimize the call-sites
> impact?

Done.

[dgozman, 2014-01-14 19:14:31]

pfeldman, I've added comments, as we discussed. Please take a look.

jochen, please make an owner review.

tsepez, please take a look at devtools_embedder_dispatcher.

[Tom Sepez, 2014-01-14 19:17:44]

devtools_embedder_dispatcher LGTM.

[pfeldman, 2014-01-15 11:53:04]

lgtm

[jochen (gone - plz use gerrit), 2014-01-15 13:23:13]

rslgtm

[commit-bot: I haz the power, 2014-01-16 13:00:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/108213012/780001

[commit-bot: I haz the power, 2014-01-16 14:21:26]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-16 14:35:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/108213012/780001

[commit-bot: I haz the power, 2014-01-16 20:37:46]

Change committed as 245308