[webkit-changes] [58748] trunk/WebCore

[Alexander Pavlov]

Hey Nikolas,

The primary reason behind this change was the complete
destruction/re-creation of InspectorDOMAgent on every frontend dis/connect.
InspectorCSSStore only changes the lifetime of some maps but is still
private to InspectorDOMAgent, which is why no behaviour (except reset()) has
been extracted. Now that I'm looking at the code, some use-cases do seem
reasonable to be extracted as checker/accessor methods, and since I'm
working on these classes at the moment, I will brush up InspectorCSSStore in
the subsequent change. Thanks for the suggestion!

On Tue, May 4, 2010 at 2:58 PM, Nikolas Zimmermann <
zimmermann at physik.rwth-aachen.de> wrote:

> Good evening Alexander,
>
> just had a quick glance over your last refactoring patch.
> I don't see the point in creating a new struct and just delegating the
> calls into the InspectorCSSStore, to access public variables.
> The code in InspectorDOMAgent is harder to read now. Why don't you
> encapsulate this properly?
>
> +struct InspectorCSSStore {
> +    InspectorCSSStore();
> +    ~InspectorCSSStore();
> +    void reset();
> +
> +    StyleToIdMap styleToId;
> +    IdToStyleMap idToStyle;
> +    RuleToIdMap ruleToId;
> +    IdToRuleMap idToRule;
> +    IdToDisabledStyleMap idToDisabledStyle;
> +    RefPtr<CSSStyleSheet> inspectorStyleSheet;
> +};
> +
>
>   void InspectorDOMAgent::applyStyleText(long callId, long styleId, const String& styleText, const String& propertyName) {-    IdToStyleMap::iterator it = m_idToStyle.find(styleId);
> -    if (it == m_idToStyle.end()) {+    IdToStyleMap::iterator it = cssStore()->idToStyle.find(styleId);
> +    if (it == cssStore()->idToStyle.end()) {         m_frontend->didApplyStyleText(callId, false, ScriptValue::undefined(), m_frontend->newScriptArray());         return;
>
> +    IdToDisabledStyleMap::iterator disabledIt = cssStore()->idToDisabledStyle.find(styleId);
> +    if (disabledIt == cssStore()->idToDisabledStyle.end())
> +        disabledIt = cssStore()->idToDisabledStyle.set(styleId, DisabledStyleDeclaration()).first;
>
> Just selected some examples, code like the one above could go into the
> InspectorCSSStore directly.
> Note that I'm not familiar at all with the Inspector code, but it looks
> like this more convoluted then it needs to be :-)
>
> Cheers,
> Niko
>
>
>


-- 
-alexander
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-changes/attachments/20100504/6e9f080c/attachment.html>