User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 Build Identifier: Followup to bug 499655 Reproducible: Always

In particular, in bug 499655 comment 9: + if(data.mIsHTMLContent) { You should add a comment that if we had a stricter test here (i.e., that it was an HTML node in a text/html document) and perhaps some tweaks to RuleHash, we could completely remove the notion of case sensitivity from style sheets. (I think this would fix some existing bugs related to the handling of our user-agent style sheets.) Maybe I could convince you to do that in a followup patch?

Ignore comment 1; I thought you were filing a different bug.

Comment on attachment 391773 [details] [diff] [review] Patch 1.1 >+struct TagHashTableEntry : public RuleHashTableEntry { >+ nsCOMPtr<nsIAtom> mTag; >+}; Could you call this RuleHashTagTableEntry instead? >+static PRBool >+TagRuleHash_MatchEntry(PLDHashTable *table, const PLDHashEntryHdr *hdr, >+ const void *key) Call this RuleHash_TagTable_MatchEntry ? >+ // Instead of getKey, use the tag member. No need for that comment. >+static void TagRuleHash_ClearEntry(PLDHashTable *table, PLDHashEntryHdr *entry) And call this RuleHash_TagTable_ClearEntry? >+void RuleHash::PrependRuleToTagTable(const void* aKey, RuleValue* aRuleInfo) >+{ >+ // Get a new or exisiting entry >+ TagHashTableEntry *entry = static_cast<TagHashTableEntry*> >+ (PL_DHashTableOperate(&mTagTable, aKey, PL_DHASH_ADD)); >+ if (!entry) >+ return; >+ >+ entry->mTag = const_cast<nsIAtom*>(static_cast<const nsIAtom*>(aKey)); >+ entry->mRules = aRuleInfo->Add(mRuleCount++, entry->mRules); >+} You should at least add a comment that this potentially gives the same rule two different rule counts, but that isn't a problem since we'd never be combining two different entries in the tag table. >+ else if (selector->IsPseudoElement()) { >+ PrependRuleToTagTable(selector->mLowercaseTag, aRuleInfo); > RULE_HASH_STAT_INCREMENT(mTagSelectors); > } >+ else if (selector->HasTagSelector()) { >+ PrependRuleToTagTable(selector->mCasedTag, aRuleInfo); >+ RULE_HASH_STAT_INCREMENT(mTagSelectors); >+ if (selector->mCasedTag != selector->mLowercaseTag) { >+ PrependRuleToTagTable(selector->mLowercaseTag, aRuleInfo); >+ RULE_HASH_STAT_INCREMENT(mTagSelectors); >+ } >+ } This would be a little shorter if you did: else if (selector->mLowercaseTag) { ... if (selector->mCasedTag && selector->mCasedTag != selector->mLowercaseTag) { ... } } Looks good with those changes.

Comment on attachment 391782 [details] [diff] [review] Patch addressing comments >+ else if (selector->mLowercaseTag) { >+ PrependRuleToTagTable(selector->mLowercaseTag, aRuleInfo); >+ RULE_HASH_STAT_INCREMENT(mTagSelectors); >+ } >+ else if (selector->mCasedTag && >+ selector->mCasedTag != selector->mLowercaseTag) { >+ PrependRuleToTagTable(selector->mCasedTag, aRuleInfo); > RULE_HASH_STAT_INCREMENT(mTagSelectors); > } That's not quite right, and I think it would actually regress the testcase again. I meant what I wrote when I put the if inside the else if; you want the second half nested in the first rather than a separate else in the chain.

Oh... but there's actually another problem, which is that we actually need two RuleValue objects, but we're only making one and trying to stick the same one in two linked lists.

I'm inclined to back out bug 499655 for the alpha (which could happen anytime, basically), at least until we get this sorted out.

The previous patch did indeed fail mochitests. Should have tested those too, not just reftests.

Comment on attachment 391921 [details] [diff] [review] Fixes tests >+ else if (selector->mLowercaseTag) { >+ PrependRuleToTagTable(selector->mLowercaseTag, aRuleInfo); > RULE_HASH_STAT_INCREMENT(mTagSelectors); >+ if (selector->mCasedTag && >+ selector->mCasedTag != selector->mLowercaseTag) { >+ PrependRuleToTagTable(selector->mCasedTag, >+ &RuleValue(aRuleInfo->mRule, aRuleInfo->mSelector)); This needs to be: new (&mArena) RuleValue(aRuleInfo->mRule, aRuleInfo->mSelector) since otherwise you're just passing PrependRuleToTagTable a stack-allocated object (which will go away after the call completes, since it's a temporary) and telling it to hold on to that pointer. >+ RULE_HASH_STAT_INCREMENT(mTagSelectors); >+ } > } r=dbaron once you fix that, although I'm surprised it didn't crash on the simple tests you have

Comment on attachment 391939 [details] [diff] [review] Patch to check-in r=dbaron