DevTools: [CSS] Add CSS.editRangeInStyleSheetText() to the protocol

DevTools: [CSS] Add CSS.editRangeInStyleSheetText() to the
protocol

This patch adds method "editRangeInStyleSheetText" into CSS domain
of protocol.json. Also this doesn't simplify protocol on its own,
usages of methods setPropertyText and setSelectorText will be eventually 
substituted with the call to this method.

BUG=341506

[lushnikov, 2014-02-19 17:01:40]

please take a look

[apavlov, 2014-02-20 09:58:13]

https://codereview.chromium.org/172593003/diff/1/LayoutTests/http/tests/inspe...
File LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js
(right):

https://codereview.chromium.org/172593003/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:231:
InspectorTest.sendCommandOrDie = function(command, properties, callback) {
The name does not clearly describe the functionality. I believe something like
sendCommandAndBailOnError() could be more comprehensible.

https://codereview.chromium.org/172593003/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:236:
InspectorTest.log(msg.error.message);
I'd vote for ("ERROR: " + msg.error.message), since the message may
coincidentally turn out to be the same as the last message logged by a test.

https://codereview.chromium.org/172593003/diff/1/LayoutTests/inspector-protoc...
File LayoutTests/inspector-protocol/css/css-replace-range.html (right):

https://codereview.chromium.org/172593003/diff/1/LayoutTests/inspector-protoc...
LayoutTests/inspector-protocol/css/css-replace-range.html:22:
InspectorTest.sendCommandOrDie("CSS.replaceRangeInStyleSheetText", {
It is hard to follow. Let's dump all intermediate results? The suite also seems
to lack a text removal test (range replaced by empty string) and invalid range
replacement (line/column < 0 and line/column past the end of text/i-th line), as
well as invalid message parameters tests (since we are testing the protocol)

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

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:75: #include
"wtf/text/TextPosition.h"
This should not be here

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:841: void
InspectorCSSAgent::replaceRangeInStyleSheetText(ErrorString* errorString, const
String& styleSheetId, const RefPtr<JSONObject>& range, const String& text)
Is this going to be an undoable operation, like the rest of stylesheet
operations?

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:845: *errorString = "Style sheet
with id " + styleSheetId + " not found.";
We have a convention to never place periods at the end of error messages (to
easily handle them in the frontend)

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:874: TextPosition
startPosition(OrdinalNumber::fromZeroBasedInt(startLineNumber),
OrdinalNumber::fromZeroBasedInt(startColumn));
This code doesn't seem to belong to ICSSAgent (for which you had to open
lineEndings()). Instead, it should be encapsulated into
InspectorStyleSheet::replaceText(...) or similar.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:877: int startOffset =
startPosition.toOffsetPosition(*lineEndings.get()).zeroBasedInt();
You don't need .get() here, just (*lineEndings)

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:878: int endOffset =
endPosition.toOffsetPosition(*lineEndings.get()).zeroBasedInt();
ditto

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

https://codereview.chromium.org/172593003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:2650: { "name": "range", "$ref": "SourceRange",
"description": "Source range to be replaced in style sheet text." },
"stylesheet" for consistency with the rest of prose

https://codereview.chromium.org/172593003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:2651: { "name": "text", "type": "string",
"description": "Text that will replace given range in the style sheet text." }
ditto

https://codereview.chromium.org/172593003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:2653: "description": "Replace some range of
stylesheet with a new text, thereby invalidating all existing
<code>CSSStyleId</code>'s and <code>CSSRuleId</code>'s contained by this
stylesheet.",
Replace stylesheet text range with new text...

[apavlov, 2014-02-20 10:09:17]

Please also give a descriptive subject of the change, so that Adam Barth can
understand what it does without reading the description.

Something like "DevTools: [CSS] Add CSS.replaceRangeInStyleSheetText() to the
protocol"

[pfeldman, 2014-02-20 11:29:04]

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

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:841: void
InspectorCSSAgent::replaceRangeInStyleSheetText(ErrorString* errorString, const
String& styleSheetId, const RefPtr<JSONObject>& range, const String& text)
editRangeInStyleSheetText?

