DevTools: Replace multiline InplaceEditor with CodeMirrorTextEditor

DevTools: Replace multiline InplaceEditor with CodeMirrorTextEditor

BUG=none

Review-Url: https://codereview.chromium.org/2372663004
Cr-Commit-Position: refs/heads/master@{#455202}
Committed: https://chromium.googlesource.com/chromium/src/+/24633cf6eced187fb4e0717f0083befce859d4fa

[einbinder, 2016-09-27 04:16:12]

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

[einbinder, 2016-09-27 04:16:12]

I still need to look this over, but if you have time ptal.

[lushnikov, 2016-09-27 18:28:41]

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/elements/edit/edit-dom-actions-4.html
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/elements/edit/edit-dom-actions-4.html:67:
// var value = treeElement._editing.editorForTest.text();
drop

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:825:
this._htmlEditElement.classList.toggle("multiline-editor", true);
can we re-use existing css class for this?

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:844:
if (cancelled)
maybe you can remove blur listener somewhere?

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:848:
this._htmlEditElement.classList.toggle("multiline-editor", false);
you probably can just throw away the this._htmlEditElement

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:849:
disposeCallback();
can we move this to the very end of the cancel function()

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:225:
margin-right:4px;
nit: space

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorUtils.js
(left):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorUtils.js:135:
lineWiseCopyCut: false,
let's move this to CMTE

[lushnikov, 2016-09-27 18:28:41]

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/elements/edit/edit-dom-actions-4.html
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/elements/edit/edit-dom-actions-4.html:67:
// var value = treeElement._editing.editorForTest.text();
drop

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:825:
this._htmlEditElement.classList.toggle("multiline-editor", true);
can we re-use existing css class for this?

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:844:
if (cancelled)
maybe you can remove blur listener somewhere?

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:848:
this._htmlEditElement.classList.toggle("multiline-editor", false);
you probably can just throw away the this._htmlEditElement

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:849:
disposeCallback();
can we move this to the very end of the cancel function()

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:225:
margin-right:4px;
nit: space

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorUtils.js
(left):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorUtils.js:135:
lineWiseCopyCut: false,
let's move this to CMTE

[einbinder, 2016-09-29 17:36:51]

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/elements/edit/edit-dom-actions-4.html
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/elements/edit/edit-dom-actions-4.html:67:
// var value = treeElement._editing.editorForTest.text();
On 2016/09/27 at 18:28:40, lushnikov wrote:
> drop

Done.

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:825:
this._htmlEditElement.classList.toggle("multiline-editor", true);
On 2016/09/27 at 18:28:40, lushnikov wrote:
> can we re-use existing css class for this?

Done.

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:844:
if (cancelled)
On 2016/09/27 at 18:28:41, lushnikov wrote:
> maybe you can remove blur listener somewhere?

Done.

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:848:
this._htmlEditElement.classList.toggle("multiline-editor", false);
On 2016/09/27 at 18:28:41, lushnikov wrote:
> you probably can just throw away the this._htmlEditElement

done.

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:849:
disposeCallback();
On 2016/09/27 at 18:28:41, lushnikov wrote:
> can we move this to the very end of the cancel function()

We can move to to right before we focus the treeOutline.

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css
(right):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:225:
margin-right:4px;
On 2016/09/27 at 18:28:41, lushnikov wrote:
> nit: space

Done.

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorUtils.js
(left):

https://codereview.chromium.org/2372663004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorUtils.js:135:
lineWiseCopyCut: false,
On 2016/09/27 at 18:28:41, lushnikov wrote:
> let's move this to CMTE

Done.

[lushnikov, 2016-09-29 21:16:26]

the f2/esc syntax throws error for me

https://codereview.chromium.org/2372663004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):

https://codereview.chromium.org/2372663004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:835:
// editor.widget().element.addEventListener("blur", this._editing.commit, true);
drop

[einbinder, 2016-09-29 22:16:19]

On 2016/09/29 at 21:16:26, lushnikov wrote:
> the f2/esc syntax throws error for me
> 
>
https://codereview.chromium.org/2372663004/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):
> 
>
https://codereview.chromium.org/2372663004/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:835:
// editor.widget().element.addEventListener("blur", this._editing.commit, true);
> drop

Sorry about that, fixed.

[lushnikov, 2016-10-01 00:55:43]

1. esc to exit mode triggers console
2. the "shift-page-up" doesn't work

[einbinder, 2016-10-03 18:41:12]

On 2016/10/01 at 00:55:43, lushnikov wrote:
> 1. esc to exit mode triggers console
> 2. the "shift-page-up" doesn't work

Done.

[lushnikov, 2016-10-13 23:56:16]

Description was changed from

==========
DevTools: Replace multiline InplaceEditor with CodeMirrorTextEditor

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

to

==========
DevTools: Replace multiline InplaceEditor with CodeMirrorTextEditor

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

[lushnikov, 2016-10-13 23:56:16]

lushnikov@chromium.org changed reviewers:
- lushnikov@chromium.org

[einbinder, 2017-03-01 23:08:33]

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

[einbinder, 2017-03-01 23:08:33]

ptal

[lushnikov, 2017-03-01 23:52:48]

code looks good! Let's fix the exception and land after the branch

lgtm

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:795:
function cancel() {
let's name this dispose (and call dispose.all(this) in commit())

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:814:
disposeCallback();
let's call this as the very-last line of the method

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:827:
var isMetaOrCtrl = Host.isMac() ? event.metaKey && !event.shiftKey &&
!event.ctrlKey && !event.altKey :
KeyboardShortcuts.eventHasCtrlOrMeta

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:829:
if (isEnterKey(event) && (isMetaOrCtrl || event.isMetaOrCtrlForTest)) {
Occassionally i hit this exception:

ElementsTreeElement.js:796 Uncaught (in promise) TypeError: Cannot read property
'editor' of undefined
    at Elements.ElementsTreeElement.cancel (ElementsTreeElement.js:796)
    at Elements.ElementsTreeOutline.setVisible (ElementsTreeOutline.js:300)
    at Elements.ElementsPanel.willHide (ElementsPanel.js:342)
    at Elements.ElementsPanel._notify (Widget.js:227)
    at Elements.ElementsPanel._processWillHide (Widget.js:204)
    at UI.ViewManager._ContainerWidget._callOnVisibleChildren (Widget.js:174)
    at UI.ViewManager._ContainerWidget._processWillHide (Widget.js:203)
    at UI.ViewManager._ContainerWidget._hideWidget (Widget.js:347)
    at UI.ViewManager._ContainerWidget.detach (Widget.js:371)
    at UI.TabbedPane._hideTab (TabbedPane.js:788)

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js
(left):

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:148:
if (this._multilineEditing)
let's resize to accomodate all space

[einbinder, 2017-03-02 00:25:10]

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js
(right):

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:795:
function cancel() {
On 2017/03/01 at 23:52:48, lushnikov wrote:
> let's name this dispose (and call dispose.all(this) in commit())

Done.

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:814:
disposeCallback();
On 2017/03/01 at 23:52:47, lushnikov wrote:
> let's call this as the very-last line of the method

Done.

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:827:
var isMetaOrCtrl = Host.isMac() ? event.metaKey && !event.shiftKey &&
!event.ctrlKey && !event.altKey :
On 2017/03/01 at 23:52:47, lushnikov wrote:
> KeyboardShortcuts.eventHasCtrlOrMeta

Done.

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:829:
if (isEnterKey(event) && (isMetaOrCtrl || event.isMetaOrCtrlForTest)) {
On 2017/03/01 at 23:52:48, lushnikov wrote:
> Occassionally i hit this exception:
> 
> ElementsTreeElement.js:796 Uncaught (in promise) TypeError: Cannot read
property 'editor' of undefined
>     at Elements.ElementsTreeElement.cancel (ElementsTreeElement.js:796)
>     at Elements.ElementsTreeOutline.setVisible (ElementsTreeOutline.js:300)
>     at Elements.ElementsPanel.willHide (ElementsPanel.js:342)
>     at Elements.ElementsPanel._notify (Widget.js:227)
>     at Elements.ElementsPanel._processWillHide (Widget.js:204)
>     at UI.ViewManager._ContainerWidget._callOnVisibleChildren (Widget.js:174)
>     at UI.ViewManager._ContainerWidget._processWillHide (Widget.js:203)
>     at UI.ViewManager._ContainerWidget._hideWidget (Widget.js:347)
>     at UI.ViewManager._ContainerWidget.detach (Widget.js:371)
>     at UI.TabbedPane._hideTab (TabbedPane.js:788)

Done.

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js
(left):

https://codereview.chromium.org/2372663004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:148:
if (this._multilineEditing)
On 2017/03/01 at 23:52:48, lushnikov wrote:
> let's resize to accomodate all space

Done.

[einbinder, 2017-03-06 22:04:54]

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

[commit-bot: I haz the power, 2017-03-06 22:05:26]

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, 2017-03-07 00:05:56]

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

[commit-bot: I haz the power, 2017-03-07 00:05:57]

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

[einbinder, 2017-03-07 01:52:02]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-03-07 01:52:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2372663004/#ps140001
(title: "resize width")

[commit-bot: I haz the power, 2017-03-07 01:53:17]

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, 2017-03-07 04:49:50]

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

[commit-bot: I haz the power, 2017-03-07 04:49:51]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[einbinder, 2017-03-07 06:03:38]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2017-03-07 06:04:12]

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, 2017-03-07 08:28:56]

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

[commit-bot: I haz the power, 2017-03-07 08:28:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[einbinder, 2017-03-07 19:32:03]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2017-03-07 19:33:19]

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, 2017-03-07 20:30:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488915123605950,
"parent_rev": "69e6c56eba06ece07675c89d8b3dd08229ae7584", "commit_rev":
"24633cf6eced187fb4e0717f0083befce859d4fa"}

[commit-bot: I haz the power, 2017-03-07 20:31:26]

Description was changed from

==========
DevTools: Replace multiline InplaceEditor with CodeMirrorTextEditor

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

to

==========
DevTools: Replace multiline InplaceEditor with CodeMirrorTextEditor

BUG=none

Review-Url: https://codereview.chromium.org/2372663004
Cr-Commit-Position: refs/heads/master@{#455202}
Committed:
https://chromium.googlesource.com/chromium/src/+/24633cf6eced187fb4e0717f0083...
==========

[commit-bot: I haz the power, 2017-03-07 20:31:27]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/24633cf6eced187fb4e0717f0083...