Last Comment Bug 599983 - Remove use or optimize performance of moz attributes in editor and serializer
: Remove use or optimize performance of moz attributes in editor and serializer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Aryeh Gregor (:ayg) (away until October 25)
:
:
Mentors:
Depends on:
Blocks: domperf 744830
  Show dependency treegraph
 
Reported: 2010-09-27 13:28 PDT by Olli Pettay [:smaug] (reviewing overload)
Modified: 2012-04-24 18:09 PDT (History)
12 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bluegriffon preferences screenshot (87.03 KB, image/png)
2012-04-18 08:49 PDT, Daniel Glazman (:glazou)
no flags Details
Patch part 1, v1 -- Clean up nsEditor::MarkNodeDirty (1.18 KB, patch)
2012-04-19 06:14 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v0.9 -- Don't emit or process _moz_dirty more than necessary (9.28 KB, patch)
2012-04-19 06:20 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review-
bzbarsky: review-
Details | Diff | Splinter Review
Patch part 1, v2 -- Clean up nsEditor::MarkNodeDirty (1.07 KB, patch)
2012-04-20 00:56 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Don't emit or process _moz_dirty more than necessary (13.16 KB, patch)
2012-04-23 02:51 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
bzbarsky: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (reviewing overload) 2010-09-27 13:28:46 PDT
Getting the moz attributes shows up in the GetInnerHTML profiles.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2010-09-27 15:14:52 PDT
Can we do this for 2.0?  This one issue is about 10% of innerHTML time in the profiles I was looking at...
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2010-09-27 15:15:16 PDT
In particular, can we somehow make use of our editable node flags instead?
Comment 3 :Ehsan Akhgari 2010-09-27 15:35:20 PDT
Can someone please augment this bug with some information on what it's actually about?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2010-09-27 19:08:45 PDT
Sure, sorry about leaving that out.  nsXHTMLContentSerializer::CheckElementStart has this code:

566   // The _moz_dirty attribute is emitted by the editor to
567   // indicate that this element should be pretty printed
568   // even if we're not in pretty printing mode
569   aForceFormat = aContent->HasAttr(kNameSpaceID_None,
570                                    nsGkAtoms::mozdirty);

This runs for every element during GetInnerHTML, and is 10% of the innerHTML profiles I've looked at recently.

