Editor should use parser service for IsBlock

VERIFIED FIXED in mozilla1.2alpha

Status

()

Core
Editor
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Akkana Peck, Assigned: Akkana Peck)

Tracking

Trunk
mozilla1.2alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in hand)

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
We probably won't be able to justify this risk for 1.0 at this point, so marking
1.0.1.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
(Assignee)

Comment 2

16 years ago
Created attachment 83612 [details] [diff] [review]
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

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

r=brade
Attachment #83612 - Flags: review+
(Assignee)

Comment 4

16 years ago
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

16 years ago
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.
(Assignee)

Comment 6

16 years ago
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).
Whiteboard: fix in hand, need sr
Target Milestone: mozilla1.0.1 → mozilla1.2alpha

Comment 7

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

sr=kin@netscape.com

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?
Attachment #83612 - Flags: superreview+

Updated

16 years ago
Whiteboard: fix in hand, need sr → fix in hand
(Assignee)

Comment 8

16 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
did a few block-level changes (add, modify, delete), and it seems fine. vrfy'd
with 2003.04.10.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.