212133 - [FIXr]Images flub xml:base

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

The basic problem is that the load is kicked off at attr-setting time, before we
have a parent...

This also causes problems at http://www.hixie.ch/tests/adhoc/xml/base/002.xml

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127324 [details] [diff] [review]
Proposed patch

>@@ -388,20 +388,34 @@ nsImageLoadingContent::ImageURIChanged(c

>+  // Do we actually need to do anything?
>+  nsCOMPtr<nsIURI> existingURI;

This can be inside the if.

>+  nsCOMPtr<imgIRequest> request = mPendingRequest ? mPendingRequest : mCurrentRequest;
>+  if (request) {
>+    request->GetURI(getter_AddRefs(existingURI));




> NS_IMETHODIMP
> nsHTMLImageElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>                             const nsAString& aValue, PRBool aNotify)
> {
>-  // Call ImageURIChanged first so that the image load kicks off
>-  // _before_ the reflow triggered by the SetAttr
>-  if (aNameSpaceID == kNameSpaceID_None && aName == nsHTMLAtoms::src) {
>+  // Call ImageURIChanged first so that the image load kicks off _before_ the
>+  // reflow triggered by the SetAttr.  If aNotify is false, we are coming from
>+  // the parser or some such place; we'll get our parent set after all the
>+  // attributes have been set, so we'll do the image load from SetParent.

This comment now has a strange feeling to it (the thing it talks about first
comes after the thing it talks about second in the source).  Perhaps moving the
"Call ImageURIChanged..." portion inside the if would help?

>+  if (aNotify &&
>+      aNameSpaceID == kNameSpaceID_None && aName == nsHTMLAtoms::src) {
>     ImageURIChanged(aValue);


>+NS_IMETHODIMP
>+nsHTMLImageElement::SetDocument(nsIDocument* aDocument, PRBool aDeep,
>+                                PRBool aCompileEventHandlers)
>+{
>+  nsresult rv = nsGenericHTMLLeafElement::SetDocument(aDocument, aDeep,
>+                                                      aCompileEventHandlers);
>+  if (aDocument && mParent) {
>+    // Our base url may have changed; claim that our URI changed, and the

"Our base url" ... could you use URI instead of url?  And in SetParent() below
as well?

r=me.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 127324 [details] [diff] [review]
Proposed patch

sr=jst

Comment 4

[Boris Zbarsky [:bzbarsky]]

Made those changes, as well as a change to nsImageDocument to not reenable the
image loading content till after it's been appended to its parent, and checked
in for 1.5a.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I had to back this out due to perf issues.  Basically, SetDocument() is called
twice on every node during parsing and the check in nsImageLoadingContent, while
helping some, was itself not so cheap.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed better patch

This should be faster, I think....

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 127394 [details] [diff] [review]
Proposed better patch

sr=jst

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.  Here's hoping that sticks to the tree.... (insofar as I can
determine perf regressions at all in today's frenzy  :( ).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Luck is not with us.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in again; last backout was a false alarm.