DevTools: [SSP] Implement adding new rule in user stylesheet

DevTools: [SSP] Implement adding new rule in user stylesheet

This patch implements an "add new rule" functionality to add a style rule in a user stylesheet. It is also possible to
add rule inside media query rules.

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

[lushnikov, 2014-08-11 15:19:13]

The screenshot is attached to the bug, though it's not that interesting.
Please, take a look.

[vsevik, 2014-08-12 15:53:54]

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File LayoutTests/inspector-protocol/css/css-add-rule.html (right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/css-add-rule.html:51: ruleText: "#test {
content: 'EDITED'; }",
Can we test a scenario where we add some '\n' symbols outside of the rule?

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon-expected.txt
(right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon-expected.txt:25:
div#other  { (inspector-stylesheet:5 -> inspector-stylesheet:5:1)
This change seems incorrect - the \n adding strategy should not have changed

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html
(right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html:51:
InspectorTest.selectNodeAndWaitForStyles("inspected", step5);
Nit: It is possible that you don't need to select node here - I believe this was
done to implicitly steal focus from the edit field.

[lushnikov, 2014-08-12 15:57:02]

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon-expected.txt
(right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon-expected.txt:25:
div#other  { (inspector-stylesheet:5 -> inspector-stylesheet:5:1)
On 2014/08/12 15:53:54, vsevik wrote:
> This change seems incorrect - the \n adding strategy should not have changed 

It changed. We do not add newlines to separate rules currently; but we will with
this change.

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html
(right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html:51:
InspectorTest.selectNodeAndWaitForStyles("inspected", step5);
On 2014/08/12 15:53:54, vsevik wrote:
> Nit: It is possible that you don't need to select node here - I believe this
was
> done to implicitly steal focus from the edit field.

We dump the styles of the element in step5()

[vsevik, 2014-08-12 16:06:01]

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
File Source/devtools/front_end/elements/StylesSidebarPane.js (right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
Source/devtools/front_end/elements/StylesSidebarPane.js:833: this.expand();
Why is this needed?

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
Source/devtools/front_end/elements/StylesSidebarPane.js:1955: var rulePrefix =
this._ruleLocation.startLine === 0 && this._ruleLocation.startColumn === 0 ? ""
: "\n\n";
Could we be a bit smarter and make sure that we have exactly two '\n' between
the rule we insert after and our new rule ? (at least that we don't add more
than that)

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
Source/devtools/front_end/elements/StylesSidebarPane.js:1956: var ruleText =
rulePrefix + newContent + " {}";
Maybe " {\n}\n" ? 
We could also take indent into account

[vsevik, 2014-08-13 06:47:06]

lgtm

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html
(right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html:51:
InspectorTest.selectNodeAndWaitForStyles("inspected", step5);
I mean you select 'other' again then and dump its styles. We could probably
avoid one extra node selection. This is minor though.

[lushnikov, 2014-08-13 09:10:49]

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File LayoutTests/inspector-protocol/css/css-add-rule.html (right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/css-add-rule.html:51: ruleText: "#test {
content: 'EDITED'; }",
On 2014/08/12 15:53:54, vsevik wrote:
> Can we test a scenario where we add some '\n' symbols outside of the rule?

Done.

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
File LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html
(right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/LayoutTests/insp...
LayoutTests/inspector/elements/styles/styles-add-new-rule-colon.html:51:
InspectorTest.selectNodeAndWaitForStyles("inspected", step5);
This also waits for new styles.. I'd like to leave as-is in the patch.

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
File Source/devtools/front_end/elements/StylesSidebarPane.js (right):

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
Source/devtools/front_end/elements/StylesSidebarPane.js:833: this.expand();
On 2014/08/12 16:06:01, vsevik wrote:
> Why is this needed?

Acknowledged.

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
Source/devtools/front_end/elements/StylesSidebarPane.js:1955: var rulePrefix =
this._ruleLocation.startLine === 0 && this._ruleLocation.startColumn === 0 ? ""
: "\n\n";
As per offline discussion, this is sufficient for general case.

https://chromiumcodereview.appspot.com/441873010/diff/130001/Source/devtools/...
Source/devtools/front_end/elements/StylesSidebarPane.js:1956: var ruleText =
rulePrefix + newContent + " {}";
Discussed offline.

[lushnikov, 2014-08-13 09:10:57]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2014-08-13 09:11:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lushnikov@chromium.org/441873010/150001

[commit-bot: I haz the power, 2014-08-13 09:47:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22493)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/41040)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46257)

[commit-bot: I haz the power, 2014-08-13 09:54:19]

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

[commit-bot: I haz the power, 2014-08-13 09:54:19]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[lushnikov, 2014-08-13 10:09:47]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2014-08-13 10:10:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lushnikov@chromium.org/441873010/170001

[commit-bot: I haz the power, 2014-08-13 11:09:22]

Change committed as 180164