Closed Bug 599983 Opened 14 years ago Closed 12 years ago

Remove use or optimize performance of moz attributes in editor and serializer

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: smaug, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

bluegriffon preferences screenshot
87.03 KB, image/png
Details
Patch part 1, v2 -- Clean up nsEditor::MarkNodeDirty
1.07 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Don't emit or process _moz_dirty more than necessary
13.16 KB, patch
ehsan.akhgari
: review+
bzbarsky
: review+
Details | Diff | Splinter Review 
Getting the moz attributes shows up in the GetInnerHTML profiles.
Can we do this for 2.0?  This one issue is about 10% of innerHTML time in the profiles I was looking at...
In particular, can we somehow make use of our editable node flags instead?
Can someone please augment this bug with some information on what it's actually about?
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?
Note that CheckElementEnd has similar code, btw.  And so does nsHTMLContentSerializer::AppendElementEnd.
(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.
Note, IIRC some code uses editors moz- attributes to style the content.
_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.
What is the usecase for mozdirty?
Could we use properties? (nsINode::SetProperty)
(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.
Would a prop test be faster than HasAttr?  Can we not just use a boolean flag on nsINode?
Yeah, I was looking at the property code too and perhaps it isn't faster.
Do we have any extra space in nsINode?
Seems like we do have, since
bool mNodeHasRenderingObservers : 1; has been added already.

(nsINode is getting quite heavy :/ )
> 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.  ;)
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.
Sure, but can we reliably and quickly depend editable content in the sense that matters here?
(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?
I don't know; that's the point!  If it is, perfect.
(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?
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".
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...
Assignee: nobody → ehsan
Is there more to do here than the NODE_IS_EDITABLE check?  'cause I can do that part..
(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!  ;-)
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....
(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.
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....
> 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.
(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.
(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.
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...
> 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.
(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?
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.
(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... ;-)
> 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.
(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...  :(
> 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.
(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?
The one that involves us looking for this _moz_ attribute in CheckElementStart.
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.
(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
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.
> 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.
Copy/paste does not use pretty print, but I think Composer might...
Oh, and we definitely do not want to spec mozdirty!  ;-)
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.
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...
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.
(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.
> 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.
(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.
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.
(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.
> > 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.
(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.
Assignee: ehsan → ayg
Status: NEW → ASSIGNED

        Attachment #616537 -
        Flags: review?(ehsan)

    
    

    
  


  
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?)

        Attachment #616539 -
        Flags: review?(ehsan)

        Attachment #616539 -
        Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]

    
    

    
  
Also something I don't know:

* Whether this actually fixes the performance problem (although I can't see why it wouldn't).
Hardware: x86 → All
Version: unspecified → Trunk

    
  
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

    
    

    
  
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

    
    

    
  
(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?

    
    

    
  
> (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?

    
  

        Attachment #616537 -
        Flags: review?(ehsan) → review+

    
    

    
  
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.

        Attachment #616539 -
        Flags: review?(ehsan) → review-

    
    

    
  
(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?

    
    

    
  
How come which?

With this patch, the serializer ignores _moz_dirty unless you explicitly pass in the new flag to the document encoder, right?

    
  
Blocks: 744830

    
    

    
  
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.)

    
    

    
  
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

    
  
Whiteboard: [autoland-in-queue]

    
    

    
  
(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.)

    
    

    
  
In GetMarkup, yes.  Are there no other cases that serialize editors and care about the mozdirty?

    
    

    
  
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 on attachment 616539 [details] [diff] [review]
Patch part 2, v0.9 -- Don't emit or process _moz_dirty more than necessary

Per comments.

        Attachment #616539 -
        Flags: review?(bzbarsky) → review-

    
    

    
  
(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?

    
    

    
  


  
Per Ms2ger's feedback.

        Attachment #616537 -
        Attachment is obsolete: true

        Attachment #616899 -
        Flags: review?(ehsan)

    
    

    
  
(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 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!

        Attachment #616899 -
        Flags: review?(ehsan) → review+

    
    

    
  
(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.

    
    

    
  
(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>

    
    

    
  


  
Disclaimer: I only half-understand this patch, but it seems to work.  :)

        Attachment #616539 -
        Attachment is obsolete: true

        Attachment #617434 -
        Flags: review?(ehsan)

        Attachment #617434 -
        Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]

    
  
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

    
    

    
  
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 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.

        Attachment #617434 -
        Flags: review?(ehsan) → review+

    
    

    
  
(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.

    
    

    
  
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

    
  
Whiteboard: [autoland-in-queue]

    
    

    
  
Comment on attachment 617434 [details] [diff] [review]
Patch part 2, v1 -- Don't emit or process _moz_dirty more than necessary

r=me

        Attachment #617434 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/12933772f0a9
https://hg.mozilla.org/mozilla-central/rev/b20ee61f3aed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
  
Depends on: 1205573

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: