[FIXr]Images flub xml:base

RESOLVED FIXED in mozilla1.5beta

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.5beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

4.23 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Details | Diff | Splinter Review
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
Attachment #127324 - Flags: superreview?(jst)
Attachment #127324 - Flags: review?(caillon)
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.
Attachment #127324 - Flags: review?(caillon) → review+
Comment on attachment 127324 [details] [diff] [review]
Proposed patch

sr=jst
Attachment #127324 - Flags: superreview?(jst) → superreview+
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.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Summary: Images flub xml:base → Images flub xml:base
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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 127394 [details] [diff] [review]
Proposed better patch

This should be faster, I think....
Attachment #127324 - Attachment is obsolete: true
Attachment #127394 - Flags: superreview?(jst)
Attachment #127394 - Flags: review?(caillon)
Priority: -- → P2
Summary: Images flub xml:base → [FIX]Images flub xml:base
Target Milestone: --- → mozilla1.5beta
Comment on attachment 127394 [details] [diff] [review]
Proposed better patch
Attachment #127394 - Flags: review?(caillon) → review+
Comment on attachment 127394 [details] [diff] [review]
Proposed better patch

sr=jst
Attachment #127394 - Flags: superreview?(jst) → superreview+
Summary: [FIX]Images flub xml:base → [FIXr]Images flub xml:base
Checked in.  Here's hoping that sticks to the tree.... (insofar as I can
determine perf regressions at all in today's frenzy  :( ).
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
Luck is not with us.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Checked in again; last backout was a false alarm.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.