Closed Bug 113173 Opened 20 years ago Closed 18 years ago

Cache nsIURIs instead of strings for list-style-image and -moz-binding

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: hyatt, Assigned: dbaron)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [patch])

Attachments

(2 files, 5 obsolete files)

For both the style data structs and rule structs, we should be caching nsIURIs
rather than raw strings.  Then consumers (like the XUL image frame and the CSS
frame constructor) don't need to waste time allocating their own unique URIs.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
I guess I think eCSSUnit_URL should just become an nsIURI pointer.  (It's hard
to use an nsCOMPtr because nsCSSValue has a union.)
Agreed.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.2
Blocks: 162234
Blocks: 128985
I had some work-in-progress on bug 128985.
Taking bug.
Assignee: hyatt → dbaron
Status: ASSIGNED → NEW
Whiteboard: [patch]
Target Milestone: mozilla1.2alpha → mozilla1.4alpha
Blocks: 133465
Blocks: latebg
Blocks: 162253
Blocks: 102072
Blocks: 167262
Attached patch some work in progress, perhaps? (obsolete) — Splinter Review
Blocks: 208384
Attached patch previous patch updated to trunk (obsolete) — Splinter Review
Attachment #123560 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
This patch builds and runs.
Attachment #132005 - Attachment is obsolete: true
Comment on attachment 132029 [details] [diff] [review]
patch

I didn't propagate the substition of URIs for strings very far into the XBL
code -- that would make this patch rapidly get much bigger.  Perhaps more in a
later patch...
Attachment #132029 - Flags: superreview?(bz-vacation)
Attachment #132029 - Flags: review?(bz-vacation)
Still working on the full review, but for starters we really do want to be
passing a origin charset to NS_NewURI.  This may involve restoring the mCharset
stuff I removed in revision 3.189 of nsCSSParser.cpp (and setting the charset
from the loader before starting the parse).

Also, the second call to NS_NewURI is indeed unnecessary; I can think of no
cases where we would fail with a base URI but succeed without.

On the other hand, do we really want to store a null value in the data struct if
the NS_NewURI fails? Or do we want to bail out and return false?  It'll only
really fail if the string we get just can't be coerced into anything resembling
a URI to necko....
>Index: content/html/content/src/nsGenericHTMLElement.cpp
>+              nsCOMPtr<nsIURI> uri;
>+              rv = NS_NewURI(getter_AddRefs(uri), spec, nsnull, docURL);

nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(uri), spec, doc, docURL)

>Index: content/html/style/src/nsCSSParser.cpp

>+      nsresult rv = NS_NewURI(getter_AddRefs(url), tk->mIdent, nsnull, mURL);
>+      if (NS_FAILED(rv) && mURL) { // XXXldb Does this make sense anymore?
>+        NS_NewURI(getter_AddRefs(url), tk->mIdent);
>       }

Yeah, we definitely want to bail if the URI creation fails, since the
rulenode code and data structs assume the url value will never be
null.

If the error returned is not NS_ERROR_MALFORMED_URI (eg
NS_ERROR_OUT_OF_MEMORY), we may even want to update aErrorCode....

>Index: content/html/style/src/nsCSSValue.cpp

>+nsCSSValue::nsCSSValue(nsIURI* aValue)
>+  : mUnit(eCSSUnit_URL)
>+{
>+  mValue.mURL = aValue;
>+}

Need an NS_IF_ADDREF (or NS_ADDREF, but aValue could be null unless we
very clearly state somewhere that people should just not do that)
there.

