DevTools: never iterate over css source data and cssom rules in the same loop.

Source/core/inspector/InspectorStyleSheet.cpp

 /*
  * Copyright (C) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
  *     notice, this list of conditions and the following disclaimer in the
@@ skipping 889 matching lines @@
 
     RefPtrWillBeRawPtr<CSSMediaRule>  mediaRule = InspectorCSSAgent::asCSSMediaRule(rule.get());
     mediaRule->media()->setMediaText(text);
 
     replaceText(sourceData->ruleHeaderRange, text, newRange, oldText);
     onStyleSheetTextChanged();
 
     return mediaRule;
 }
 
-unsigned InspectorStyleSheet::ruleIndexBySourceRange(CSSMediaRule* parentMediaRule, const SourceRange& sourceRange)
+RefPtrWillBeRawPtr<CSSRuleSourceData> InspectorStyleSheet::ruleAfterSourceRange(const SourceRange& sourceRange)

[lushnikov, 2015/06/25 15:33:08]

ruleAfterSourceRange is a confusing name given its return type.
Lets have ::ruleSourceDataAfterSourceRange

[pfeldman, 2015/06/25 16:10:36]

Acknowledged.

 {
     ASSERT(m_sourceData);
     unsigned index = 0;
-    for (size_t i = 0; i < m_flatRules.size() && i < m_sourceData->size(); ++i) {
-        RefPtrWillBeRawPtr<CSSRule> rule = m_flatRules.at(i);
-        if (rule->parentRule() != parentMediaRule)
-            continue;
-        RefPtrWillBeRawPtr<CSSRuleSourceData> ruleSourceData = m_sourceData->at(i);
-        if (ruleSourceData->ruleBodyRange.end < sourceRange.start)
-            ++index;
+    for (; index < m_sourceData->size(); ++index) {
+        RefPtrWillBeRawPtr<CSSRuleSourceData> sd = m_sourceData->at(index);
+        if (sd->ruleHeaderRange.start >= sourceRange.end)
+            break;
     }
-    return index;
+    return index < m_sourceData->size() ? m_sourceData->at(index) : nullptr;
 }
 
-CSSStyleRule* InspectorStyleSheet::insertCSSOMRuleInStyleSheet(const SourceRange& sourceRange, const String& ruleText, ExceptionState& exceptionState)
+CSSStyleRule* InspectorStyleSheet::insertCSSOMRuleInStyleSheet(CSSRule* insertBefore, const String& ruleText, ExceptionState& exceptionState)

[lushnikov, 2015/06/25 15:33:08]

isn't insertBefore a CSSStyleRule actually?

[pfeldman, 2015/06/25 16:10:36]

This can me a media rule.

 {
-    unsigned index = ruleIndexBySourceRange(nullptr, sourceRange);
+    unsigned index = 0;
+    for (; index < m_pageStyleSheet->length(); ++index) {
+        CSSRule* rule = m_pageStyleSheet->item(index);
+        if (rule == insertBefore)
+            break;
+    }

[lushnikov, 2015/06/25 15:33:07]

let's ASSERT(!insertBefore || index < length())

[pfeldman, 2015/06/25 16:10:36]

It can happen when inserting into media rule or upon CSSOM operation.

+
     m_pageStyleSheet->insertRule(ruleText, index, exceptionState);
     CSSRule* rule = m_pageStyleSheet->item(index);
     CSSStyleRule* styleRule = InspectorCSSAgent::asCSSStyleRule(rule);
     if (!styleRule) {
         m_pageStyleSheet->deleteRule(index, ASSERT_NO_EXCEPTION);
         exceptionState.throwDOMException(SyntaxError, "The rule '" + ruleText + "' could not be added in style sheet.");
         return nullptr;
     }
     return styleRule;
 }
 
-CSSStyleRule* InspectorStyleSheet::insertCSSOMRuleInMediaRule(CSSMediaRule* mediaRule, const SourceRange& sourceRange, const String& ruleText, ExceptionState& exceptionState)
+CSSStyleRule* InspectorStyleSheet::insertCSSOMRuleInMediaRule(CSSMediaRule* mediaRule, CSSRule* insertBefore, const String& ruleText, ExceptionState& exceptionState)

[lushnikov, 2015/06/25 15:33:08]

ditto about insertBefore type

[pfeldman, 2015/06/25 16:10:36]

Acknowledged.

 {
-    unsigned index = ruleIndexBySourceRange(mediaRule, sourceRange);
+    unsigned index = 0;
+    for (; index < mediaRule->length(); ++index) {
+        CSSRule* rule = mediaRule->item(index);
+        if (rule == insertBefore)
+            break;
+    }

[lushnikov, 2015/06/25 15:33:07]

ditto

[pfeldman, 2015/06/25 16:10:36]

Acknowledged.

+
     mediaRule->insertRule(ruleText, index, exceptionState);
     CSSRule* rule = mediaRule->item(index);
     CSSStyleRule* styleRule = InspectorCSSAgent::asCSSStyleRule(rule);
     if (!styleRule) {
         mediaRule->deleteRule(index, ASSERT_NO_EXCEPTION);
         exceptionState.throwDOMException(SyntaxError, "The rule '" + ruleText + "' could not be added in media rule.");
         return nullptr;
     }
     return styleRule;
 }
 
 CSSStyleRule* InspectorStyleSheet::insertCSSOMRuleBySourceRange(const SourceRange& sourceRange, const String& ruleText, ExceptionState& exceptionState)
 {
     ASSERT(m_sourceData);
 
-    int containingRuleIndex = -1;
-    unsigned containingRuleLength = 0;
+    RefPtrWillBeRawPtr<CSSRuleSourceData> containingRuleSourceData;
     for (size_t i = 0; i < m_sourceData->size(); ++i) {
         RefPtrWillBeRawPtr<CSSRuleSourceData> ruleSourceData = m_sourceData->at(i);
         if (ruleSourceData->ruleHeaderRange.start < sourceRange.start && sourceRange.start < ruleSourceData->ruleBodyRange.start) {
             exceptionState.throwDOMException(NotFoundError, "Cannot insert rule inside rule selector.");
             return nullptr;
         }
         if (sourceRange.start < ruleSourceData->ruleBodyRange.start || ruleSourceData->ruleBodyRange.end < sourceRange.start)
             continue;
-        if (containingRuleIndex == -1 || containingRuleLength > ruleSourceData->ruleBodyRange.length()) {
-            containingRuleIndex = i;
-            containingRuleLength = ruleSourceData->ruleBodyRange.length();
-        }
+        if (!containingRuleSourceData || containingRuleSourceData->ruleBodyRange.length() > ruleSourceData->ruleBodyRange.length())
+            containingRuleSourceData = ruleSourceData;
     }
-    if (containingRuleIndex == -1)
-        return insertCSSOMRuleInStyleSheet(sourceRange, ruleText, exceptionState);
 
-    RefPtrWillBeRawPtr<CSSRule> rule = containingRuleIndex < static_cast<int>(m_flatRules.size()) ? m_flatRules.at(containingRuleIndex) : nullptr;
+    RefPtrWillBeRawPtr<CSSRuleSourceData> insertBefore = ruleAfterSourceRange(sourceRange);
+    RefPtrWillBeRawPtr<CSSRule> insertBeforeRule = ruleForSourceData(insertBefore.get());
+
+    if (!containingRuleSourceData)
+        return insertCSSOMRuleInStyleSheet(insertBeforeRule.get(), ruleText, exceptionState);
+
+    RefPtrWillBeRawPtr<CSSRule> rule = ruleForSourceData(containingRuleSourceData.get());
     if (!rule || rule->type() != CSSRule::MEDIA_RULE) {
         exceptionState.throwDOMException(NotFoundError, "Cannot insert rule in non-media rule.");
         return nullptr;
     }
 
-    return insertCSSOMRuleInMediaRule(toCSSMediaRule(rule.get()), sourceRange, ruleText, exceptionState);
+    return insertCSSOMRuleInMediaRule(toCSSMediaRule(rule.get()), insertBeforeRule.get(), ruleText, exceptionState);
 }
 
 RefPtrWillBeRawPtr<CSSStyleRule> InspectorStyleSheet::addRule(const String& ruleText, const SourceRange& location, SourceRange* addedRange, ExceptionState& exceptionState)
 {
     if (location.start != location.end) {
         exceptionState.throwDOMException(NotFoundError, "Source range must be collapsed.");
         return nullptr;
     }
 
     if (!verifyRuleText(m_pageStyleSheet->ownerDocument(), ruleText)) {
@@ skipping 18 matching lines @@
 }
 
 bool InspectorStyleSheet::deleteRule(const SourceRange& range, ExceptionState& exceptionState)
 {
     if (!m_sourceData) {
         exceptionState.throwDOMException(NotFoundError, "Style is read-only.");
         return false;
     }
 
     // Find index of CSSRule that entirely belongs to the range.
-    RefPtrWillBeRawPtr<CSSRule> rule = nullptr;
-    unsigned containingRuleLength = 0;
+    RefPtrWillBeRawPtr<CSSRuleSourceData> foundData = nullptr;
 
-    for (size_t i = 0; i < m_flatRules.size() && i < m_sourceData->size(); ++i) {
+    for (size_t i = 0; i < m_sourceData->size(); ++i) {
         RefPtrWillBeRawPtr<CSSRuleSourceData> ruleSourceData = m_sourceData->at(i);
         unsigned ruleStart = ruleSourceData->ruleHeaderRange.start;
         unsigned ruleEnd = ruleSourceData->ruleBodyRange.end + 1;
         bool startBelongs = ruleStart >= range.start && ruleStart < range.end;
         bool endBelongs = ruleEnd > range.start && ruleEnd <= range.end;
 
         if (startBelongs != endBelongs)
             break;
         if (!startBelongs)
             continue;
-        if (!rule || containingRuleLength > ruleSourceData->ruleBodyRange.length()) {
-            containingRuleLength = ruleSourceData->ruleBodyRange.length();
-            rule = m_flatRules.at(i).get();
-        }
+        if (!foundData || foundData->ruleBodyRange.length() > ruleSourceData->ruleBodyRange.length())
+            foundData = ruleSourceData;
     }
+    RefPtrWillBeRawPtr<CSSRule> rule = ruleForSourceData(foundData.get());
     if (!rule) {
         exceptionState.throwDOMException(NotFoundError, "No style rule could be found in given range.");
         return false;
     }
     CSSStyleSheet* styleSheet = rule->parentStyleSheet();
     if (!styleSheet) {
         exceptionState.throwDOMException(NotFoundError, "No parent stylesheet could be found.");
         return false;
     }
     CSSRule* parentRule = rule->parentRule();
@@ skipping 296 matching lines @@
         RefPtrWillBeRawPtr<CSSRuleSourceData> ruleSourceData = m_sourceData->at(i);
         if (ruleSourceData->ruleBodyRange.start == sourceRange.start && ruleSourceData->ruleBodyRange.end == sourceRange.end) {
             return ruleSourceData;
         }
     }
     return nullptr;
 }
 
 RefPtrWillBeRawPtr<CSSRule> InspectorStyleSheet::ruleForSourceData(CSSRuleSourceData* sourceData)
 {
-    if (!m_sourceData)
+    if (!m_sourceData || !sourceData)
         return nullptr;
     for (size_t i = 0; i < m_sourceData->size(); ++i) {
         if (m_sourceData->at(i).get() == sourceData)
             return i < m_flatRules.size() ? m_flatRules.at(i) : nullptr;
     }
     return nullptr;
 }
 
 RefPtrWillBeRawPtr<CSSRuleSourceData> InspectorStyleSheet::sourceDataForRule(CSSRule* rule)
 {
@@ skipping 125 matching lines @@
 }
 
 DEFINE_TRACE(InspectorStyleSheetForInlineStyle)
 {
     visitor->trace(m_element);
     visitor->trace(m_inspectorStyle);
     InspectorStyleSheetBase::trace(visitor);
 }
 
 } // namespace blink