DevTools: Split creating inspector stylesheet and adding a new rule into stylesheet in protocol.

DevTools: Split creating inspector stylesheet and adding a new rule into stylesheet in protocol.

R=lushnikov@chromium.org, pfeldman

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

[vsevik, 2014-02-27 12:56:21]

PTAL

[lushnikov, 2014-02-28 11:55:59]

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
File LayoutTests/inspector/elements/styles/edit-inspector-stylesheet.html
(left):

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
LayoutTests/inspector/elements/styles/edit-inspector-stylesheet.html:21:
InspectorTest.waitForStyles("inspected", stylesReceived, true);
Is this step skipped intentionally? Why?

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
File LayoutTests/inspector/elements/styles/styles-new-API.html (right):

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
LayoutTests/inspector/elements/styles/styles-new-API.html:302:
InspectorTest.addResult("error: " + error);
InspectorTest.completeTest() is needed here as well

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.cpp:82: using
WebCore::TypeBuilder::CSS::StyleSheetId;
there's a single use of this type in the file - can we inline it?

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.h:133: virtual void
createViaInspectorStyleSheet(ErrorString*, const String& frameId,
TypeBuilder::CSS::StyleSheetId* outStyleSheetId) OVERRIDE;
all other methods returns values via RefPtr's - why using raw pointers here?

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/CSSStyleModel.js (right):

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/CSSStyleModel.js:317: requestViaInspectorStylesheet:
function(node, callback)
ensureViaInspectorStyleSheet?

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/CSSStyleModel.js:324: viaInspectorStyleSheetHeader =
styleSheetHeader;
we don't need viaInspectorStyleSheetHeader var - let's call
callback(styleSheetHeader) right here

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

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:2674: "name": "createViaInspectorStyleSheet",
I would rather name it "createNewStyleSheet" to avoid introducing obscure entity
"ViaInspectorStyleSheet" in the protocol.

[vsevik, 2014-02-28 14:35:27]

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
File LayoutTests/inspector/elements/styles/edit-inspector-stylesheet.html
(left):

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
LayoutTests/inspector/elements/styles/edit-inspector-stylesheet.html:21:
InspectorTest.waitForStyles("inspected", stylesReceived, true);
addNewRule will callback after the styles are received.

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
File LayoutTests/inspector/elements/styles/styles-new-API.html (right):

https://codereview.chromium.org/177963004/diff/20001/LayoutTests/inspector/el...
LayoutTests/inspector/elements/styles/styles-new-API.html:302:
InspectorTest.addResult("error: " + error);
Done

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.cpp:82: using
WebCore::TypeBuilder::CSS::StyleSheetId;
Done

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/177963004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.h:133: virtual void
createViaInspectorStyleSheet(ErrorString*, const String& frameId,
TypeBuilder::CSS::StyleSheetId* outStyleSheetId) OVERRIDE;
String is not RefCounted

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/CSSStyleModel.js (right):

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/CSSStyleModel.js:317: requestViaInspectorStylesheet:
function(node, callback)
I prefer requestViaInspectorStyleSheet.

https://codereview.chromium.org/177963004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/CSSStyleModel.js:324: viaInspectorStyleSheetHeader =
styleSheetHeader;
On 2014/02/28 11:55:59, lushnikov wrote:
> we don't need viaInspectorStyleSheetHeader var - let's call
> callback(styleSheetHeader) right here

Done.

[vsevik, 2014-02-28 14:36:01]

>
https://codereview.chromium.org/177963004/diff/20001/Source/devtools/protocol...
> Source/devtools/protocol.json:2674: "name": "createViaInspectorStyleSheet",
> I would rather name it "createNewStyleSheet" to avoid introducing obscure
entity
> "ViaInspectorStyleSheet" in the protocol.

Renamed to createStyleSheet

[lushnikov, 2014-02-28 15:02:01]

On 2014/02/28 14:36:01, vsevik wrote:
> >
>
https://codereview.chromium.org/177963004/diff/20001/Source/devtools/protocol...
> > Source/devtools/protocol.json:2674: "name": "createViaInspectorStyleSheet",
> > I would rather name it "createNewStyleSheet" to avoid introducing obscure
> entity
> > "ViaInspectorStyleSheet" in the protocol.
> 
> Renamed to createStyleSheet

lgtm given protocol test

[lushnikov, 2014-03-03 12:43:08]

https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
File LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html
(right):

https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html:12:
setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000);
1. not needed
2. bad indentation

https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html:29:
return;
don't we want to die here?

InspectorTest.log("Error: unexpected stylesheet");
InspectorTest.completeTest();

https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html:37:
InspectorTest.log("Adding a rule to the existing stylesheet.");
FYI: there's a InspectorTest.runTestSuite method which prints function names
upon executing them.

https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
File LayoutTests/inspector-protocol/css/css-protocol-test.js (right):

https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/css/css-protocol-test.js:4:
InspectorTest.sendDomainCommand = function(domain, method, parameters, callback)
consider using sendCommandOrBailOnError

[vsevik, 2014-03-03 13:58:51]

PTAL

[vsevik, 2014-03-03 16:48:14]

>
https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
> LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html:12:
> setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000);
> 1. not needed
> 2. bad indentation
Removed.

>
https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
> LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html:29:
> return;
> don't we want to die here?
> 
> InspectorTest.log("Error: unexpected stylesheet");
> InspectorTest.completeTest();
No, this is a valid situation in this test (we create a stylesheet later).


>
https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
> LayoutTests/inspector-protocol/css/css-create-stylesheet-and-add-rule.html:37:
> InspectorTest.log("Adding a rule to the existing stylesheet.");
> FYI: there's a InspectorTest.runTestSuite method which prints function names
> upon executing them.
This is irrelevant here.

>
https://codereview.chromium.org/177963004/diff/60001/LayoutTests/inspector-pr...
> LayoutTests/inspector-protocol/css/css-protocol-test.js:4:
> InspectorTest.sendDomainCommand = function(domain, method, parameters,
callback)
> consider using sendCommandOrBailOnError
Done.

[vsevik, 2014-03-03 16:48:50]

Please take another look, I can not upload this patch right now due to git cl
issue, but the only thing changed is the removal of setTimeout in test.

[lushnikov, 2014-03-04 08:22:35]

lgtm with nit

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

https://chromiumcodereview.appspot.com/177963004/diff/120001/LayoutTests/insp...
LayoutTests/inspector-protocol/css/css-protocol-test.js:29:
InspectorTest.requestNodeId = function(selector, callback)
Not really sure these DOM-domain and Page-domain commands belong to CSS
initializers, looks like they might fit good all over the place. How about
moving them to inspector-protocol-test.js?

[vsevik, 2014-03-04 08:53:56]

The CQ bit was checked by vsevik@chromium.org

[commit-bot: I haz the power, 2014-03-04 08:54:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vsevik@chromium.org/177963004/40002

[vsevik, 2014-03-04 15:00:19]

Committed patchset #9 manually as r168388 (presubmit successful).

[pfeldman, 2014-03-17 06:10:49]

https://codereview.chromium.org/177963004/diff/40002/LayoutTests/inspector/el...
File LayoutTests/inspector/elements/styles/styles-new-API.html (right):

https://codereview.chromium.org/177963004/diff/40002/LayoutTests/inspector/el...
LayoutTests/inspector/elements/styles/styles-new-API.html:301: if (error) {
Note that nothing prevents from executing rebuildUpdate in the middle of these
operations.