Devtools: [LayoutEditor] highlight changed value in the SSP

Devtools: [LayoutEditor] highlight changed value in the SSP

BUG=501896
NOTRY=true

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

[sergeyv, 2015-08-21 01:37:25]

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

[pfeldman, 2015-08-21 01:39:41]

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:3231: "name": "layoutEditorChange",
this name lgtm, deferring the rest to dgozman.

[pfeldman, 2015-08-21 01:43:00]

https://codereview.chromium.org/1310433002/diff/1/Source/core/inspector/Inspe...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/1310433002/diff/1/Source/core/inspector/Inspe...
Source/core/inspector/InspectorCSSAgent.cpp:1663:
frontend()->layoutEditorChange(inspectorStyleSheet->id(),
inspectorStyleSheet->buildSourceRangeObject(changeRange));
You might need to flush stylesheetadded events.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/PropertyChangeHighlighter.js (right):

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
make sure lushnikov@ likes it.

[sergeyv, 2015-08-21 02:17:18]

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

[lushnikov, 2015-08-21 02:36:08]

lgtm given usability is nice

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/PropertyChangeHighlighter.js (right):

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:18: 
stray line

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:36: delete
this._nextAnimation;
you might want to check ssp's node's target here

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:48: if
(this._checkRanges(declaration.range, this._changeRange)) {
FYI: you compare outdated declaration range here.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:61: if
(treeElement.property.range  && this._checkRanges(treeElement.property.range,
this._changeRange)) {
FYI: this might occasionally fail as property range is outdated

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:80: delete
this._animationStart;
let's delete background

[sergeyv, 2015-08-21 04:05:42]

https://codereview.chromium.org/1310433002/diff/1/Source/core/inspector/Inspe...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/1310433002/diff/1/Source/core/inspector/Inspe...
Source/core/inspector/InspectorCSSAgent.cpp:1663:
frontend()->layoutEditorChange(inspectorStyleSheet->id(),
inspectorStyleSheet->buildSourceRangeObject(changeRange));
On 2015/08/21 01:43:00, pfeldman wrote:
> You might need to flush stylesheetadded events.

Done.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
File Source/devtools/front_end/elements/PropertyChangeHighlighter.js (right):

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/08/21 01:43:00, pfeldman wrote:
> make sure lushnikov@ likes it.

Done.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:18: 
On 2015/08/21 02:36:07, lushnikov wrote:
> stray line

Done.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:36: delete
this._nextAnimation;
On 2015/08/21 02:36:07, lushnikov wrote:
> you might want to check ssp's node's target here

Done.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:48: if
(this._checkRanges(declaration.range, this._changeRange)) {
On 2015/08/21 02:36:07, lushnikov wrote:
> FYI: you compare outdated declaration range here. 

Done.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:61: if
(treeElement.property.range  && this._checkRanges(treeElement.property.range,
this._changeRange)) {
On 2015/08/21 02:36:07, lushnikov wrote:
> FYI: this might occasionally fail as property range is outdated

Done.

https://codereview.chromium.org/1310433002/diff/1/Source/devtools/front_end/e...
Source/devtools/front_end/elements/PropertyChangeHighlighter.js:80: delete
this._animationStart;
On 2015/08/21 02:36:07, lushnikov wrote:
> let's delete background

Done.

[sergeyv, 2015-08-21 04:05:54]

The CQ bit was checked by sergeyv@chromium.org

[sergeyv, 2015-08-21 04:05:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1310433002/#ps20001
(title: "address comments")

[commit-bot: I haz the power, 2015-08-21 04:06:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310433002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310433002/20001

[commit-bot: I haz the power, 2015-08-21 06:19:17]

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

[commit-bot: I haz the power, 2015-08-21 06:19:18]

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

[sergeyv, 2015-08-21 16:53:48]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2015-08-21 16:54:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310433002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310433002/20001

[commit-bot: I haz the power, 2015-08-21 16:58:35]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200991