> nsCSSValue::nsCSSValue(const nsCSSValue& aCopy)
...
>+  else if (eCSSUnit_URL == mUnit){
>+    mValue.mURL = aCopy.mValue.mURL;
>+    NS_ADDREF(mValue.mURL);

NS_IF_ADDREF?  Someone could create an nsCSSValue with a null URI...

> nsCSSValue& nsCSSValue::operator=(const nsCSSValue& aCopy)
...
>+  else if (eCSSUnit_URL == mUnit){
>+    mValue.mURL = aCopy.mValue.mURL;
>+    NS_ADDREF(mValue.mURL);
>+  }

Same (this is in operator=)

> PRBool nsCSSValue::operator==(const nsCSSValue& aOther) const
...
>+    else if (eCSSUnit_URL == mUnit){
>+      return mValue.mURL == aOther.mValue.mURL;

Wouldn't it make more sense to use nsIURI::Equals() here?

>+void nsCSSValue::SetURLValue(nsIURI* aValue)
>+{
>+  Reset();
>+  mUnit = eCSSUnit_URL;
>+  mValue.mURL = aValue;
>+  NS_ADDREF(mValue.mURL);

Again, should this be NS_IF_ADDREF?

>@@ -252,25 +277,25 @@ void nsCSSValue::AppendToString(nsAStrin
>+  else if (eCSSUnit_URL == mUnit) {
>+    nsCAutoString spec;
>+    mValue.mURL->GetSpec(spec);
>+    aBuffer.Append(NS_ConvertUTF8toUCS2(spec));

AppendUTF8toUTF16(spec, aBuffer);

>Index: content/html/style/src/nsCSSValue.h

>   void  Reset(void)  // sets to null
>+    } else if (eCSSUnit_URL == mUnit) {
>+      NS_RELEASE(mValue.mURL);

NS_IF_RELEASE?

>Index: content/html/style/src/nsROCSSPrimitiveValue.cpp

> nsROCSSPrimitiveValue::GetStringValue(nsAString& aReturn)
...
>+    case CSS_URI: {
>+      nsCAutoString spec;
>+      mValue.mURI->GetSpec(spec);
>+      aReturn.Assign(NS_ConvertUTF8toUCS2(spec));

CopyUTF8toUTF16(spec, aReturn);

>Index: content/html/style/src/nsROCSSPrimitiveValue.h

>   void Reset(void)
>+      case CSS_URI:
>+        NS_ASSERTION(mValue.mRect, "Null URI should never happen");
>+        NS_RELEASE(mValue.mURI);

The assertion should assert on mValue.mURI

>Index: content/shared/public/nsStyleStruct.h

>+  PRBool operator==(const nsStyleContentData& aOther) {
>+    if (mType == eStyleContentType_URL) {
>+      return mContent.mURL == aOther.mContent.mURL;
>+    }

Again, using nsIURI::Equals() may be better.

>Index: content/shared/src/nsStyleStruct.cpp

> nsStyleList::nsStyleList() 
> {
>   mListStyleType = NS_STYLE_LIST_STYLE_BASIC;
>   mListStylePosition = NS_STYLE_LIST_STYLE_POSITION_OUTSIDE;
>-  mListStyleImage.Truncate();  
>+  mListStyleImage;  

What's the purpose of that line?  Should just remove it... (and maybe change to
initializers like "foo::foo() : mFoo(whatever) {}" here?)

I've gotten up to nsIXBLService.h; more tonight.
>Index: content/xbl/src/nsBindingManager.cpp
>+  nsCOMPtr<nsIURI> uri;
>+  rv = NS_NewURI(getter_AddRefs(uri), aURL);
>+  NS_ENSURE_SUCCESS(rv, rv);

This is probably an intl bug, but not any more so than the code that
was here before....  Could you add an XXX comment that this should
either use nsIURI or know what the origin charset for the URL string
was?

>Index: content/xbl/src/nsXBLService.cpp

>+        nsCAutoString spec;
>+        styleBinding->GetBindingURI(spec);
>+        nsCOMPtr<nsIURI> uri;
>+        NS_NewURI(getter_AddRefs(uri), spec.get());

Same issue here.

>   if (!newBinding) {
>-    nsCAutoString str( "Failed to locate XBL binding. XBL is now using id
instead of name to reference bindings. Make sure you have switched over.  The
invalid binding name is: ");
>-    str.AppendWithConversion(aURL);
>+    nsCAutoString spec;
>+    aURL->GetSpec(spec);
>+    nsCAutoString str(NS_LITERAL_CSTRING("Failed to locate XBL binding. XBL is
now using id instead of name to reference bindings. Make sure you have switched
over.  The invalid binding name is: ") + spec);
>     NS_ERROR(str.get());

Shouldn't the nsCAutoString declaration, the GetSpec call, and the
second nsCAutoString declaration, as well as the NS_ERROR, all be
inside #ifdef DEBUG?  In opt builds, this is just extra useless code,
since NS_ERROR evals to nothing.

>Index: layout/html/base/src/nsFrameManager.cpp

>-        if (!oldColor->mBackgroundImage.IsEmpty() &&
>-            oldColor->mBackgroundImage != newColor->mBackgroundImage) {
>+        PRBool equal = PR_TRUE;
>+        if (oldColor->mBackgroundImage &&
>+            (!newColor->mBackgroundImage ||
>+             (oldColor->mBackgroundImage->Equals(newColor->mBackgroundImage,
&equal),
>+              equal))) {

Shouldn't that be "!equal" at the end of the condition, instead of "equal"?

Also, Equals() can fail, because various random people write nsIURI
impls; I suspect that we want to stop the image load if it fails...

>Index: layout/xul/base/src/nsImageBoxFrame.cpp
>+    NS_NewURI(getter_AddRefs(mURI), src, nsnull, baseURI);

Could you add an XXX comment about the charset here too, please?

>     PRBool eq;
>-    requestURI->Equals(srcURI, &eq);
>+    requestURI->Equals(mURI, &eq);

This should really handle errors from Equals().

>+  PRBool equal;
>+  if (newURI != mURI && newURI && (newURI->Equals(mURI, &equal), equal))
>     return NS_OK;

Same here.

>Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

>+    nsCAutoString spec;
>+    uri->GetSpec(spec);
>+    imageSrc = NS_ConvertUTF8toUTF16(spec);

CopyUTF8toUTF16(spec, imageSrc);

>-    NS_NewURI(getter_AddRefs(srcURI), *imagePtr, nsnull, baseURI);
>+    NS_NewURI(getter_AddRefs(srcURI), imageSrc, nsnull, baseURI);

Could use an XXX comment here too.... <sigh>

Attachment #132029 - Flags: superreview?(bzbarsky)
Attachment #132029 - Flags: superreview-
Attachment #132029 - Flags: review?(bzbarsky)
Attachment #132029 - Flags: review-
Do we really need the origin charset for all these things?  I had a vague memory
that it was only supposed to be used when the URL was displayed.
Attached patch patch (obsolete) — Splinter Review
Attachment #132029 - Attachment is obsolete: true
Everything seems ok with
http://www.w3.org/Style/CSS/Test/CSS1/current/test564.htm (including poking at
it with DOM Inspector), but there's a problem with the dropdowns on the back and
forward buttons (although it could be in the trunk too -- I just updated).
Attached patch patch (obsolete) — Splinter Review
That's what I get for trying to fix bug 220717 in the same patch.
Attachment #132366 - Attachment is obsolete: true
Attachment #132372 - Flags: superreview?(bzbarsky)
Attachment #132372 - Flags: review?(bzbarsky)
> Do we really need the origin charset for all these things?

We need it for any URI we send back to a webserver, really... 
Comment on attachment 132372 [details] [diff] [review]
patch

>Index: content/base/src/nsRuleNode.cpp

>+            data.mContent.mURL = value.GetURLValue();
>+            NS_ADDREF(data.mContent.mURL);

NS_IF_ADDREF

>Index: content/html/style/src/nsCSSParser.cpp

>+      NS_NewURI(getter_AddRefs(url), tk->mIdent, nsnull, mURL);

I guess it's ok to leave like this for now, but please file a follow-up bug (on
me, I guess) to actually use the right charset here...	We're lucky not that
many sites use non-ascii chars in URLs...

>Index: content/html/style/src/nsCSSValue.cpp

>+      mValue.mURL->Equals(aOther.mValue.mURL, &eq);
>+      return eq;

Equals() could fail and not set eq (used to for mailnews URIs until recently,
in fact).  This code should probably assume FALSE if Equals() fails....

>Index: content/html/style/src/nsROCSSPrimitiveValue.cpp
>+        mValue.mURI->GetSpec(spec);
>+      CopyUTF8toUTF16(spec, aReturn);

Weird indent.

>Index: content/shared/public/nsStyleStruct.h

>+  ~nsStyleContentData() {
>+    if (mType == eStyleContentType_URL) {
>+      NS_RELEASE(mContent.mURL);

NS_IF_RELEASE

>+    if (mType == eStyleContentType_URL) {
>+      mContent.mURL = aOther.mContent.mURL;
>+      NS_ADDREF(mContent.mURL);

NS_IF_ADDREF

>+      PRBool eq;
>+      mContent.mURL->Equals(aOther.mContent.mURL, &eq);
>+      return eq;

Again, Equals() can fail....

>Index: content/xbl/src/nsXBLService.cpp

>+        PRBool equal = PR_FALSE;
>+        uri->Equals(aURL, &equal);
>+        if (equal)

And here.

>Index: extensions/inspector/base/src/inCSSValueSearch.cpp

>+    value.GetURLValue()->GetSpec(spec);

GetURLValue() could return null, no?  This needs to handle that case...

>Index: layout/xul/base/src/nsImageBoxFrame.cpp
>     PRBool eq;
>-    requestURI->Equals(srcURI, &eq);
>+    requestURI->Equals(mURI, &eq);

This could fail too.

>+  PRBool equal;
>+  if (newURI != mURI && newURI && (newURI->Equals(mURI, &equal), equal))
>     return NS_OK;

As could this.

r+sr=bzbarsky with those nits fixed.
Attachment #132372 - Flags: superreview?(bzbarsky)
Attachment #132372 - Flags: superreview+
Attachment #132372 - Flags: review?(bzbarsky)
Attachment #132372 - Flags: review+
> >Index: content/html/style/src/nsROCSSPrimitiveValue.cpp
> >+        mValue.mURI->GetSpec(spec);
> >+      CopyUTF8toUTF16(spec, aReturn);
>
> Weird indent.

What's weird about it?  The previous line is an if, and I thought what I did was
simpler than having an |else { aReturn.Truncate(); }|.
Attached patch patchSplinter Review
This is what I just checked in.
Attachment #132372 - Attachment is obsolete: true
Fix checked in to trunk, 2003-10-01 15:53 -0700.

Leaving bug open for a few pieces of additional cleanup, or at least filing
separate bugs to do them.
Comment on attachment 132480 [details] [diff] [review]
patch

Some tinderbox results from this landing:

Tp on btek improved from 991ms to 976ms, averaging over 6 samples before and
after the checkin (p=2e-6).

Txul seemed to improve on comet (from about 423.5 to 420.5), p=0.017, and
perhaps on beast as well (although I'd like to wait for more data).

Codesize dropped on most tinderboxes, but increased on one.  I suspect most of
the changes was due to the inlining changes in nsCSSValue.h.

brad: -18140  (gcc 3.3.1 Linux)
comet: +11864 (gcc 2.96 (RH) Linux)
luna: -19272  (gcc 3.2 (RH) Linux)
beast: -1754  (VC 6.0 Windows)
Actually, doesn't the code in nsStyleContext::DumpRegressionData (DEBUG-only)
need updating too?
Comment on attachment 132620 [details] [diff] [review]
additional fixes that bz caught

r+sr=bzbarsky
Attachment #132620 - Flags: superreview+
Attachment #132620 - Flags: review+
Comment on attachment 132620 [details] [diff] [review]
additional fixes that bz caught

Checked in.
bryner found another problem, and the fix is in bug 221186
Blocks: 224765
Is this fixed now? It looks like the patch was checked in months ago :)
See comment 20.
bzbarsky actually did the remaining XBL stuff, so I guess I can just mark it as
fixed, although there were a few straggling GetSpec callers...
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
OK, thanks. It's just blocking so many bugs, and it looked like it was fixed, so
thought I'd check :)
*** Bug 229959 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.