132352 - Editor should use parser service for IsBlock

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Akkana Peck]

Long ago I wrote code to make the editor use the parser service for checking
"IsBlock"ness of html tags (see nsHTMLEditor::NodeIsBlockStatic,
USE_PARSER_FOR_BLOCKNESS).  For some reason, this is disabled and no one
remembers why.  We should enable it, use it and remove the other code.

Comment 1

[Akkana Peck]

We probably won't be able to justify this risk for 1.0 at this point, so marking
1.0.1.

Comment 2

[Akkana Peck]

Attachment: Make it so

Needs r, sr.  Note: the reason it's static is not because there's a lot of
overhead (I don't think there is, for the small number of html editor instances
likely at any one time) but because this routine is used from static routines
so we can't rely on having a "this" pointer.

Comment 3

[Kathleen :Brade]

Comment on attachment 83612 [details] [diff] [review]
Make it so

r=brade

Comment 4

[Akkana Peck]

Kin: Can you sr this, at your convenience?

I should be clearer (Kathy asked me about this): this is trunk only, not being
proposed for the branch.  I expect that we will notice some issues with tags
that are being treated differently by the parser vs. our built-in list, and we
will need some time to shake all of those out.  They'll be very easy to fix
when/if they're noticed, but definitely not something we'd want to do this late
on the branch.

Also, Kathy asked about the 0 vs. nsnull when clearing the nsCOMPtr in the
destructor.  I'm agnostic about 0 vs. nsnull myself; I'm perfectly happy to use
nsnull if Kin thinks it's better.

Comment 5

[Joe Francis]

For my own personal sanity please just comment out the code in NodeIsBlockStatic 
rather than remove it.  Later after we have corrected any probs with the patch I 
can remove the commented out code.

Comment 6

[Akkana Peck]

Now knowing that 1.0.1 is also on the branch, I'm moving the milestone out since
this is only intended for trunk builds.  I'm still looking for an sr -- Kin?

To address Joe's point: I agree, we shouldn't remove any code yet (and the
current patch doesn't).  We can remove it later after the new code has been
enabled for a while (and I'll get a review from Joe before removing anything).

Comment 7

[kinmoz]

Comment on attachment 83612 [details] [diff] [review]
Make it so

sr=[e-mail]

I don't think we need to initiallize sParserService with zero since the comptr
constructor takes care of that for us:

+nsCOMPtr<nsIParserService> nsHTMLEditor::sParserService = 0;

Also, do we need to make sure that the set of tags used by the |if| expression
used in the !USE_PARSER_FOR_BLOCKNESS code is in sync with the DEBUG code in
the USE_PARSER_FOR_BLOCKNESS block you are enabling?

Comment 8

[Akkana Peck]

It's in.  Any regressions or annoying assertions that come up, please file to me.

I removed the =0 in the nsCOMPtr initializer, as Kin suggested.  Yes, we have to
keep those two lists synchronized, but they shouldn't be changing much at this
point and I hope that the !USE_PARSER fork will go away before too long, after
this has been tested and tuned.

Sujay: the only verification for this is just ensure there are no bad
regressions from it (maybe try some block-level operations in a couple test
files); the real change is code-level.

Comment 9

[sairuh (rarely reading bugmail)]

did a few block-level changes (add, modify, delete), and it seems fine. vrfy'd
with 2003.04.10.