DevTools: Reimplemented WI.WatchExpressionsSidebarPane

DevTools: Reimplemented WI.WatchExpressionsSidebarPane

BUG=
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190480

[sergeyv, 2015-02-06 16:42:20]

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

[pfeldman, 2015-02-09 13:13:53]

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/ObjectPropertiesSection.js (right):

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/ObjectPropertiesSection.js:194: if
(!this.property.value && this.property.getter)
I don't see how the old code maps to the new code. Please extract this into a
separate change.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/ObjectPropertiesSection.js:1039:
nameElement.createTextChildren("\"", name.replace(/\n/g, "\u21B5"), "\"");
What is happening here?

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
File Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js (right):

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:57:
this._rebuildWatchExpressionUIs(WebInspector.settings.watchExpressions.get());
You should only do this upon wasShown as previously.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:122:
_rebuildWatchExpressionUIs: function(watchExpressionStrings)
Please annotate.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:142: if
(this._emptyElement) {
You should use CSS to show / hide it.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:379: function
handleClick() {
{ next line

[sergeyv, 2015-02-17 14:22:58]

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/ObjectPropertiesSection.js (right):

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/ObjectPropertiesSection.js:194: if
(!this.property.value && this.property.getter)
On 2015/02/09 13:13:52, pfeldman wrote:
> I don't see how the old code maps to the new code. Please extract this into a
> separate change.

Done.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
File Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js (right):

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:57:
this._rebuildWatchExpressionUIs(WebInspector.settings.watchExpressions.get());
On 2015/02/09 13:13:52, pfeldman wrote:
> You should only do this upon wasShown as previously.

Done.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:122:
_rebuildWatchExpressionUIs: function(watchExpressionStrings)
On 2015/02/09 13:13:53, pfeldman wrote:
> Please annotate.

Done.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:142: if
(this._emptyElement) {
On 2015/02/09 13:13:53, pfeldman wrote:
> You should use CSS to show / hide it.

Done.

https://codereview.chromium.org/905743003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:379: function
handleClick() {
On 2015/02/09 13:13:53, pfeldman wrote:
> { next line

Done.

[aandrey, 2015-02-17 14:42:56]

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

[aandrey, 2015-02-17 14:42:57]

https://codereview.chromium.org/905743003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js (right):

https://codereview.chromium.org/905743003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:56:
this._emptyElement = this.bodyElement.createChild("div", "info");
have you considered extending from {WebInspector.NativeBreakpointsSidebarPane} ?

[sergeyv, 2015-02-18 17:29:57]

Patchset #4 (id:60001) has been deleted

[sergeyv, 2015-02-18 17:33:00]

Patchset #4 (id:80001) has been deleted

[sergeyv, 2015-02-18 18:05:34]

Patchset #4 (id:100001) has been deleted

[pfeldman, 2015-02-18 20:51:00]

Mostly looks good, lets brush up the styles and land this.

https://codereview.chromium.org/905743003/diff/120001/LayoutTests/inspector/s...
File LayoutTests/inspector/sources/debugger/properties-special.html (right):

https://codereview.chromium.org/905743003/diff/120001/LayoutTests/inspector/s...
LayoutTests/inspector/sources/debugger/properties-special.html:23:
InspectorTest.addSniffer(WebInspector.WatchExpression.prototype,
"_createWatchExpression", watchExpressionsUpdated, true);
You'll have to rebaseline this.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/components/ObjectPropertiesSection.js (left):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/components/ObjectPropertiesSection.js:224:
populateContextMenu: function(contextMenu)
Remove this?

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/components/ObjectPropertiesSection.js (right):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/components/ObjectPropertiesSection.js:1: /*
Looks like you can simply land changes in this file separately.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js (right):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:44:
this.bodyElement.classList.add("vbox");
add("vbox", "watch-expressions")

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:382:
this._preventClickTimeout = setTimeout(handleClick, 333);
You don't need this anymore.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:435:
this._element.classList.add("hovered");
why not :hover style?

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/sources/sourcesPanel.css (right):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:213: .watch-expressions {
Lets try distilling these styles - seems like you are adding more than there
was.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:215: padding-right: 6px;
use single padding: shorthand

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:221: flex-shrink: 0;
flex: none

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:272: .watch-expression
.section > .header::before {
Why is this one necessary?

[sergeyv, 2015-02-19 10:53:46]

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/components/ObjectPropertiesSection.js (left):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/components/ObjectPropertiesSection.js:224:
populateContextMenu: function(contextMenu)
On 2015/02/18 20:51:00, pfeldman wrote:
> Remove this?

Done.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/components/ObjectPropertiesSection.js (right):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/components/ObjectPropertiesSection.js:1: /*
On 2015/02/18 20:51:00, pfeldman wrote:
> Looks like you can simply land changes in this file separately.

Unfortunately, It will break watch expressions

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js (right):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:44:
this.bodyElement.classList.add("vbox");
On 2015/02/18 20:51:00, pfeldman wrote:
> add("vbox", "watch-expressions")

Done.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:382:
this._preventClickTimeout = setTimeout(handleClick, 333);
On 2015/02/18 20:51:00, pfeldman wrote:
> You don't need this anymore.

We still need this to correctly handle dbl click

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js:435:
this._element.classList.add("hovered");
On 2015/02/18 20:51:00, pfeldman wrote:
> why not :hover style?

Done.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
File Source/devtools/front_end/sources/sourcesPanel.css (right):

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:213: .watch-expressions {
On 2015/02/18 20:51:00, pfeldman wrote:
> Lets try distilling these styles - seems like you are adding more than there
> was.

Well, I tried

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:215: padding-right: 6px;
On 2015/02/18 20:51:00, pfeldman wrote:
> use single padding: shorthand

Done.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:221: flex-shrink: 0;
On 2015/02/18 20:51:00, pfeldman wrote:
> flex: none

Done.

https://codereview.chromium.org/905743003/diff/120001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:272: .watch-expression
.section > .header::before {
On 2015/02/18 20:51:00, pfeldman wrote:
> Why is this one necessary?

By default  it has value 2px, so it is lower than it is need

[pfeldman, 2015-02-19 13:23:17]

lgtm

[sergeyv, 2015-02-19 13:24:55]

New patchsets have been uploaded after l-g-t-m from pfeldman@chromium.org

[sergeyv, 2015-02-19 13:25:54]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2015-02-19 13:27:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/905743003/160001

[sergeyv, 2015-02-19 15:22:59]

The CQ bit was unchecked by sergeyv@chromium.org

[sergeyv, 2015-02-19 15:23:00]

The CQ bit was checked by sergeyv@chromium.org

[sergeyv, 2015-02-19 15:23:07]

The CQ bit was unchecked by sergeyv@chromium.org

[sergeyv, 2015-02-19 15:23:08]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2015-02-19 15:27:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/905743003/160001

[commit-bot: I haz the power, 2015-02-19 15:39:18]

Committed patchset #6 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190480