So can we either skip this check if aContent is not being edited or have a way for GetInnerHTML to override this behavior or something?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2010-09-27 19:09:53 PDT
Note that CheckElementEnd has similar code, btw.  And so does nsHTMLContentSerializer::AppendElementEnd.
Comment 6 :Ehsan Akhgari 2010-09-27 21:07:25 PDT
(In reply to comment #4)
> So can we either skip this check if aContent is not being edited or have a way
> for GetInnerHTML to override this behavior or something?

I like the first approach better...

Is this something that we absolutely want for 2.0?  In that case, it should be nominated to block.
Comment 7 Olli Pettay [:smaug] (reviewing overload) 2010-09-28 03:13:19 PDT
Note, IIRC some code uses editors moz- attributes to style the content.
Comment 8 Daniel Glazman (:glazou) 2010-09-28 03:17:12 PDT
_moz_ attributes are very useful to the editor. It's the only way we can add content to the editor view and make sure it's not serialized out. Inline table editing uses it, image resizing uses it, positioning uses it.
Comment 9 Olli Pettay [:smaug] (reviewing overload) 2010-09-28 03:32:43 PDT
What is the usecase for mozdirty?
Could we use properties? (nsINode::SetProperty)
Comment 10 Daniel Glazman (:glazou) 2010-09-28 03:40:20 PDT
(In reply to comment #9)
> What is the usecase for mozdirty?
> Could we use properties? (nsINode::SetProperty)

Yes probably. I don't think there is any CSS attached to _moz_dirty.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2010-09-28 04:55:57 PDT
Would a prop test be faster than HasAttr?  Can we not just use a boolean flag on nsINode?
Comment 12 Olli Pettay [:smaug] (reviewing overload) 2010-09-28 05:02:30 PDT
Yeah, I was looking at the property code too and perhaps it isn't faster.
Do we have any extra space in nsINode?
Comment 13 Olli Pettay [:smaug] (reviewing overload) 2010-09-28 05:03:07 PDT
Seems like we do have, since
bool mNodeHasRenderingObservers : 1; has been added already.

(nsINode is getting quite heavy :/ )
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2010-09-28 05:40:34 PDT
> Seems like we do have, since
> bool mNodeHasRenderingObservers : 1; has been added already.

Yes, exactly.  I have a bug on my plate to make it take up less space, fwiw.  

> (nsINode is getting quite heavy :/ )

Numbers in https://bugzilla.mozilla.org/show_bug.cgi?id=598833#c0 if you care.  ;)
Comment 15 :Ehsan Akhgari 2010-09-28 09:24:09 PDT
If we choose to only look for _moz_ attributes in editable content, we're paying the cost where it matters, right?  I'm not sure if dropping _moz_ attributes and replacing them by something else (e.g. properties) is practical.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2010-09-28 11:38:42 PDT
Sure, but can we reliably and quickly depend editable content in the sense that matters here?
Comment 17 :Ehsan Akhgari 2010-09-28 12:34:28 PDT
(In reply to comment #16)
> Sure, but can we reliably and quickly depend editable content in the sense that
> matters here?

I'm not sure what you're asking here.  Isn't NODE_IS_EDITABLE all that we need here?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2010-09-28 12:51:57 PDT
I don't know; that's the point!  If it is, perfect.
Comment 19 :Ehsan Akhgari 2010-09-28 13:32:02 PDT
(In reply to comment #18)
> I don't know; that's the point!  If it is, perfect.

Yes, I think that's enough.

Boris, what do you think about the last paragraph of comment 6?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2010-09-28 13:42:23 PDT
I think this is something we should do for 2.0 if it's safe and easy.  I don't think we need to block on it.  The same is true of pretty much any non-regression performance bug, with emphasis on "safe and easy".
Comment 21 :Ehsan Akhgari 2010-09-28 14:03:09 PDT
So, I'll tentatively take this, but I'm not at all sure that I can look at it before 2.0.  I have a huge list of lower priority bugs which I'm planning to go through when my blockers are done and there are no other blockers that I can help with, but that might not happen before 2.0...
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2010-09-28 15:29:54 PDT
Is there more to do here than the NODE_IS_EDITABLE check?  'cause I can do that part..
Comment 23 :Ehsan Akhgari 2010-09-28 15:40:25 PDT
(In reply to comment #22)
> Is there more to do here than the NODE_IS_EDITABLE check?  'cause I can do that
> part..

Not on the surface, but then again I've had bugs related to the editor which turned into a 10-part patch and they seemed like a one-liner before I started to work on them!  ;-)
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2010-10-04 11:21:34 PDT
Checking the flag is not good enough; have to make an IsEditable() call on the node, right?  Otherwise nodes in editable documents won't do the right thing....
Comment 25 :Ehsan Akhgari 2010-10-04 12:33:01 PDT
(In reply to comment #24)
> Checking the flag is not good enough; have to make an IsEditable() call on the
> node, right?  Otherwise nodes in editable documents won't do the right
> thing....

Why?  We're not interested to see if the node is actually editable or not, right?  We just want to see if it's worth spending the time to check the moz attributes on a node, so if we try to grab the editor for the document and go through the IsEditable call, we'll defeat our purpose.
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2010-10-04 12:51:44 PDT
I really wonder what the zimbra testcase is doing here...  I can't get this codepath to be more than about 2% of total serialization time on other pages I've tried....
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2010-10-04 12:53:03 PDT
> We just want to see if it's worth spending the time to check the moz
> attributes on a node

Right, but the NODE_IS_EDITABLE flag is not set on nodes in documents that are being edited in their entirety, as far as I can see.  Unless I missed something.

That is, we need a check which _will_ catch nodes the editor might be messing with (but might catch some others too).  And NODE_IS_EDITABLE has the exact opposite property.
Comment 28 :Ehsan Akhgari 2010-10-04 13:15:26 PDT
(In reply to comment #27)
> > We just want to see if it's worth spending the time to check the moz
> > attributes on a node
> 
> Right, but the NODE_IS_EDITABLE flag is not set on nodes in documents that are
> being edited in their entirety, as far as I can see.  Unless I missed
> something.

Hmm, why do you say that?  Those documents should have that flag on their document node, which should make it inherited by every node in the document, right?

> That is, we need a check which _will_ catch nodes the editor might be messing
> with (but might catch some others too).  And NODE_IS_EDITABLE has the exact
> opposite property.

I don't really see where this conclusion is coming from.
Comment 29 :Ehsan Akhgari 2010-10-04 13:22:48 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > > We just want to see if it's worth spending the time to check the moz
> > > attributes on a node
> > 
> > Right, but the NODE_IS_EDITABLE flag is not set on nodes in documents that are
> > being edited in their entirety, as far as I can see.  Unless I missed
> > something.
> 
> Hmm, why do you say that?  Those documents should have that flag on their
> document node, which should make it inherited by every node in the document,
> right?

Ah, actually, I was wrong.  Setting the document to editable doesn't change the flags on every node in the document, it just sets NODE_IS_EDITABLE on the document node itself.  So, calling nsINode::IsEditable is enough (I thought that you're talking about nsIEditor::IsEditable, hence my comments).

> > That is, we need a check which _will_ catch nodes the editor might be messing
> > with (but might catch some others too).  And NODE_IS_EDITABLE has the exact
> > opposite property.
> 
> I don't really see where this conclusion is coming from.

But this still doesn't explain this part of your comment.
Comment 30 :Ehsan Akhgari 2010-10-04 13:25:06 PDT
This is also interesting: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#6138>.  However, I'm not sure if we want to do this here...
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2010-10-04 17:47:23 PDT
> But this still doesn't explain this part of your comment.

In an editable document, the individual nodes are not NODE_IS_EDITABLE.  So all NODE_IS_EDITABLE nodes are editable, but not all editable nodes are NODE_IS_EDITABLE.  Correct?

> However, I'm not sure if we want to do this here...

We don't need that complexity here, I think.
Comment 32 :Ehsan Akhgari 2010-10-05 11:38:58 PDT
(In reply to comment #31)
> > But this still doesn't explain this part of your comment.
> 
> In an editable document, the individual nodes are not NODE_IS_EDITABLE.  So all
> NODE_IS_EDITABLE nodes are editable, but not all editable nodes are
> NODE_IS_EDITABLE.  Correct?

Yes, but if you call nsINode::IsEditable on a node and it returns true, then the node is editable, otherwise it's not, right?
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2010-10-05 12:06:41 PDT
Yes, nsINode::IsEditable seems to be the test we want here; the question is how fast it is compared to what we have now.  Hence comment 26, which is what happened when I tried to write a test to evaluate that.
Comment 34 :Ehsan Akhgari 2010-10-05 17:22:00 PDT
(In reply to comment #33)
> Yes, nsINode::IsEditable seems to be the test we want here; the question is how
> fast it is compared to what we have now.  Hence comment 26, which is what
> happened when I tried to write a test to evaluate that.

Hmm, nsINode::IsEditable should be pretty cheap, as it just checks a couple of flags...  But yes, without measurements we can't tell if it's going to do us harm or good.

BTW, why do I get the feeling that half of the info about this bug is not in the comments?  Comment 26 is the only place in this bug where a zimbra test case is mentioned... ;-)
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2010-10-05 18:35:28 PDT
> as it just checks a couple of flags

Sure; the question is whether it's cheaper than HasAttr (in the 0-attribute limit; it's of course cheaper if the element has tons of attributes)... ;)

> Comment 26 is the only place in this bug where a zimbra test case is mentioned

Ah, sorry.  This issue initially came up when profiling zimbra stuff.  Those are the profiles comment 1 refers to.
Comment 36 :Ehsan Akhgari 2010-10-05 19:38:08 PDT
(In reply to comment #35)
> > Comment 26 is the only place in this bug where a zimbra test case is mentioned
> 
> Ah, sorry.  This issue initially came up when profiling zimbra stuff.  Those
> are the profiles comment 1 refers to.

OK, can you please let me know how I can reproduce that profile?  At any rate, it would be quite surprising if I can spend the time to fix it before 2.0...  :(
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2010-10-05 19:45:40 PDT
> OK, can you please let me know how I can reproduce that profile?

Mail sent.

> it would be quite surprising if I can spend the time to fix it before 2.0.

Makes sense.  I'll think a bit more about how to measure the impact.  I do wish we just didn't have to support this feature, though... at least in innerHTML.  Maybe we could just have a serializer flag to opt out of it or something?  That would be even better than trying to make the check fast.
Comment 38 :Ehsan Akhgari 2010-10-06 10:55:50 PDT
(In reply to comment #37)
> I do wish
> we just didn't have to support this feature, though... at least in innerHTML. 
> Maybe we could just have a serializer flag to opt out of it or something?  That
> would be even better than trying to make the check fast.

Which feature are you mentioning?
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2010-10-06 11:10:26 PDT
The one that involves us looking for this _moz_ attribute in CheckElementStart.
Comment 40 Olli Pettay [:smaug] (reviewing overload) 2010-10-06 11:15:39 PDT
So just for the mozdirty attribute we could easily add or replace it with
a boolean flag in nsIContent. For other -moz* or _moz* we should do something
else. But for performance, mozdirty is the most important one.
Comment 41 :Ehsan Akhgari 2010-10-06 11:46:22 PDT
(In reply to comment #40)
> So just for the mozdirty attribute we could easily add or replace it with
> a boolean flag in nsIContent. For other -moz* or _moz* we should do something
> else. But for performance, mozdirty is the most important one.

I think replacing moz-dirty would probably break some stuff in other applications (possibly editor widgets as well, as they may rely on all sorts of craziness in our editor), but adding a flag on the node in addition to setting the attr seems like a sane thing to do
Comment 42 Aryeh Gregor (:ayg) (away until October 25) 2012-04-17 06:13:47 PDT
Could someone clue me in on why we have mozdirty at all?  Why can't we just get rid of it entirely -- what would regress?  What gets pretty-printed differently based on it, and why should that happen?  Is it relevant to web content at all?  If it is, I want to spec it; if it's not, I don't get why it should affect performance on webpages.
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2012-04-17 08:48:31 PDT
> What gets pretty-printed differently based on it, and why should that happen? 

The relevant code in the serializer basically treats mozdirty as equivalent to mDoFormat being set.  The other way it's set is this:

  mDoFormat = (mFlags & nsIDocumentEncoder::OutputFormatted && !mDoRaw); 

Neither one has any effect if mDoRaw is set, which it is for innerHTML and the like.  Are there any web-visible methods that _do_ prettyprint?  I wouldn't think so....  Copy/paste might, though.
Comment 44 :Ehsan Akhgari 2012-04-17 15:46:32 PDT
Copy/paste does not use pretty print, but I think Composer might...
Comment 45 :Ehsan Akhgari 2012-04-17 15:47:37 PDT
Oh, and we definitely do not want to spec mozdirty!  ;-)
Comment 46 Aryeh Gregor (:ayg) (away until October 25) 2012-04-17 23:45:53 PDT
What does Composer use pretty-print for?  And why doesn't it pretty-print everything or nothing, instead of just the mozdirty stuff?  I'd really like to rip out this feature entirely.
Comment 47 Daniel Glazman (:glazou) 2012-04-17 23:52:31 PDT
Yes, serialization in Composer uses pretty-printing. But FWIW, pretty-printing
in the serializer suffers from enormous bugs or lacks. Line wrapping for instance
is vastly broken and I did a lot of improvements there for BlueGriffon.
But this bug is about _moz_dirty and I don't really use it. Some other may use
it though...
Comment 48 Boris Zbarsky [:bz] (still a bit busy) 2012-04-18 06:59:10 PDT
Aryeh, the point of an HTML editor is to edit HTML documents.  When doing that you generally:

1)  Do not want to reformat/prettyprint the stuff that was already in the document before
    you started editing (because your HTML editor may not be the only thing working with
    this HTML, for one thing).
2)  Do want to prettyprint the new things you add to the document so they don't end up all
    on one line or whatnot.
Comment 49 Aryeh Gregor (:ayg) (away until October 25) 2012-04-18 07:39:33 PDT
(In reply to Boris Zbarsky (:bz) from comment #48)
> Aryeh, the point of an HTML editor is to edit HTML documents.

So I guess my work to this point has focused exclusively on web use-cases, plus mail clients, which are a subset of web use-cases due to webmail.  This means writing and editing blog posts/forum posts/e-mail etc. in WYSIWYG fashion.  For these use-cases, pretty-printed HTML isn't essential.  In particular, it's not exposed to the web anywhere, and I've never heard anyone ask for it, which suggests it's not of much interest for the web use-cases.

Are there other important use-cases I'm not aware of?  Like, do people use the editor to write entire webpages from scratch?  Who wants or uses the pretty-printing features here?  Comment 47 suggests the feature isn't very useful as-is and needs to be reimplemented anyway.

> 1)  Do not want to reformat/prettyprint the stuff that was already in the
> document before
>     you started editing (because your HTML editor may not be the only thing
> working with
>     this HTML, for one thing).

If other things can't handle pretty-printing of old content, they won't handle pretty-printing of new content either.  Also, the point of pretty-printing is to make the source code human-readable, and it's not very helpful to have only half of it readable.

> 2)  Do want to prettyprint the new things you add to the document so they
> don't end up all
>     on one line or whatnot.

See above.  If this is used by Composer, maybe someone who works on Composer can tell us why it's needed?  If we're going to use the editor code for Composer as well as for contenteditable (which as far as I'm concerned is by far the primary use-case), then I'd expect we should be working with Composer people to identify their needs.  Based on discussion so far in this bug, I have no evidence that anyone wants or uses mozdirty-based pretty-printing.  Maybe someone does, but then I'd like to hear from them.
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2012-04-18 07:49:04 PDT
> Like, do people use the editor to write entire webpages from scratch?

Yes, exactly.

> If other things can't handle pretty-printing of old content

It's not a matter of "can't handle".  Think simple case of a web page being edited by two people, with a version control system involved.   One hand-writes HTML, one uses an HTML editor.  In many cases they're actually the same person.  If the HTML editor kept totally reformatting the hand-written parts, there would be quite a bit of annoyance.  And there has been in the past; just read all the bugs we have about our serializer reformatting people's source when they didn't want it reformatting.

I'll defer to glazou on the details, but this set of use cases why mozdirty exists to start with, afaik.
Comment 51 Daniel Glazman (:glazou) 2012-04-18 08:49:55 PDT
Created attachment 616160 [details]
bluegriffon preferences screenshot

(In reply to Boris Zbarsky (:bz) from comment #50)

> It's not a matter of "can't handle".  Think simple case of a web page being
> edited by two people, with a version control system involved.   One
> hand-writes HTML, one uses an HTML editor.  In many cases they're actually
> the same person.  If the HTML editor kept totally reformatting the
> hand-written parts, there would be quite a bit of annoyance.  And there has
> been in the past; just read all the bugs we have about our serializer
> reformatting people's source when they didn't want it reformatting.
> 
> I'll defer to glazou on the details, but this set of use cases why mozdirty
> exists to start with, afaik.

I am attaching a screenshot of bluegriffon's preferences.
Basically, the auto-indent and wrap options trigger the pretty-printing.
It's obviously not a desirable behaviour in asian languages but in
western languages, most users of Wysiwyg editors appear to run Tidy after
their edits. So this was needed in BlueGriffon. And that could the case
for all embedders of the editor.
Pretty-printing in libeditor should be debugged and probably reworked from
scratch. It should definitely not be removed.
Comment 52 :Ehsan Akhgari 2012-04-18 16:25:09 PDT
Gecko's HTML editor is used for at least three different purposes (I've heard people also build XML editors on top of it, but I don't know much about that.)

1. contenteditable/designMode support for the web.
2. editing HTML mail (used by Thunderbird.
3. editing HTML documents for desktop HTML editors (such as Composer, Kompozer, BlueGriffon, etc.)

While my focus has been the case 1 above, I don't think we can start breaking the other two use cases at will.  But it is possible to hide HTML editor functionality from the web by checking to see which case we're in at runtime.

If the editor has been created in the second mode, it has the nsIPlaintextEditor::eEditorMailMask flag.  If it has been created in the third mode, it lacks the nsIPlaintextEditor::eEditorAllowInteraction flag.  So it would be perfectly fine if you write a patch which would hide mozdirty in the first case above based on these flags.
Comment 53 Aryeh Gregor (:ayg) (away until October 25) 2012-04-19 01:03:54 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #52)
> Gecko's HTML editor is used for at least three different purposes (I've
> heard people also build XML editors on top of it, but I don't know much
> about that.)
> 
> 1. contenteditable/designMode support for the web.
> 2. editing HTML mail (used by Thunderbird.

(2) is conceptually a subset of (1), because a major use-case for (1) is webmail, and webmail will want to be able to do anything Thunderbird does.

> 3. editing HTML documents for desktop HTML editors (such as Composer,
> Kompozer, BlueGriffon, etc.)

This is a very separate use-case and I'm not nearly as familiar with it.

> While my focus has been the case 1 above, I don't think we can start
> breaking the other two use cases at will.  But it is possible to hide HTML
> editor functionality from the web by checking to see which case we're in at
> runtime.
> 
> If the editor has been created in the second mode, it has the
> nsIPlaintextEditor::eEditorMailMask flag.  If it has been created in the
> third mode, it lacks the nsIPlaintextEditor::eEditorAllowInteraction flag. 
> So it would be perfectly fine if you write a patch which would hide mozdirty
> in the first case above based on these flags.

That's what I was about to suggest before I saw your post.  Thunderbird probably doesn't need the pretty-printing either, does it?  So I'd say that if nsIPlaintextEditor::eEditorAllowInteraction is set, we shouldn't generate mozdirty flags and shouldn't check for them when serializing text.  Does that sound reasonable?  On webpages, we shouldn't be adding these extra attributes to start with -- although they vanish in .innerHTML, they're still visible in .attributes and maybe other places.
Comment 54 Daniel Glazman (:glazou) 2012-04-19 01:34:45 PDT
> > 1. contenteditable/designMode support for the web.
> > 2. editing HTML mail (used by Thunderbird.
> 
> (2) is conceptually a subset of (1), because a major use-case for (1) is
> webmail, and webmail will want to be able to do anything Thunderbird does.

No. Webmail does not have to implement many things Thunderbird has to implement.

> That's what I was about to suggest before I saw your post.  Thunderbird
> probably doesn't need the pretty-printing either, does it?  So I'd say that

Ah? On the contrary, when rich text is selected for new messages in TB,
pretty printing is probably the best choice for serialization.
Comment 55 Aryeh Gregor (:ayg) (away until October 25) 2012-04-19 05:07:07 PDT
(In reply to Daniel Glazman from comment #54)
> No. Webmail does not have to implement many things Thunderbird has to
> implement.

Such as?

> Ah? On the contrary, when rich text is selected for new messages in TB,
> pretty printing is probably the best choice for serialization.

I can do this, but why?  Who reads the source of HTML e-mails?  E-mail clients tend to generate horrifyingly ugly HTML.
Comment 56 Aryeh Gregor (:ayg) (away until October 25) 2012-04-19 06:14:33 PDT
Created attachment 616537 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsEditor::MarkNodeDirty
Comment 57 Aryeh Gregor (:ayg) (away until October 25) 2012-04-19 06:20:13 PDT
Created attachment 616539 [details] [diff] [review]
Patch part 2, v0.9 -- Don't emit or process _moz_dirty more than necessary

What I can confirm:

* After applying this patch, I don't see _moz_dirty in my browser anymore.

What I don't know:

* Whether this breaks Thunderbird or Composer.
* How to test this in a way that will give correct results both for us and Thunderbird/Composer (they run our tests too, right?).
* Whether this causes failure of any existing tests.
* How to ask for review when one patch touches two components.  (Should I break this into two patches?)
Comment 58 Aryeh Gregor (:ayg) (away until October 25) 2012-04-19 06:21:32 PDT
Also something I don't know:

* Whether this actually fixes the performance problem (although I can't see why it wouldn't).
Comment 59 Mozilla RelEng Bot 2012-04-19 06:25:01 PDT
Autoland Patchset:
	Patches: 616537, 616539
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=351a367b9436
Try run started, revision 351a367b9436. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=351a367b9436
Comment 60 Daniel Glazman (:glazou) 2012-04-19 06:26:20 PDT
(In reply to Aryeh Gregor from comment #55)
> (In reply to Daniel Glazman from comment #54)
> > No. Webmail does not have to implement many things Thunderbird has to
> > implement.
> 
> Such as?

A Webmail will use execCommand() to act on the contenteditable area while
Thunderbird can use any of nsIHTMLEditor/nsIPlaintextEditor/nsIEditor/... methods?
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2012-04-19 07:33:52 PDT
> (2) is conceptually a subset of (1), because a major use-case for (1) is webmail, and
> webmail will want to be able to do anything Thunderbird does.

Unless it does it server-side or via script.  For example, serialization to format=flowed: should that be supported in the web-exposed serializer?

> * Whether this actually fixes the performance problem 

It probably does, but shouldn't the flag be flipped?  This patch changes behavior for all editor consumers by default, unless they change their code; is that what we actually want?
Comment 62 :Ehsan Akhgari 2012-04-19 10:58:46 PDT
Comment on attachment 616539 [details] [diff] [review]
Patch part 2, v0.9 -- Don't emit or process _moz_dirty more than necessary

Review of attachment 616539 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsIDocumentEncoder.idl
@@ +67,5 @@
>     */
>    nsIDOMNode fixupNode(in nsIDOMNode aNode, out boolean aSerializeCloneKids);
>  };
>  
> +[scriptable, uuid(52c8c5b2-e0d1-4e52-a073-e216105aae71)]

You don't need to rev the uuid for changes which do not affect the vtable of the interface.

::: editor/idl/nsIEditor.idl
@@ +500,5 @@
> +  /**
> +   * Returns true if markNodeDirty() has any effect.  Returns false if
> +   * markNodeDirty() is a no-op.
> +   */
> +  [notxpcom] boolean outputsMozDirty();

Hmm, you can just get the flags from nsIEditor.  Let's not add to nsIEditor for this.

::: editor/libeditor/base/nsEditor.cpp
@@ +1273,5 @@
> +bool
> +nsEditor::OutputsMozDirty()
> +{
> +  return (mFlags & ~nsIPlaintextEditor::eEditorAllowInteraction) ||
> +         (mFlags & nsIPlaintextEditor::eEditorMailMask);

Please add a comment here explaining what you're checking for.
Comment 63 :Ehsan Akhgari 2012-04-19 10:59:42 PDT
(In reply to Boris Zbarsky (:bz) from comment #61)
> > * Whether this actually fixes the performance problem 
> 
> It probably does, but shouldn't the flag be flipped?  This patch changes
> behavior for all editor consumers by default, unless they change their code;
> is that what we actually want?

How come?
Comment 64 Boris Zbarsky [:bz] (still a bit busy) 2012-04-19 11:02:48 PDT
How come which?

With this patch, the serializer ignores _moz_dirty unless you explicitly pass in the new flag to the document encoder, right?
Comment 65 :Ms2ger (⌚ UTC+1/+2) 2012-04-19 11:15:31 PDT
Comment on attachment 616537 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsEditor::MarkNodeDirty

Review of attachment 616537 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +1270,5 @@
>  }
>  
>  
>  NS_IMETHODIMP
> +nsEditor::MarkNodeDirty(nsIDOMNode *aNode)

* to the left

@@ +1275,2 @@
>  {  
> +  nsCOMPtr<nsIContent> element(do_QueryInterface(aNode));

nsCOMPtr<nsIContent> element = do_QueryInterface(aNode);

(Though I would QI to dom::Element, then you don't do useless work for non-element nodes.)
Comment 66 Mozilla RelEng Bot 2012-04-19 11:31:47 PDT
Try run for 351a367b9436 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=351a367b9436
Results (out of 221 total builds):
    exception: 1
    success: 187
    warnings: 31
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-351a367b9436
Comment 67 :Ehsan Akhgari 2012-04-19 11:56:42 PDT
(In reply to Boris Zbarsky (:bz) from comment #64)
> How come which?
> 
> With this patch, the serializer ignores _moz_dirty unless you explicitly
> pass in the new flag to the document encoder, right?

Yes, and we use the flags set on the editor to explicitly pass that new flag to the document encoder, right?  (I could be missing something here.)
Comment 68 Boris Zbarsky [:bz] (still a bit busy) 2012-04-19 12:12:02 PDT
In GetMarkup, yes.  Are there no other cases that serialize editors and care about the mozdirty?
Comment 69 :Ehsan Akhgari 2012-04-19 12:29:54 PDT
I'm not sure, but now I do see your point.  Yeah, maybe we should make the document encoder to respect mozditry by default unless a flag tells it not to, and then set that flag in the call sites where we know we don't care about mozdirty.
Comment 70 Boris Zbarsky [:bz] (still a bit busy) 2012-04-19 21:50:43 PDT
Comment on attachment 616539 [details] [diff] [review]
Patch part 2, v0.9 -- Don't emit or process _moz_dirty more than necessary

Per comments.
Comment 71 Aryeh Gregor (:ayg) (away until October 25) 2012-04-20 00:55:42 PDT
(In reply to Boris Zbarsky (:bz) from comment #61)
> It probably does, but shouldn't the flag be flipped?  This patch changes
> behavior for all editor consumers by default, unless they change their code;
> is that what we actually want?

I didn't think about that.  You're right, it would make more sense reversed.

(In reply to Ehsan Akhgari [:ehsan] from comment #62)
> You don't need to rev the uuid for changes which do not affect the vtable of
> the interface.

Okay, but I have no idea what changes affect the vtable of the interface.  :)

> 
> ::: editor/idl/nsIEditor.idl
> @@ +500,5 @@
> > +  /**
> > +   * Returns true if markNodeDirty() has any effect.  Returns false if
> > +   * markNodeDirty() is a no-op.
> > +   */
> > +  [notxpcom] boolean outputsMozDirty();
> 
> Hmm, you can just get the flags from nsIEditor.  Let's not add to nsIEditor
> for this.

I don't follow -- what do you mean?  We don't want callers looking at the editor's flags directly, because in the future we might change the criteria for emitting _moz_dirty, in which case we don't want to have to track down and update callers.

> ::: editor/libeditor/base/nsEditor.cpp
> @@ +1273,5 @@
> > +bool
> > +nsEditor::OutputsMozDirty()
> > +{
> > +  return (mFlags & ~nsIPlaintextEditor::eEditorAllowInteraction) ||
> > +         (mFlags & nsIPlaintextEditor::eEditorMailMask);
> 
> Please add a comment here explaining what you're checking for.

Um, I would, but I have no idea what I'm checking for.  I'm just doing what you said in comment 52 without understanding it.  :)

(In reply to Ms2ger from comment #65)
> ::: editor/libeditor/base/nsEditor.cpp
> @@ +1270,5 @@
> >  }
> >  
> >  
> >  NS_IMETHODIMP
> > +nsEditor::MarkNodeDirty(nsIDOMNode *aNode)
> 
> * to the left

Oh, drat, is that Gecko style?  I'd been converting them to be the other way.  Now I see that <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Declarations> does say that, although it also explains why it's possibly a bad idea.  Okay, I'll switch it.

> @@ +1275,2 @@
> >  {  
> > +  nsCOMPtr<nsIContent> element(do_QueryInterface(aNode));
> 
> nsCOMPtr<nsIContent> element = do_QueryInterface(aNode);
> 
> (Though I would QI to dom::Element, then you don't do useless work for
> non-element nodes.)

Okay.


I'll upload a new part 2 when Ehsan clarifies whether he really wants me to remove the outputsMozDirty method, and what comment he wants; and when someone tells me what kind of tests I should write here.  Should I write a mochitest that checks for the absence of _moz_dirty after running an execCommand()?  Anything else?
Comment 72 Aryeh Gregor (:ayg) (away until October 25) 2012-04-20 00:56:30 PDT
Created attachment 616899 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsEditor::MarkNodeDirty

Per Ms2ger's feedback.
Comment 73 :Ehsan Akhgari 2012-04-20 11:33:59 PDT
(In reply to Aryeh Gregor from comment #71)
> (In reply to Ehsan Akhgari [:ehsan] from comment #62)
> > You don't need to rev the uuid for changes which do not affect the vtable of
> > the interface.
> 
> Okay, but I have no idea what changes affect the vtable of the interface.  :)

Basically adding/removing/reordering of methods.  Also, if you change the parameters of a method in any way you should rev the IID as well for binary compat (even though doing that will not change the vtable entries.)

> > Hmm, you can just get the flags from nsIEditor.  Let's not add to nsIEditor
> > for this.
> 
> I don't follow -- what do you mean?  We don't want callers looking at the
> editor's flags directly, because in the future we might change the criteria
> for emitting _moz_dirty, in which case we don't want to have to track down
> and update callers.

OK, that's fair.

> > Please add a comment here explaining what you're checking for.
> 
> Um, I would, but I have no idea what I'm checking for.  I'm just doing what
> you said in comment 52 without understanding it.  :)

Oh.  You're checking to see if the editor is being used by mailnews or composer.  :-)  (which is what those two checks do, respectively.)

> (In reply to Ms2ger from comment #65)
> > ::: editor/libeditor/base/nsEditor.cpp
> > @@ +1270,5 @@
> > >  }
> > >  
> > >  
> > >  NS_IMETHODIMP
> > > +nsEditor::MarkNodeDirty(nsIDOMNode *aNode)
> > 
> > * to the left
> 
> Oh, drat, is that Gecko style?  I'd been converting them to be the other
> way.  Now I see that
> <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Declarations>
> does say that, although it also explains why it's possibly a bad idea. 
> Okay, I'll switch it.

Personally, I don't care about that at all.  For a while I did, but that caused my own coding habits to be ruined, so now I attach * and & randomly to the left or right in the code I write myself!  ;-)

> I'll upload a new part 2 when Ehsan clarifies whether he really wants me to
> remove the outputsMozDirty method, and what comment he wants; and when
> someone tells me what kind of tests I should write here.  Should I write a
> mochitest that checks for the absence of _moz_dirty after running an
> execCommand()?  Anything else?

In addition to that, you want to create two more tests.  One should use a xul:editor (which will unset the interactive flag -- same as what Composer does) in a mochitest-chrome to make sure that moz-dirty is preserved in that case.  Another test to set the mail flag on the editor manually and then make sure that moz-dirty is present again.
Comment 74 :Ehsan Akhgari 2012-04-20 11:35:32 PDT
Comment on attachment 616899 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsEditor::MarkNodeDirty

Review of attachment 616899 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, you don't need to request review again on a patch that I mark r+.  Basically I'll trust you to fix the stylistic changes.  But if you feel that the changes are so pervasive that you'd want me to take a look again, please do set the flag and I will happily re-review.  :-)

Thanks!
Comment 75 Aryeh Gregor (:ayg) (away until October 25) 2012-04-22 01:37:41 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #73)
> Basically adding/removing/reordering of methods.  Also, if you change the
> parameters of a method in any way you should rev the IID as well for binary
> compat (even though doing that will not change the vtable entries.)

Okay.

> Oh.  You're checking to see if the editor is being used by mailnews or
> composer.  :-)  (which is what those two checks do, respectively.)

Okay.

> Personally, I don't care about that at all.  For a while I did, but that
> caused my own coding habits to be ruined, so now I attach * and & randomly
> to the left or right in the code I write myself!  ;-)

Well, that certainly seems to match the coding convention in editor/.  :)

> In addition to that, you want to create two more tests.  One should use a
> xul:editor (which will unset the interactive flag -- same as what Composer
> does) in a mochitest-chrome to make sure that moz-dirty is preserved in that
> case.  Another test to set the mail flag on the editor manually and then
> make sure that moz-dirty is present again.

Okay, I'll see if I can figure out how to do those.

(In reply to Ehsan Akhgari [:ehsan] from comment #74)
> FWIW, you don't need to request review again on a patch that I mark r+. 
> Basically I'll trust you to fix the stylistic changes.  But if you feel that
> the changes are so pervasive that you'd want me to take a look again, please
> do set the flag and I will happily re-review.  :-)
> 
> Thanks!

All right, noted.  I wasn't sure how far I could take an r+ without re-requesting review.
Comment 76 :Ehsan Akhgari 2012-04-22 12:42:31 PDT
(In reply to Aryeh Gregor from comment #75)
> > In addition to that, you want to create two more tests.  One should use a
> > xul:editor (which will unset the interactive flag -- same as what Composer
> > does) in a mochitest-chrome to make sure that moz-dirty is preserved in that
> > case.  Another test to set the mail flag on the editor manually and then
> > make sure that moz-dirty is present again.
> 
> Okay, I'll see if I can figure out how to do those.

You can use this test as an example: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug616590.xul>
Comment 77 Aryeh Gregor (:ayg) (away until October 25) 2012-04-23 02:51:52 PDT
Created attachment 617434 [details] [diff] [review]
Patch part 2, v1 -- Don't emit or process _moz_dirty more than necessary

Disclaimer: I only half-understand this patch, but it seems to work.  :)
Comment 78 Mozilla RelEng Bot 2012-04-23 02:57:36 PDT
Autoland Patchset:
	Patches: 616899, 617434
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=c26a15628437
Try run started, revision c26a15628437. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=c26a15628437
Comment 79 :Ehsan Akhgari 2012-04-23 07:35:06 PDT
Comment on attachment 617434 [details] [diff] [review]
Patch part 2, v1 -- Don't emit or process _moz_dirty more than necessary

Review of attachment 617434 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/tests/test_bug599983.xul
@@ +27,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var allowInteraction = Components.interfaces.nsIPlaintextEditor
> +                                              .eEditorAllowInteraction;
> +  var mailMask = Components.interfaces.nsIPlaintextEditor.eEditorMailMask;

Nit: please make these const, and rename to kFooBar.

@@ +50,5 @@
> +    editorObj.flags = (setAllowInteraction ? allowInteraction : 0) |
> +                      (setMailMask ? mailMask : 0);
> +
> +    var editorDoc = editorElem.contentDocument;
> +    editorDoc.designMode = 'on';

Hmm, this should not be necessary, the document should be editable already.
Comment 80 Aryeh Gregor (:ayg) (away until October 25) 2012-04-23 08:00:40 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #79)
> Hmm, this should not be necessary, the document should be editable already.

You're right -- I cargo-culted it from an example someplace, but it works the same without it.
Comment 81 Mozilla RelEng Bot 2012-04-23 08:01:07 PDT
Try run for c26a15628437 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c26a15628437
Results (out of 226 total builds):
    success: 186
    warnings: 37
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-c26a15628437
Comment 82 Boris Zbarsky [:bz] (still a bit busy) 2012-04-23 20:36:07 PDT
Comment on attachment 617434 [details] [diff] [review]
Patch part 2, v1 -- Don't emit or process _moz_dirty more than necessary

r=me

Note You need to log in before you can comment on or make changes to this bug.