Closed
Bug 132352
Opened 22 years ago
Closed 22 years ago
Editor should use parser service for IsBlock
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
VERIFIED
FIXED
mozilla1.2alpha
People
(Reporter: akkzilla, Assigned: akkzilla)
Details
(Whiteboard: fix in hand)
Attachments
(1 file)
3.86 KB,
patch
|
Brade
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
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•22 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•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 83612 [details] [diff] [review] Make it so r=brade
Attachment #83612 -
Flags: review+
Assignee | ||
Comment 4•22 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•22 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•22 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 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+
Assignee | ||
Comment 8•22 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
Closed: 22 years ago
Resolution: --- → FIXED
Comment 9•21 years ago
|
||
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.
Description
•