[lushnikov, 2014-02-20 14:17:03]

Please, take another look

https://codereview.chromium.org/172593003/diff/1/LayoutTests/http/tests/inspe...
File LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js
(right):

https://codereview.chromium.org/172593003/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:231:
InspectorTest.sendCommandOrDie = function(command, properties, callback) {
On 2014/02/20 09:58:14, apavlov wrote:
> The name does not clearly describe the functionality. I believe something like
> sendCommandAndBailOnError() could be more comprehensible.

Done.

https://codereview.chromium.org/172593003/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:236:
InspectorTest.log(msg.error.message);
On 2014/02/20 09:58:14, apavlov wrote:
> I'd vote for ("ERROR: " + msg.error.message), since the message may
> coincidentally turn out to be the same as the last message logged by a test.

Done.

https://codereview.chromium.org/172593003/diff/1/LayoutTests/inspector-protoc...
File LayoutTests/inspector-protocol/css/css-replace-range.html (right):

https://codereview.chromium.org/172593003/diff/1/LayoutTests/inspector-protoc...
LayoutTests/inspector-protocol/css/css-replace-range.html:22:
InspectorTest.sendCommandOrDie("CSS.replaceRangeInStyleSheetText", {
On 2014/02/20 09:58:14, apavlov wrote:
> It is hard to follow. Let's dump all intermediate results? The suite also
seems
> to lack a text removal test (range replaced by empty string) and invalid range
> replacement (line/column < 0 and line/column past the end of text/i-th line),
as
> well as invalid message parameters tests (since we are testing the protocol)

Done.

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

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:75: #include
"wtf/text/TextPosition.h"
On 2014/02/20 09:58:14, apavlov wrote:
> This should not be here

Done.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:841: void
InspectorCSSAgent::replaceRangeInStyleSheetText(ErrorString* errorString, const
String& styleSheetId, const RefPtr<JSONObject>& range, const String& text)
On 2014/02/20 09:58:14, apavlov wrote:
> Is this going to be an undoable operation, like the rest of stylesheet
> operations?

This is essentially a wrapper around setStyleSheetText, so it is undoable out of
the box.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:841: void
InspectorCSSAgent::replaceRangeInStyleSheetText(ErrorString* errorString, const
String& styleSheetId, const RefPtr<JSONObject>& range, const String& text)
On 2014/02/20 11:29:05, pfeldman wrote:
> editRangeInStyleSheetText?

Done.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:845: *errorString = "Style sheet
with id " + styleSheetId + " not found.";
On 2014/02/20 09:58:14, apavlov wrote:
> We have a convention to never place periods at the end of error messages (to
> easily handle them in the frontend)

Done.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:874: TextPosition
startPosition(OrdinalNumber::fromZeroBasedInt(startLineNumber),
OrdinalNumber::fromZeroBasedInt(startColumn));
On 2014/02/20 09:58:14, apavlov wrote:
> This code doesn't seem to belong to ICSSAgent (for which you had to open
> lineEndings()). Instead, it should be encapsulated into
> InspectorStyleSheet::replaceText(...) or similar.

InspectorStyleSheet soaked all the logic in its lineNumberAndColumnToOffset
method - done.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:877: int startOffset =
startPosition.toOffsetPosition(*lineEndings.get()).zeroBasedInt();
On 2014/02/20 09:58:14, apavlov wrote:
> You don't need .get() here, just (*lineEndings)

Done.

https://codereview.chromium.org/172593003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorCSSAgent.cpp:878: int endOffset =
endPosition.toOffsetPosition(*lineEndings.get()).zeroBasedInt();
On 2014/02/20 09:58:14, apavlov wrote:
> ditto

Done.

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

https://codereview.chromium.org/172593003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:2650: { "name": "range", "$ref": "SourceRange",
"description": "Source range to be replaced in style sheet text." },
On 2014/02/20 09:58:14, apavlov wrote:
> "stylesheet" for consistency with the rest of prose

Done.

https://codereview.chromium.org/172593003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:2651: { "name": "text", "type": "string",
"description": "Text that will replace given range in the style sheet text." }
On 2014/02/20 09:58:14, apavlov wrote:
> ditto

