Devtools: Add editable keyframes to the styles sidebar pane

Devtools: Add editable keyframes to the styles sidebar pane

BUG=266276
Committed: https://crrev.com/301c00ccc158984672abc7de19466e48619e81a5
Cr-Commit-Position: refs/heads/master@{#372224}

[samli, 2016-01-11 07:54:23]

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

[samli, 2016-01-11 07:54:23]

PTAL.

Screenshot: http://i.imgur.com/93OuFLX.png

[paulirish, 2016-01-12 01:51:42]

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

[samli, 2016-01-12 23:19:14]

ping, ptal.

[pfeldman, 2016-01-13 00:08:19]

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:379: String
text = "@keyframes boguzAnim { " + keyText + " { " + bogusPropertyName + ":
none; } }";
Just inline bogusPropertyName it here and below.

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(left):

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:56:
WebInspector.StylesSidebarPane.Events = {
Also looks like a separate change.

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

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js:1077:
WebInspector.CSSRule.prototype = {
What is happening here? Can we land this as a separate patch?

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:3131: { "name": "origin",
"$ref": "StyleSheetOrigin", "description": "Parent stylesheet's origin."},
I'd rather remove "origin" from CSSRule and not add it here - we have origin as
a part of header already. (Can be done in a separate patch.)

[samli, 2016-01-13 00:53:37]

PTAL

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:379: String
text = "@keyframes boguzAnim { " + keyText + " { " + bogusPropertyName + ":
none; } }";
On 2016/01/13 at 00:08:19, pfeldman wrote:
> Just inline bogusPropertyName it here and below.

