DevTools: setting to auto-reveal in navigator

DevTools: setting to auto-reveal in navigator

BUG=556508
Committed: https://crrev.com/ae7e36e936dfb5efdf5e29919187cbb81d32deb1
Cr-Commit-Position: refs/heads/master@{#414893}

[luoe, 2016-08-19 09:40:39]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 09:41:05]

Dry run: 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-08-19 09:43:39]

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

[commit-bot: I haz the power, 2016-08-19 09:43:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[luoe, 2016-08-19 21:03:35]

luoe@chromium.org changed reviewers:
+ lushnikov@chromium.org

[luoe, 2016-08-19 21:03:35]

- Opens the left side navigator if it wasn't opened
- Doesn't trigger in Quick Source
- Reveals in navigator when a tab is selected (includes when opened by pause in
debugger)
- Settings has a new checkbox that is False by default

[luoe, 2016-08-19 21:03:42]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 21:04:34]

Dry run: 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-08-20 01:34:48]

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

[commit-bot: I haz the power, 2016-08-20 01:34:48]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2016-08-23 19:07:51]

>> Opens the left side navigator if it wasn't opened

Let's not open it in this case? I think we can avoid adding setting in this case

[pfeldman, 2016-08-23 19:14:26]

On 2016/08/23 19:07:51, lushnikov wrote:
> >> Opens the left side navigator if it wasn't opened
> 
> Let's not open it in this case? I think we can avoid adding setting in this
case

We should not open navigator.

[pfeldman, 2016-08-23 19:14:58]

(and we need the setting for reveal)

[luoe, 2016-08-24 00:57:24]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 00:58:00]

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

[luoe, 2016-08-24 00:58:29]

Thanks for the comments.  No longer opens navigator, checkbox setting remains,
skipFocus stuff removed.

[commit-bot: I haz the power, 2016-08-24 02:57:41]

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

[commit-bot: I haz the power, 2016-08-24 02:57:41]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2016-08-25 02:00:10]

https://codereview.chromium.org/2254683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:374: if
(skipShow)
should it be vice-versa?

[luoe, 2016-08-25 18:19:34]

This one should actually work.

https://codereview.chromium.org/2254683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:374: if
(skipShow)
On 2016/08/25 02:00:09, lushnikov wrote:
> should it be vice-versa?

This check is wrong.  Really, we want to call showView so that clicking on a
snippet will open the 'snippets' tab in the navigator when it's open.  I've
changed it to a boolean passed in that tells the view to skip the reveal
callback.  This prevents the navigator from opening, but still switches tabs
(sources/content scripts/snippets) when it's open.

[lushnikov, 2016-08-25 20:41:54]

https://codereview.chromium.org/2254683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:374:
WebInspector.viewManager.showView(extensions[i].descriptor()["viewId"],
skipReveal);
let's select tab explicitly via tabbedPane.selectTab; i don't like the
skipReveal flag in the showView method

[luoe, 2016-08-26 18:33:45]

https://codereview.chromium.org/2254683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:374:
WebInspector.viewManager.showView(extensions[i].descriptor()["viewId"],
skipReveal);
On 2016/08/25 20:41:54, lushnikov wrote:
> let's select tab explicitly via tabbedPane.selectTab; i don't like the
> skipReveal flag in the showView method

Done with _navigatorTabbedLocation.tabbedPane().selectTab

[lushnikov, 2016-08-26 22:31:09]

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:360:
_revealInNavigator: function(uiSourceCode, skipReveal)
this seems to be never used.

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:376: if
(this.editorView.mainWidget())
isn't it the same as always calling WebInspector.viewManager.showView(viewId) ?

[pfeldman, 2016-08-26 22:36:26]

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

[pfeldman, 2016-08-26 22:36:27]

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:376: if
(this.editorView.mainWidget())
On 2016/08/26 22:31:09, lushnikov wrote:
> isn't it the same as always calling WebInspector.viewManager.showView(viewId)
?

You should not do WebInspector.viewManager.showView(viewId) until necessary.

[luoe, 2016-08-26 23:14:29]

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:360:
_revealInNavigator: function(uiSourceCode, skipReveal)
On 2016/08/26 22:31:09, lushnikov wrote:
> this seems to be never used.

Yeah, I messed up the if statement on #376

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:376: if
(this.editorView.mainWidget())
On 2016/08/26 22:31:09, lushnikov wrote:
> isn't it the same as always calling WebInspector.viewManager.showView(viewId)
?

Sorry, this condition should have been skipReveal, which is only true when the
setting is on && tab is selected.

https://codereview.chromium.org/2254683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:376: if
(this.editorView.mainWidget())
On 2016/08/26 22:36:27, pfeldman wrote:
> On 2016/08/26 22:31:09, lushnikov wrote:
> > isn't it the same as always calling
WebInspector.viewManager.showView(viewId)
> ?
> 
> You should not do WebInspector.viewManager.showView(viewId) until necessary.

Right, the condition should say if (skipReveal) {...} else { showView }
showView should only be called if we are handling a context menu event.

[lushnikov, 2016-08-26 23:23:52]

lgtm

https://codereview.chromium.org/2254683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2254683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:376: if
(skipReveal)
I guess @pfeldman meant that we should shortcut on viewManager.showView calls?

In this case, let's do

if (skipReveal || this.editorView.mainWidget()) ...

[luoe, 2016-08-26 23:37:27]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 23:38:21]

Dry run: 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-08-27 01:23:52]

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

[commit-bot: I haz the power, 2016-08-27 01:23:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[luoe, 2016-08-27 02:30:50]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 02:31:36]

Dry run: 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-08-27 03:33:08]

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

[commit-bot: I haz the power, 2016-08-27 03:33:09]

Dry run: This issue passed the CQ dry run.

[luoe, 2016-08-27 06:59:51]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2016-08-27 07:00:49]

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-08-27 10:40:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-27 10:42:58]

Description was changed from

==========
DevTools: setting to auto-reveal in navigator

BUG=556508
==========

to

==========
DevTools: setting to auto-reveal in navigator

BUG=556508

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

[commit-bot: I haz the power, 2016-08-27 10:42:59]

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