Done.

https://codereview.chromium.org/172593003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:2653: "description": "Replace some range of
stylesheet with a new text, thereby invalidating all existing
<code>CSSStyleId</code>'s and <code>CSSRuleId</code>'s contained by this
stylesheet.",
On 2014/02/20 09:58:14, apavlov wrote:
> Replace stylesheet text range with new text...

Done.

[apavlov, 2014-02-20 14:21:23]

https://codereview.chromium.org/172593003/diff/100001/Source/core/inspector/I...
File Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/172593003/diff/100001/Source/core/inspector/I...
Source/core/inspector/InspectorStyleSheet.cpp:1560: bool
InspectorStyleSheet::lineNumberAndColumnToOffset(int lineNumber, int
columnNumber, unsigned* offset) const
int -> unsigned

https://codereview.chromium.org/172593003/diff/100001/Source/core/inspector/I...
Source/core/inspector/InspectorStyleSheet.cpp:1563: if (lineNumber < 0 ||
(size_t)lineNumber >= endings->size())
Use static_cast<unsigned>() in ICSSAgent

[lushnikov, 2014-02-20 14:37:34]

https://codereview.chromium.org/172593003/diff/100001/Source/core/inspector/I...
File Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/172593003/diff/100001/Source/core/inspector/I...
Source/core/inspector/InspectorStyleSheet.cpp:1560: bool
InspectorStyleSheet::lineNumberAndColumnToOffset(int lineNumber, int
columnNumber, unsigned* offset) const
On 2014/02/20 14:21:24, apavlov wrote:
> int -> unsigned

Done.

https://codereview.chromium.org/172593003/diff/100001/Source/core/inspector/I...
Source/core/inspector/InspectorStyleSheet.cpp:1563: if (lineNumber < 0 ||
(size_t)lineNumber >= endings->size())
On 2014/02/20 14:21:24, apavlov wrote:
> Use static_cast<unsigned>() in ICSSAgent

Don't need this anymore