In the other methods too? They check the property name too.

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(left):

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:56:
WebInspector.StylesSidebarPane.Events = {
On 2016/01/13 at 00:08:19, pfeldman wrote:
> Also looks like a separate change.

Done. Split into separate change.

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

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js:1077:
WebInspector.CSSRule.prototype = {
On 2016/01/13 at 00:08:19, pfeldman wrote:
> What is happening here? Can we land this as a separate patch?

Done. Split into a separate patch.

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1577723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:3131: { "name": "origin",
"$ref": "StyleSheetOrigin", "description": "Parent stylesheet's origin."},
On 2016/01/13 at 00:08:19, pfeldman wrote:
> I'd rather remove "origin" from CSSRule and not add it here - we have origin
as a part of header already. (Can be done in a separate patch.)

Protocol says that the optional StyleSheetId for CSSRule is absent for user
agent stylesheet and user-specified stylesheet rules. If that's true, we can't
just rely on header origin.

[lushnikov, 2016-01-13 01:42:18]

lgtm

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:745:
* @param {?WebInspector.CSSStyleModel.MatchedStyleResult} matchedStyles
can we keep matchedStyles non-nullable? You can make method isKeyframesSection()
which checks the type of style.parentRule

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1181:
_isPropertyOverloaded: function(property)
you can avoid doing this if matchedStyles will be non-nullable

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js (right):

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js:1060: * @param
{{ style: !CSSAgent.CSSStyle, styleSheetId: (string|undefined), origin:
!CSSAgent.StyleSheetOrigin}} payload
nit: excessive space

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js:1402:
setKeyText: function(newKeyText, userCallback)
Let's have it promisified right away!

[lushnikov, 2016-01-13 01:42:19]

lgtm

[samli, 2016-01-13 04:59:36]

pfeldman ptal @ core/inspector changes.

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:745:
* @param {?WebInspector.CSSStyleModel.MatchedStyleResult} matchedStyles
On 2016/01/13 at 01:42:18, lushnikov wrote:
> can we keep matchedStyles non-nullable? You can make method
isKeyframesSection() which checks the type of style.parentRule

I just tried this and think it's semantically wrong. The keyframe sections don't
rely on matched styles at all. If we make it non-nullable there's no way for a
tree element to know if it is overloaded or not.

Also semantically, StylesPropertySection shouldn't need to be self-aware about
keyframe sections.

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1181:
_isPropertyOverloaded: function(property)
On 2016/01/13 at 01:42:18, lushnikov wrote:
> you can avoid doing this if matchedStyles will be non-nullable

Ack, see above comment.

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js (right):

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js:1060: * @param
{{ style: !CSSAgent.CSSStyle, styleSheetId: (string|undefined), origin:
!CSSAgent.StyleSheetOrigin}} payload
On 2016/01/13 at 01:42:18, lushnikov wrote:
> nit: excessive space

Done.

https://codereview.chromium.org/1577723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSStyleModel.js:1402:
setKeyText: function(newKeyText, userCallback)
On 2016/01/13 at 01:42:18, lushnikov wrote:
> Let's have it promisified right away!

Done.

[pfeldman, 2016-01-13 17:43:58]

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(left):

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:56:
WebInspector.StylesSidebarPane.Events = {
This was done separately.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:252:
return Promise.all(promises).spread(this._innerRebuildUpdate.bind(this));
Did we just add another roundtrip into the general update workflow? We should
send them in a single payload.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:766:
// TODO(samli): Rename selector to header text.
Either do this or drop it.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1086:
if (this._matchedStyles && this._matchedStyles.isInherited(this._style)) {
How can there be no matched styles now?

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1183:
return !!this._matchedStyles && this._matchedStyles.propertyState(property) ===
WebInspector.CSSStyleModel.MatchedStyleResult.PropertyState.Overloaded;
ditto

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1801:
editingSelectorCommitted: function(element, newContent, oldContent, context,
moveDirection)
The name suggests it was copied from somewhere. Should we refactor the code
somehow to make it universal? Can we merge editing of the selector and of the
keyframe text into a single value editing workflow?

[samli, 2016-01-18 02:10:32]

PTAL

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(left):

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:56:
WebInspector.StylesSidebarPane.Events = {
On 2016/01/13 at 17:43:58, pfeldman wrote:
> This was done separately.

Ack.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:252:
return Promise.all(promises).spread(this._innerRebuildUpdate.bind(this));
On 2016/01/13 at 17:43:58, pfeldman wrote:
> Did we just add another roundtrip into the general update workflow? We should
send them in a single payload.

Done.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:766:
// TODO(samli): Rename selector to header text.
On 2016/01/13 at 17:43:58, pfeldman wrote:
> Either do this or drop it.

Done.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1086:
if (this._matchedStyles && this._matchedStyles.isInherited(this._style)) {
On 2016/01/13 at 17:43:58, pfeldman wrote:
> How can there be no matched styles now?

Reverted this.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1183:
return !!this._matchedStyles && this._matchedStyles.propertyState(property) ===
WebInspector.CSSStyleModel.MatchedStyleResult.PropertyState.Overloaded;
On 2016/01/13 at 17:43:58, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1577723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:1801:
editingSelectorCommitted: function(element, newContent, oldContent, context,
moveDirection)
On 2016/01/13 at 17:43:58, pfeldman wrote:
> The name suggests it was copied from somewhere. Should we refactor the code
somehow to make it universal? Can we merge editing of the selector and of the
keyframe text into a single value editing workflow?

Done.

[samli, 2016-01-18 02:10:44]

Patchset #6 (id:100001) has been deleted

[samli, 2016-01-20 21:26:16]

ping pfeldman

[pfeldman, 2016-01-21 19:35:27]

lgtm

[samli, 2016-01-21 23:05:02]

The CQ bit was checked by samli@chromium.org

[samli, 2016-01-21 23:05: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/1577723002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-01-21 23:05:54]

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

[commit-bot: I haz the power, 2016-01-22 00:15:46]

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

[commit-bot: I haz the power, 2016-01-22 00:15:47]

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

[samli, 2016-01-22 00:56:30]

The CQ bit was checked by samli@chromium.org

[samli, 2016-01-22 00:56:30]

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/1577723002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-01-22 00:58:02]

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

[commit-bot: I haz the power, 2016-01-22 02:35:56]

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

[commit-bot: I haz the power, 2016-01-22 02:35:57]

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

[samli, 2016-01-28 22:55:33]

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

[commit-bot: I haz the power, 2016-01-28 22:56:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577723002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577723002/140001

[commit-bot: I haz the power, 2016-01-29 00:24:06]

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

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

Dry run: This issue passed the CQ dry run.

[samli, 2016-01-29 00:29:20]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-01-29 00:33:25]

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

[commit-bot: I haz the power, 2016-01-29 00:40:25]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-29 00:41:23]

Description was changed from

==========
Devtools: Add editable keyframes to the styles sidebar pane

BUG=266276
==========

to

==========
Devtools: Add editable keyframes to the styles sidebar pane

BUG=266276

Committed: https://crrev.com/301c00ccc158984672abc7de19466e48619e81a5
Cr-Commit-Position: refs/heads/master@{#372224}
==========

[commit-bot: I haz the power, 2016-01-29 00:41:24]

Patchset 7 (id:??) landed as
https://crrev.com/301c00ccc158984672abc7de19466e48619e81a5
Cr-Commit-Position: refs/heads/master@{#372224}