Closed Bug 312592 Opened 19 years ago Closed 18 years ago

Reduce content creation redundancy in nsHTMLContentSink

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We currently have a bunch of methods like ProcessAREATag and ProcessMETATag that
manage special handling of some elements in the sink. However, these functions
also create the element and insert it in the DOM. A better way of doing things
is to let the normal codepath create and insert the element and just call the
ProcessXXX function to do post-processing.

I also killed the code that move all <area> descendants of a <map> to be direct
children of the map (by adding the areas to mCurrentMap instead of to the
current insertposition). I modified nsImageMap to do a deep search for both <a>
and <area> so that we get the same behaviour.

I had to keep ProcessLINKTag for now since it does processing both before and
after the element is inserted in the DOM. I'll deal with that one in a separate
patch.
Attached patch Patch to fix (obsolete) — Splinter Review
This is to be applied on top of the patch in bug 311785
Assignee: general → bugmail
Status: NEW → ASSIGNED
Attachment #199701 - Flags: superreview?(peterv)
Attachment #199701 - Flags: review?(mrbkap)
ugh, i forgot to diff enough directories. There's also a change to nsContentSink
that makes ProcessMETATag public
Blocks: 239152
No longer depends on: 239152
Comment on attachment 199701 [details] [diff] [review]
Patch to fix

>+      // Additional processing needed once the element is in the tree
>       switch (nodeType) {
>+      case eHTMLTag_base:
>+        if (!mSink->mInsideNoXXXTag) {
>+          mSink->ProcessBASEElement(content);
>+        }
>+
>+      case eHTMLTag_meta:
>+        if (!mSink->mInsideNoXXXTag && !mSink->mFrameset) {
>+          rv = mSink->ProcessMETATag(content);
>+        }

You need some "Fall through" comments here. Also, it looks like base tags will end up going through ProcessMETATag, which I don't think is the correct thing to do.

>+    if (child->IsContentOfType(nsIContent::eHTML)) {
>+      // First check if this map element contains an AREA element.
>+      // If so, we only look for AREA elements
>+      if (!aFoundAnchor && child->Tag() == nsHTMLAtoms::area) {
>+        aFoundArea = PR_TRUE;
>+        rv = AddArea(child);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        
>+        // Continue to next child. This stops mContainsBlockContents from
>+        // getting set. It also makes us ignore children of <area>s which
>+        // is consistent with how we react to dynamic insertion of such
>+        // children.
>+        continue;
>+      }
>+      // If we haven't determined that the map element contains an
>+      // AREA element yet, the look for anchors.
>+      else if (!aFoundArea && child->Tag() == nsHTMLAtoms::a) {
>+        aFoundAnchor = PR_TRUE;

This doesn't seem to be doing what the comment says it's doing. Consider: <map><a><area>. Your loop will find the <a> and ignore the <area> since you'll set aFoundAnchor first. Is that the desired behavior here? Also, please nix the else-after-continue (where continue is a form of break).
(In reply to comment #3)
> You need some "Fall through" comments here. Also, it looks like base tags will
> end up going through ProcessMETATag, which I don't think is the correct thing
> to do.

Good catch, that's missing some |break|s. Not sure what you mean by 'fall through comments' though?

> This doesn't seem to be doing what the comment says it's doing. Consider:
> <map><a><area>. Your loop will find the <a> and ignore the <area> since you'll
> set aFoundAnchor first. Is that the desired behavior here?

That is the behaviour we want (or at least, what the old code did). What comment doesn't agree with that?
(In reply to comment #4)
> Good catch, that's missing some |break|s. Not sure what you mean by 'fall
> through comments' though?

If you'd meant to fall through to the DoneCreatingElement (as I'd assumed you did) I would have expected comments like: // Fall through... to indicate so. Since you didn't, you don't need them.

> That is the behaviour we want (or at least, what the old code did). What
> comment doesn't agree with that?

+      // First check if this map element contains an AREA element.
+      // If so, we only look for AREA elements

This seems to imply to me at least that if the map contains _any_ area elements, then we'd ignore the anchors. I'm not sure what I'd replace it with though.
How about:

// Look for <area> or <a> elements. We'll use whichever type we find first.

You want a new patch?
Might as well for the missing |break|s. Now you can attach a real diff instead of the interdiff ;-).
(Note that HTML5 currently drops support for <a shape> in <map>, having only <area> for image maps).
Attached patch Patch v2Splinter Review
Addresses comments from blake
Attachment #199701 - Attachment is obsolete: true
Attachment #202124 - Flags: superreview?(peterv)
Attachment #202124 - Flags: review?(mrbkap)
Attachment #199701 - Flags: superreview?(peterv)
Attachment #199701 - Flags: review?(mrbkap)
Comment on attachment 202124 [details] [diff] [review]
Patch v2

r=mrbkap
Attachment #202124 - Flags: review?(mrbkap) → review+
Attachment #202124 - Flags: superreview?(peterv) → superreview+
Comment on attachment 202124 [details] [diff] [review]
Patch v2

Sorry, wrong attachment :-).
Attachment #202124 - Flags: superreview+ → superreview?
Attachment #202124 - Flags: superreview? → superreview?(peterv)
Comment on attachment 202124 [details] [diff] [review]
Patch v2

>Index: content/html/document/src/nsHTMLContentSink.cpp
>===================================================================

>@@ -1471,18 +1454,30 @@ SinkContext::AddLeaf(const nsIParserNode

>       switch (nodeType) {
>+      case eHTMLTag_base:
>+        if (!mSink->mInsideNoXXXTag) {
>+          mSink->ProcessBASEElement(content);
>+        }
>+        break;
>+
>+      case eHTMLTag_meta:
>+        if (!mSink->mInsideNoXXXTag && !mSink->mFrameset) {

You removed some comments here:

>-  // XXX It's just not sufficient to check if the parent is head. Also
>-  // check for the preference.
>-  // Bug 40072: Don't evaluate METAs after FRAMESET.

Are they unneeded?

>Index: layout/generic/nsImageMap.cpp
>===================================================================

>@@ -798,75 +799,70 @@ nsImageMap::Init(nsIPresShell* aPresShel

>+nsImageMap::SearchForAreas(nsIContent* aParent, PRBool& aFoundArea,

>+      // If we haven't determined that the map element contains an
>+      // <a> element yet, the look for <area>.

s/the/then/

>+        continue;
>+      }
>+      // If we haven't determined that the map element contains an
>+      // <area> element yet, the look for <a>.

s/the/then/

>+      else if (!aFoundArea && child->Tag() == nsHTMLAtoms::a) {

No else after continue
Attachment #202124 - Flags: superreview?(peterv) → superreview+
This was checked in 2005-11-10 07:51
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.