[apavlov, 2014-02-21 14:37:54]

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/css/css-edit-range.html (right):

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/css-edit-range.html:82: function
testOutOfBoundsRangeParametersNoCrash(next)
What about the range:
- ending on the last line past the last character
- starting on a non-existent line
- starting on the last line, past the last character

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/css-edit-range.html:115: function
ensureProtocolError(options, next)
"ensure..." methods usually initialize something lazily. In this case the
"check" prefix works best.

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/css-edit-range.html:123:
InspectorTest.log("ERROR: protocol method call did not raise expected error.
Instead, the following message received: " + JSON.stringify(message));
received -> was received

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:844: *errorString = "Style sheet
with id " + styleSheetId + " not found";
"Stylesheet" ?

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:851: if
(!range->getNumber("startLine", &startLineNumber)) {
This could be extracted:

static bool rangeComponent(ErrorString* errorString, const RefPtr<JSONObject>&
range, const String& component, unsigned& result)
{
    if (!range->getNumber(component, &result) || result < 0) {
        *errorString = component + " must be a non-negative integer";
        return false;
    }
    return true;
}

...

if (!rangeComponent(errorString, range, "startLine", startLineNumber) ||
        !rangeComponent(errorString, range, "startColumn", startColumn) ||
        !rangeComponent(errorString, range, "endLine", endLineNumber) ||
        !rangeComponent(errorString, range, "endColumn", endColumn))
    return;

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:852: *errorString = "range.startLine
must be an integer value";
"...must be an integer" is sufficient (since "integer" is most commonly used as
a noun)

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:879: bool successConvertion =
inspectorStyleSheet->lineNumberAndColumnToOffset(startLineNumber, startColumn,
&startOffset);
successConvertion -> success (or something along the lines of
|conversionSuccessful|)

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:880: successConvertion =
successConvertion &&
inspectorStyleSheet->lineNumberAndColumnToOffset(endLineNumber, endColumn,
&endOffset);
if (successConversion)
    successConversion = ...

to avoid doing unnecessary work

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:881: if (!successConvertion ||
startOffset > oldText.length() || endOffset > oldText.length()) {
it does not make any sense to extract a part of lineNumberAndColumnToOffset()
into here

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:887: *errorString = "Range start
must preceed its end";
"precede"

or, better yet,

"Range start must not succeed its end", since the range may be empty

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
File Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorStyleSheet.cpp:1566: *offset =
position.toOffsetPosition(*endings).zeroBasedInt();
This should check for columnNumber-is-beyond-last-char-in-last-line here, too

https://codereview.chromium.org/172593003/diff/150001/Source/devtools/front_e...
File Source/devtools/front_end/StylesSidebarPane.js (right):

https://codereview.chromium.org/172593003/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/StylesSidebarPane.js:183: _forcedPseudoClasses:
function()
how is this change related to your patch?

[lushnikov, 2014-02-21 14:57:34]

https://codereview.chromium.org/172593003/diff/150001/Source/devtools/front_e...
File Source/devtools/front_end/StylesSidebarPane.js (right):

https://codereview.chromium.org/172593003/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/StylesSidebarPane.js:183: _forcedPseudoClasses:
function()
On 2014/02/21 14:37:55, apavlov wrote:
> how is this change related to your patch?

that's a drive-by :)

[lushnikov, 2014-02-23 16:07:07]

Thanks for another round of comments!
Addressed 'em all

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/css/css-edit-range.html (right):

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/css-edit-range.html:82: function
testOutOfBoundsRangeParametersNoCrash(next)
On 2014/02/21 14:37:55, apavlov wrote:
> What about the range:
> - ending on the last line past the last character
> - starting on a non-existent line
> - starting on the last line, past the last character

Done.

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/css-edit-range.html:115: function
ensureProtocolError(options, next)
On 2014/02/21 14:37:55, apavlov wrote:
> "ensure..." methods usually initialize something lazily. In this case the
> "check" prefix works best.

Done.

https://codereview.chromium.org/172593003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/css-edit-range.html:123:
InspectorTest.log("ERROR: protocol method call did not raise expected error.
Instead, the following message received: " + JSON.stringify(message));
On 2014/02/21 14:37:55, apavlov wrote:
> received -> was received

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:844: *errorString = "Style sheet
with id " + styleSheetId + " not found";
On 2014/02/21 14:37:55, apavlov wrote:
> "Stylesheet" ?

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:851: if
(!range->getNumber("startLine", &startLineNumber)) {
On 2014/02/21 14:37:55, apavlov wrote:
> This could be extracted:
> 
> static bool rangeComponent(ErrorString* errorString, const RefPtr<JSONObject>&
> range, const String& component, unsigned& result)
> {
>     if (!range->getNumber(component, &result) || result < 0) {
>         *errorString = component + " must be a non-negative integer";
>         return false;
>     }
>     return true;
> }
> 
> ...
> 
> if (!rangeComponent(errorString, range, "startLine", startLineNumber) ||
>         !rangeComponent(errorString, range, "startColumn", startColumn) ||
>         !rangeComponent(errorString, range, "endLine", endLineNumber) ||
>         !rangeComponent(errorString, range, "endColumn", endColumn))
>     return;

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:851: if
(!range->getNumber("startLine", &startLineNumber)) {
On 2014/02/21 14:37:55, apavlov wrote:
> This could be extracted:
> 
> static bool rangeComponent(ErrorString* errorString, const RefPtr<JSONObject>&
> range, const String& component, unsigned& result)
> {
>     if (!range->getNumber(component, &result) || result < 0) {
>         *errorString = component + " must be a non-negative integer";
>         return false;
>     }
>     return true;
> }
> 
> ...
> 
> if (!rangeComponent(errorString, range, "startLine", startLineNumber) ||
>         !rangeComponent(errorString, range, "startColumn", startColumn) ||
>         !rangeComponent(errorString, range, "endLine", endLineNumber) ||
>         !rangeComponent(errorString, range, "endColumn", endColumn))
>     return;

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:852: *errorString = "range.startLine
must be an integer value";
On 2014/02/21 14:37:55, apavlov wrote:
> "...must be an integer" is sufficient (since "integer" is most commonly used
as
> a noun)

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:879: bool successConvertion =
inspectorStyleSheet->lineNumberAndColumnToOffset(startLineNumber, startColumn,
&startOffset);
On 2014/02/21 14:37:55, apavlov wrote:
> successConvertion -> success (or something along the lines of
> |conversionSuccessful|)

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:880: successConvertion =
successConvertion &&
inspectorStyleSheet->lineNumberAndColumnToOffset(endLineNumber, endColumn,
&endOffset);
On 2014/02/21 14:37:55, apavlov wrote:
> if (successConversion)
>     successConversion = ...
> 
> to avoid doing unnecessary work

This is essentially the same due-to "&&" operation computation short-cutting,
thus no unnecessary work is done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:881: if (!successConvertion ||
startOffset > oldText.length() || endOffset > oldText.length()) {
On 2014/02/21 14:37:55, apavlov wrote:
> it does not make any sense to extract a part of lineNumberAndColumnToOffset()
> into here

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:887: *errorString = "Range start
must preceed its end";
On 2014/02/21 14:37:55, apavlov wrote:
> "precede"
> 
> or, better yet,
> 
> "Range start must not succeed its end", since the range may be empty

Done.

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
File Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/172593003/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorStyleSheet.cpp:1566: *offset =
position.toOffsetPosition(*endings).zeroBasedInt();
On 2014/02/21 14:37:55, apavlov wrote:
> This should check for columnNumber-is-beyond-last-char-in-last-line here, too

Added check that columnNumber is beyond the amount of characters in the line.

https://codereview.chromium.org/172593003/diff/150001/Source/devtools/front_e...
File Source/devtools/front_end/StylesSidebarPane.js (right):

https://codereview.chromium.org/172593003/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/StylesSidebarPane.js:183: _forcedPseudoClasses:
function()
On 2014/02/21 14:37:55, apavlov wrote:
> how is this change related to your patch?

Removed this change from patch.

[apavlov, 2014-02-23 17:43:06]

Nice! LGTM

[lushnikov, 2014-02-24 08:49:57]

On 2014/02/23 17:43:06, apavlov wrote:
> Nice! LGTM

Great! Thanks for the review!

[lushnikov, 2014-02-24 08:54:14]

eseidel: Please review changes in wtf/text
vsevik: Please review protocol.json changes

[vsevik, 2014-02-25 10:42:41]

https://codereview.chromium.org/172593003/diff/260001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/172593003/diff/260001/Source/devtools/protoco...
Source/devtools/protocol.json:2656: "description": "Replace stylesheet text
range with a new text, thereby invalidating all existing
<code>CSSStyleId</code>'s and <code>CSSRuleId</code>'s contained by this
stylesheet.",
Replace stylesheet text range with a new text. This will invalidate all existing
<code>CSSStyleId</code>s and <code>CSSRuleId</code>s contained by this
stylesheet.

[lushnikov, 2014-02-25 11:05:44]

https://codereview.chromium.org/172593003/diff/260001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/172593003/diff/260001/Source/devtools/protoco...
Source/devtools/protocol.json:2656: "description": "Replace stylesheet text
range with a new text, thereby invalidating all existing
<code>CSSStyleId</code>'s and <code>CSSRuleId</code>'s contained by this
stylesheet.",
On 2014/02/25 10:42:42, vsevik wrote:
> Replace stylesheet text range with a new text. This will invalidate all
existing
> <code>CSSStyleId</code>s and <code>CSSRuleId</code>s contained by this
> stylesheet.

Done.

[vsevik, 2014-02-25 11:20:48]

lgtm

https://chromiumcodereview.appspot.com/172593003/diff/320001/LayoutTests/http...
File LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js
(right):

https://chromiumcodereview.appspot.com/172593003/diff/320001/LayoutTests/http...
LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:231:
InspectorTest.sendCommandAndBailOnError = function(command, properties,
callback) {
{ on the next line

https://chromiumcodereview.appspot.com/172593003/diff/320001/Source/devtools/...
File Source/devtools/protocol.json (right):

https://chromiumcodereview.appspot.com/172593003/diff/320001/Source/devtools/...
Source/devtools/protocol.json:2657: "hidden": true
We do not mark CSS domain methods as hidden (the whole domain is hidden)

[lushnikov, 2014-02-25 11:31:59]

Nice, thank you for the review

https://chromiumcodereview.appspot.com/172593003/diff/320001/LayoutTests/http...
File LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js
(right):

https://chromiumcodereview.appspot.com/172593003/diff/320001/LayoutTests/http...
LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:231:
InspectorTest.sendCommandAndBailOnError = function(command, properties,
callback) {
On 2014/02/25 11:20:48, vsevik wrote:
> { on the next line

Done.

https://chromiumcodereview.appspot.com/172593003/diff/320001/Source/devtools/...
File Source/devtools/protocol.json (right):

https://chromiumcodereview.appspot.com/172593003/diff/320001/Source/devtools/...
Source/devtools/protocol.json:2657: "hidden": true
On 2014/02/25 11:20:48, vsevik wrote:
> We do not mark CSS domain methods as hidden (the whole domain is hidden)

Done.

[lushnikov, 2014-03-08 14:35:45]

Please, take a look.

This change implements the "editRange" version of method that does OM edits.

[pfeldman, 2014-03-13 05:43:00]

not lgtm. this change adds complexity with no clear long term plan on removing
it.

[vsevik, 2014-03-13 10:44:50]

Publishing my comments to the implementation in case we reuse some of this code.

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
File LayoutTests/inspector-protocol/css/css-edit-range.html (right):

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/css-edit-range.html:38: },
InspectorTest.undoAndNext(next));
Can we make a simple InspectorTest.domUndo(callback) and pass
InspectorTest.domUndo.bind(InspectorTest, next) here?

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
File LayoutTests/inspector-protocol/css/css-protocol-test.js (right):

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/css-protocol-test.js:81: function
dumpProperties(properties, indent)
We could probably reuse it on dumpRuleMatch

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
File LayoutTests/inspector-protocol/css/edit-range-utils.js (right):

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/edit-range-utils.js:1: var
initialize_EditRangeHelpers = function() {
This could be put in css-protocol-test.js

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/edit-range-utils.js:34:
InspectorTest.undoAndNext = function(next)
This seems to complicated.

https://chromiumcodereview.appspot.com/172593003/diff/470001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/edit-range-utils.js:43:
InspectorTest.checkProtocolError = function(styleSheetId, options, callback)
InspectorTest.editStyleSheetExpectError

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/css/...
File Source/core/css/CSSPropertySourceData.cpp (right):

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/css/...
Source/core/css/CSSPropertySourceData.cpp:112: unsigned
CSSPropertySourceData::hash() const
Can we replace it with equals?

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorCSSAgent.cpp:947:
editAction->setExternalEditRangeResult(&editRangeResult);
Wouldn't it be simpler to add an actionResult getter on this action instead?

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
File Source/core/inspector/InspectorStyleSheet.cpp (right):

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorStyleSheet.cpp:945: static bool
verifyEditLocalizedToRule(const ParsedStyleSheet* originalData, const
ParsedStyleSheet* modifiedData, const unsigned ruleIndex, ExceptionState&
exceptionState)
This method does something different from what its name suggests and this is bad
:)

Let's check text before and after rule matches in both stylesheets.

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorStyleSheet.cpp:1042: size_t minSize =
std::min(originalSize, modifiedSize);
size_t minSize = originalSize;
So let's remove it.

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorStyleSheet.cpp:1044: while (leadingCongruent <
minSize) {
Can we use text for this instead?

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorStyleSheet.cpp:1096: continue;
if (!editRange intersects with rule range)
    continue;
if (!verifyEditLocalizedToRule(m_parsedStyleSheet, editedParsedData.get(), i,
exceptio
      nState))
    return false;

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorStyleSheet.cpp:1118: return
handleStyleSheetEditScope(m_parsedStyleSheet, editedParsedData.get(),
editRangeResult, exceptionState);
return false;

https://chromiumcodereview.appspot.com/172593003/diff/470001/Source/core/insp...
Source/core/inspector/InspectorStyleSheet.cpp:1885: if (m_styleText.isEmpty()) {
This is not needed anymore.