[webkit-reviews] review granted: [Bug 38906] Expose CSS rule body start/end offsets in the parent stylesheet : [Attachment 57009] [PATCH] InspectorCSSStore: struct -> class

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 38906: Expose CSS rule body start/end offsets in the parent stylesheet
https://bugs.webkit.org/show_bug.cgi?id=38906

Attachment 57009: [PATCH] InspectorCSSStore: struct -> class
https://bugs.webkit.org/attachment.cgi?id=57009&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Provided Dave's Ok with previous version, I am r+ing this. Please see notes and
fix things prior to landing though. Thanks!

=======

Now that the CSSStore is getting a solid API, it looks more like
InspectorCSSAgent. We should rename it in one of subsequent changes.

WebCore/inspector/InspectorCSSStore.cpp:83
 +	while (i < originalRuleListLength) {
Please consider extracting this into a method.



WebCore/inspector/InspectorCSSStore.cpp:95
 +	    for (InspectorController::ResourcesMap::const_iterator resIt =
m_inspectorController->resources().begin(); resIt !=
m_inspectorController->resources().end(); ++resIt) {
I would introduce public InspectorController::resourceForURL(const String&) and
use it here. One thing that bothers me is whether we can rely upon url matching
here. CSS is often static though, so we should be Ok for now.

WebCore/inspector/InspectorCSSStore.cpp:100
 +		    p.parseSheet(newStyleSheet.get(),
resIt->second->sourceString(), offsetVector);
This might be blank in some cases. Will your code handle it well?

WebCore/inspector/InspectorDOMAgent.cpp:1082
 +	CSSStyleSheet* inspectorStyleSheet = cssStore()->inspectorStyleSheet();

This whole logic can be incapsulated given that you pass node into the
inspectorStyleSheet getter, right? Also note, that for different iframes we
will use different inspector stylesheets, so you can't store single reference
in CSS store. Please either fix here or file a bug.