DevTools: Open Elements panel sooner on Inspect Element

DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
TBR=sky

Committed: https://crrev.com/dfa567bd9bfd0625abd29223e3ab038c508e5ae2
Cr-Commit-Position: refs/heads/master@{#438975}

[einbinder, 2016-12-09 00:28:55]

einbinder@chromium.org changed reviewers:
+ dgozman@chromium.org

[einbinder, 2016-12-09 00:28:57]

I'll add a bug for this later.

Also, how should I test this code?

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:529: std::string());
Do we prefer std::string() over ""? Both seem to get used.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:641:
DevToolsToggleAction::Inspect());
The Inspect action is for Ctrl+Shift+C. I'm borrowing it here for right click
inspect element, but I'm not sure if that will cause problems.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:933: url_string += "&panel=" + panel;
Concatenating the string here feels dangerous.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/ui/webui/dev...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/ui/webui/dev...
chrome/browser/ui/webui/devtools_ui.cc:145: return value;
Should I have a white list of panel names here?

https://codereview.chromium.org/2566573002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2566573002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:55: this._tabsById
= new Map();
The never ending quest to convert Objects to Maps.

[dgozman, 2016-12-09 22:19:32]

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:529: std::string());
On 2016/12/09 00:28:56, einbinder wrote:
> Do we prefer std::string() over ""? Both seem to get used.

Should not matter.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:605: default:
Omit the default clause.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:641:
DevToolsToggleAction::Inspect());
On 2016/12/09 00:28:56, einbinder wrote:
> The Inspect action is for Ctrl+Shift+C. I'm borrowing it here for right click
> inspect element, but I'm not sure if that will cause problems.

Let's do action ShowElementsPanel, and rename console to ShowConsolePanel.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/ui/webui/dev...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/ui/webui/dev...
chrome/browser/ui/webui/devtools_ui.cc:145: return value;
On 2016/12/09 00:28:56, einbinder wrote:
> Should I have a white list of panel names here?

Yes.

[einbinder, 2016-12-12 20:45:54]

Description was changed from

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=none
==========

to

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
==========

[einbinder, 2016-12-12 22:04:28]

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:605: default:
On 2016/12/09 at 22:19:31, dgozman wrote:
> Omit the default clause.

Done.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_window.cc:641:
DevToolsToggleAction::Inspect());
On 2016/12/09 at 22:19:31, dgozman wrote:
> On 2016/12/09 00:28:56, einbinder wrote:
> > The Inspect action is for Ctrl+Shift+C. I'm borrowing it here for right
click
> > inspect element, but I'm not sure if that will cause problems.
> 
> Let's do action ShowElementsPanel, and rename console to ShowConsolePanel.

Done.

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/ui/webui/dev...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2566573002/diff/1/chrome/browser/ui/webui/dev...
chrome/browser/ui/webui/devtools_ui.cc:145: return value;
On 2016/12/09 at 22:19:31, dgozman wrote:
> On 2016/12/09 00:28:56, einbinder wrote:
> > Should I have a white list of panel names here?
> 
> Yes.

Done.

[dgozman, 2016-12-13 21:00:21]

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

https://codereview.chromium.org/2566573002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1325:
bindings_->CallClientFunction("DevToolsAPI.showPanel", &panel_name, NULL,
I think we don't need this anymore except for old frontends? Let's move the
support to devtools_compatibilty.js then.

[einbinder, 2016-12-14 02:52:08]

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

https://codereview.chromium.org/2566573002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1325:
bindings_->CallClientFunction("DevToolsAPI.showPanel", &panel_name, NULL,
On 2016/12/13 at 21:00:21, dgozman wrote:
> I think we don't need this anymore except for old frontends? Let's move the
support to devtools_compatibilty.js then.

Done.

[dgozman, 2016-12-14 05:29:36]

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

https://codereview.chromium.org/2566573002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1325:
bindings_->CallClientFunction("DevToolsAPI.showPanel", &panel_name, NULL,
This call is not needed as well.

https://codereview.chromium.org/2566573002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js
(right):

https://codereview.chromium.org/2566573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:339:
DevToolsAPI.sendMessageToEmbedder('loadCompleted', [], null);
showPanel should be called here instead.

[einbinder, 2016-12-15 03:28:03]

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

https://codereview.chromium.org/2566573002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:1325:
bindings_->CallClientFunction("DevToolsAPI.showPanel", &panel_name, NULL,
On 2016/12/14 at 05:29:36, dgozman wrote:
> This call is not needed as well.

Removed.

https://codereview.chromium.org/2566573002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js
(right):

https://codereview.chromium.org/2566573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:339:
DevToolsAPI.sendMessageToEmbedder('loadCompleted', [], null);
On 2016/12/14 at 05:29:36, dgozman wrote:
> showPanel should be called here instead.

Done.

[dgozman, 2016-12-15 18:49:17]

lgtm

https://codereview.chromium.org/2566573002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2566573002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:148: (value == "elements" || value ==
"security" || value == "console"))
Remove security.

[einbinder, 2016-12-15 21:30:17]

https://codereview.chromium.org/2566573002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2566573002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:148: (value == "elements" || value ==
"security" || value == "console"))
On 2016/12/15 at 18:49:16, dgozman wrote:
> Remove security.

Done.

[einbinder, 2016-12-15 21:30:21]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2016-12-15 21:30:21]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2566573002/#ps80001
(title: "Remove security from whitelist")

[commit-bot: I haz the power, 2016-12-15 21:31:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 21:41:13]

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

[commit-bot: I haz the power, 2016-12-15 21:41:15]

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

[einbinder, 2016-12-15 22:49:28]

Description was changed from

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
==========

to

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
TBR=sky
==========

[einbinder, 2016-12-15 22:49:58]

TBRing sky for simple rename in chrome/browser/ui

[einbinder, 2016-12-15 23:13:32]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2016-12-15 23:13:32]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2566573002/#ps100001
(title: "Missed one showConsolePanel")

[commit-bot: I haz the power, 2016-12-15 23:14:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 01:15:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481843612434850,
"parent_rev": "3d36666a0587509844269909294775d97cce7cf8", "commit_rev":
"870bdebe8c7419eb5693c7cef0a64ab03c9f1bda"}

[commit-bot: I haz the power, 2016-12-16 01:16:23]

Description was changed from

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
TBR=sky
==========

to

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
TBR=sky

Review-Url: https://codereview.chromium.org/2566573002
==========

[commit-bot: I haz the power, 2016-12-16 01:16:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-16 01:20:48]

Description was changed from

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
TBR=sky

Review-Url: https://codereview.chromium.org/2566573002
==========

to

==========
DevTools: Open Elements panel sooner on Inspect Element

This patch adds a query parameter "panel" which sets the default panel
when devtools loads.

BUG=673472
TBR=sky

Committed: https://crrev.com/dfa567bd9bfd0625abd29223e3ab038c508e5ae2
Cr-Commit-Position: refs/heads/master@{#438975}
==========

[commit-bot: I haz the power, 2016-12-16 01:20:50]

Patchset 6 (id:??) landed as
https://crrev.com/dfa567bd9bfd0625abd29223e3ab038c508e5ae2
Cr-Commit-Position: refs/heads/master@{#438975}

[falken, 2016-12-16 05:14:08]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2016-12-16 05:14:09]

I think this causes a DCHECK failure
https://bugs.chromium.org/p/chromium/issues/detail?id=674809