Closed
Bug 312592
Opened 19 years ago
Closed 18 years ago
Reduce content creation redundancy in nsHTMLContentSink
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: sicking)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
20.60 KB,
patch
|
mrbkap
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
This is to be applied on top of the patch in bug 311785
Assignee | ||
Updated•19 years ago
|
Assignee: general → bugmail
Status: NEW → ASSIGNED
Assignee | ||
Updated•19 years ago
|
Attachment #199701 -
Flags: superreview?(peterv)
Attachment #199701 -
Flags: review?(mrbkap)
Assignee | ||
Comment 2•19 years ago
|
||
ugh, i forgot to diff enough directories. There's also a change to nsContentSink that makes ProcessMETATag public
Assignee | ||
Updated•19 years ago
|
Comment 3•19 years ago
|
||
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).
Assignee | ||
Comment 4•19 years ago
|
||
(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?
Comment 5•19 years ago
|
||
(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.
Assignee | ||
Comment 6•19 years ago
|
||
How about: // Look for <area> or <a> elements. We'll use whichever type we find first. You want a new patch?
Comment 7•19 years ago
|
||
Might as well for the missing |break|s. Now you can attach a real diff instead of the interdiff ;-).
Comment 8•19 years ago
|
||
(Note that HTML5 currently drops support for <a shape> in <map>, having only <area> for image maps).
Assignee | ||
Comment 9•19 years ago
|
||
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 10•19 years ago
|
||
Comment on attachment 202124 [details] [diff] [review] Patch v2 r=mrbkap
Attachment #202124 -
Flags: review?(mrbkap) → review+
Updated•19 years ago
|
Attachment #202124 -
Flags: superreview?(peterv) → superreview+
Comment 11•19 years ago
|
||
Comment on attachment 202124 [details] [diff] [review] Patch v2 Sorry, wrong attachment :-).
Attachment #202124 -
Flags: superreview+ → superreview?
Updated•19 years ago
|
Attachment #202124 -
Flags: superreview? → superreview?(peterv)
Comment 12•19 years ago
|
||
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+
Assignee | ||
Comment 13•18 years ago
|
||
This was checked in 2005-11-10 07:51
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•