Remove redundant DevToolsManager methods and clean up its clients.

Remove redundant DevToolsManager methods and clean up its clients.


BUG=None
TBR=jam

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

[Vladislav Kaznacheev, 2013-03-29 11:22:30]

Please take a look.

[pfeldman, 2013-03-29 11:35:49]

https://chromiumcodereview.appspot.com/13305002/diff/1/content/public/browser...
File content/public/browser/devtools_manager.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/1/content/public/browser...
content/public/browser/devtools_manager.h:39: virtual void
DispatchOnInspectorFrontend(DevToolsAgentHost* from,
I don't think that anyone other than devtools agent should be able to dispatch
messages on agent's behalf.

[Vladislav Kaznacheev, 2013-03-29 12:30:46]

PTAL

On 2013/03/29 11:35:49, pfeldman wrote:
>
https://chromiumcodereview.appspot.com/13305002/diff/1/content/public/browser...
> File content/public/browser/devtools_manager.h (right):
> 
>
https://chromiumcodereview.appspot.com/13305002/diff/1/content/public/browser...
> content/public/browser/devtools_manager.h:39: virtual void
> DispatchOnInspectorFrontend(DevToolsAgentHost* from,
> I don't think that anyone other than devtools agent should be able to dispatch
> messages on agent's behalf.

[pfeldman, 2013-03-29 14:59:23]

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_sanity_browsertest.cc:157:
scoped_refptr<DevToolsAgentHost> agent(
You no longer need this.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_window.cc (left):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.cc:721:
host->ReplacedWithAnotherClient();
Why did this go away?

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_window.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.cc:292: void DevToolsWindow::Hide() {
Public method that deletes this is dangerous.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_window.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.h:85: void Hide();
I'd place Hide under Show.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.h:175: static DevToolsWindow*
AsDevToolsWindow(content::DevToolsAgentHost*);
"AsDevToolsWindow" name does not make sense now.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:528: if
(DevToolsManager::GetInstance()->IsAttached(agent)) {
There already is IsDebuggerAttached(WebContents), consider aligning the two.

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/browser/dev...
File content/browser/devtools/devtools_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/browser/dev...
content/browser/devtools/devtools_manager_impl.cc:70:
old_client_host->ReplacedWithAnotherClient();
Ok. Only DevToolsWindow is allowed to kick other clients out, that's why it was
there.

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
File content/public/browser/devtools_manager.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
content/public/browser/devtools_manager.h:41: virtual bool
IsAttached(DevToolsAgentHost* agent_host) = 0;
Sounds like an instance method on DevToolsAgentHost

[Vladislav Kaznacheev, 2013-03-29 15:15:35]

https://chromiumcodereview.appspot.com/13305002/diff/1/content/public/browser...
File content/public/browser/devtools_manager.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/1/content/public/browser...
content/public/browser/devtools_manager.h:39: virtual void
DispatchOnInspectorFrontend(DevToolsAgentHost* from,
Moved to impl part.
On 2013/03/29 11:35:49, pfeldman wrote:
> I don't think that anyone other than devtools agent should be able to dispatch
> messages on agent's behalf.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_sanity_browsertest.cc:157:
scoped_refptr<DevToolsAgentHost> agent(
On 2013/03/29 14:59:23, pfeldman wrote:
> You no longer need this.

Done.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_window.cc (left):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.cc:721:
host->ReplacedWithAnotherClient();
Register... now takes care of that.
On 2013/03/29 14:59:23, pfeldman wrote:
> Why did this go away?

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_window.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.cc:292: void DevToolsWindow::Hide() {
Made private
On 2013/03/29 14:59:23, pfeldman wrote:
> Public method that deletes this is dangerous.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
File chrome/browser/devtools/devtools_window.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.h:85: void Hide();
On 2013/03/29 14:59:23, pfeldman wrote:
> I'd place Hide under Show.

Done.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/devt...
chrome/browser/devtools/devtools_window.h:175: static DevToolsWindow*
AsDevToolsWindow(content::DevToolsAgentHost*);
On 2013/03/29 14:59:23, pfeldman wrote:
> "AsDevToolsWindow" name does not make sense now.

Done.

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:528: if
(DevToolsManager::GetInstance()->IsAttached(agent)) {
There is, but it is not everywhere that we have WebContents (sometimes we only
have DTAH and it might point to a shared worker)
On 2013/03/29 14:59:23, pfeldman wrote:
> There already is IsDebuggerAttached(WebContents), consider aligning the two.

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/browser/dev...
File content/browser/devtools/devtools_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/browser/dev...
content/browser/devtools/devtools_manager_impl.cc:70:
old_client_host->ReplacedWithAnotherClient();
It was doing it through a public API which anyone can call.
On 2013/03/29 14:59:23, pfeldman wrote:
> Ok. Only DevToolsWindow is allowed to kick other clients out, that's why it
was
> there.

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
File content/public/browser/devtools_manager.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
content/public/browser/devtools_manager.h:41: virtual bool
IsAttached(DevToolsAgentHost* agent_host) = 0;
The real knowledge resides in DevToolsManager(Impl) and if we add this to
DevToolsAgentHost it will be a convenience method.
The whole point of this change was removing a public GetDevToolsClientHostFor.
On 2013/03/29 14:59:23, pfeldman wrote:
> Sounds like an instance method on DevToolsAgentHost

[pfeldman, 2013-03-30 08:54:57]

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
File content/public/browser/devtools_manager.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
content/public/browser/devtools_manager.h:41: virtual bool
IsAttached(DevToolsAgentHost* agent_host) = 0;
Here is how I see it: you are adding this method into the public API. It always
accepts DTAH that resides right next to DTM. All I am suggesting is to move it
there so that call sites were simpler. We should not consider DTM API separate
from DTAH. They are two pieces of one puzzle, the API the expose is a single
entity and I'm suggesting structuring it better.

https://chromiumcodereview.appspot.com/13305002/diff/16001/content/browser/de...
File content/browser/devtools/devtools_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/16001/content/browser/de...
content/browser/devtools/devtools_manager_impl.cc:69: if (old_client_host) {
As I mentioned, now remote debugging can kick out devtools window. This should
not be possible.

[Vladislav Kaznacheev, 2013-04-01 09:26:17]

PTAL

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
File content/public/browser/devtools_manager.h (right):

https://chromiumcodereview.appspot.com/13305002/diff/5001/content/public/brow...
content/public/browser/devtools_manager.h:41: virtual bool
IsAttached(DevToolsAgentHost* agent_host) = 0;
Agree. Done.

On 2013/03/30 08:54:57, pfeldman wrote:
> Here is how I see it: you are adding this method into the public API. It
always
> accepts DTAH that resides right next to DTM. All I am suggesting is to move it
> there so that call sites were simpler. We should not consider DTM API separate
> from DTAH. They are two pieces of one puzzle, the API the expose is a single
> entity and I'm suggesting structuring it better.

https://chromiumcodereview.appspot.com/13305002/diff/16001/content/browser/de...
File content/browser/devtools/devtools_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13305002/diff/16001/content/browser/de...
content/browser/devtools/devtools_manager_impl.cc:69: if (old_client_host) {
Not really. Both remote debugging implementations (http and chrome.debugger)
check for IsAttached before registering and return an error if true.

On 2013/03/30 08:54:57, pfeldman wrote:
> As I mentioned, now remote debugging can kick out devtools window. This should
> not be possible.

[pfeldman, 2013-04-01 12:04:17]

lgtm

[commit-bot: I haz the power, 2013-04-01 12:16:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/13305002/20001

[commit-bot: I haz the power, 2013-04-01 14:20:00]

Change committed as 191609

[jam, 2013-04-01 14:59:05]

in the future, please add me on the review if you'll TBR (and make sure TBR=jam
is in the description when you first publish it). Otherwise, I won't see this
change except by accident.

[Vladislav Kaznacheev, 2013-04-01 15:11:01]

Sure, sorry about this.

On 2013/04/01 14:59:05, jam wrote:
> in the future, please add me on the review if you'll TBR (and make sure
TBR=jam
> is in the description when you first publish it). Otherwise, I won't see this
> change except by accident.