153998 - COtherDTD should go; composer should handle containment checking itself

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

COtherDTD only exists because composer wants containment checking.  It's a way
too heavyweight solution for that...  we should do the containment checking in
composer itself and remove COtherDTD.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See also bug 130439.  (The last patch there hasn't been checked in.)

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Files to be cvs rm'ed

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 187959 [details] [diff] [review]
v1

>Index: editor/libeditor/html/nsHTMLEditUtils.cpp
>===================================================================
>+
>+
>+#define GROUP_NONE             0

Nit: Extra newline (though a comment explaining what these #defines are for
would probably not hurt).
Note: I'm assuming that you basically just translated the containment lists and
the groups from the original code. From a quick check, everything looks OK,
though.

>+PRBool
>+nsHTMLEditUtils::CanContain(PRInt32 aParent, PRInt32 aChild)
>+{
>+#ifdef DEBUG
>+  PRInt32 i;
>+  for (i = 1; i <= eHTMLTag_userdefined; ++i) {
>+    NS_ASSERTION(kElements[i - 1].mTag == i,
>+                 "You need to update kElements (missing tags).");
>+  }
>+#endif

Does this check need to happen every time someone calls ::CanContain in a debug
build? Why not have something like:
static PRBool checked = PR_FALSE;
if (!checked) {
  checked = PR_TRUE;
  // check...
}

>+  return (parent.mCanContainGroups & child.mGroup) ? PR_TRUE : PR_FALSE;

Nit: that could be "(parent.mCanContainGroups & child.mGroups) != 0", but
that's a question of style.

Other than those nits, r=me

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 187959 [details] [diff] [review]
v1

Nice! sr=jst

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 187959 [details] [diff] [review]
v1

This should give a bit of codesize reduction. I've verified that the new code
returns the same result for all combinations of input as the old code by
feeding all possible input values to both.

Comment 7

[Benjamin Smedberg]

My inclination is not to approve this for 1.8, since touching editor always
tends to blow up and gets much less QA anyway: is there a good reason why we
need this?

Comment 8

[Peter Van der Beken [:peterv]]

Well, I'm not going to spend much time on getting this approval. I felt
confident enough about the code to ask for it, and from a quick measurement this
gives about 20k codesize reduction on Windows.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 187959 [details] [diff] [review]
v1

Let's get this in on the trunk after we branch.

Comment 10

[Peter Van der Beken [:peterv]]

Finally checked this in. From brad: mZdiff:-36448.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So this just landed on the trunk, right?  Not sure how to interpret the target
milestone here...

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 63116 has been marked as a duplicate of this bug. ***

Comment 13

[[e-mail]]

Comment on attachment 187959 [details] [diff] [review]
v1

Unfortunately it seems as if this patch is also required to fix bug 299343 so I'm requesting the same approvals although less hopefully.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 187959 [details] [diff] [review]
v1

I see no need to put this on the branch, the patch for bug 299343 should be ported to the branch and that's unrelated to this patch.