Closed Bug 757371 Opened 12 years ago Closed 12 years ago

Regression: restoring type-in state can add redundant tags

Categories

(Core :: DOM: Editor, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15
Tracking Flags:
Tracking Status
firefox15 + verified

People

(Reporter: ayg, Assigned: ayg)

References

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

Patch part 2, v1 -- Improve correctness of nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet
7.96 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Do not insert style tags if the style is already applied
28.39 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 1, v2 -- Clean up nsHTMLEditRules::ReapplyCachedStyles
9.37 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 3, v2 -- Reuse existing style elements more aggressively
78.12 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
See bug 756984 comment 3, reported by Joe Sabash.  Steps to reproduce:

  1. Enable bold
  2. Type "a", "b", Backspace, "b"

Actual result: <b>a<b>b</b></b>
Expected result: <b>ab</b>

This was apparently caused by bug 590640.  We need to not restore type-in state if the desired state is already present.

With bug 748307 landed, the following should reproduce without manual intervention:

data:text/html,<!doctype html>
<div contenteditable></div>
<script>
getSelection().collapse(document.body.firstChild, 0);
document.execCommand("bold");
document.execCommand("inserttext", false, "ab");
document.execCommand("delete");
document.execCommand("inserttext", false, "b");
document.body.textContent = document.body.firstChild.innerHTML;
</script>

It should say "<b>ab</b>", not "<b>a<b>b</b></b>".
So the basic problem is that SetInlinePropertyOnNodeImpl doesn't check if the style is already set.  Fixing that causes a bunch of editing spec tests to newly pass, but also causes some regressions, which I'll have to look into before submitting a patch.  I'll probably have something ready tomorrow.
Blocks: editingspectests
Flags: in-testsuite+
Ugh -- the code here is really a huge mess.  So one problem is that IsCSSEquivalentToHTMLInlineStyleSet mostly ignores the passed value, so if we're trying to add <span style="font-weight: normal"> it will still return true if the text is bold and false if it's normal.  So bailing out if the style is "already set" is bogus in that case, unless I rewrite that to fix it.  Likewise for italic vs. oblique, it seems.  This will be harder to fix than I thought.

The right thing to do is to get rid of the distinction between "setting" and "removing" a property, and make "removing" a property just setting a different value (e.g., bold = normal instead of bold = bold).  That would probably be a lot of work, though.
I have patches that I think work, pretty much, but one of them causes some minor regressions.  I'm noting them here now so I still have the actual failure messages, so it's more convenient.  These are failures where the spec's required behavior is a bit dodgy and I'm not convinced it's right anyway:

4566 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["stylewithcss","false"],["bold",""]] "<span style=font-weight:400>fo[o</span><span style=font-weight:500>b]ar</span>" compare innerHTML; assert_equals: Unexpected innerHTML (after normalizing inline style) expected "<span style=\"font-weight:400\">fo<b>o</b></span><span style=\"font-weight:500\"><b>b</b>ar</span>" but got "<span style=\"font-weight:400\">fo<b>o</b></span><span style=\"font-weight:500\">bar</span>"
51527 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["stylewithcss","false"],["italic",""]] "fo[o<span style=font-style:oblique>b]ar</span>baz" compare innerHTML; assert_equals: Unexpected innerHTML (after normalizing inline style) expected "fo<i>o</i><span style=\"font-style:oblique\"><i>b</i>ar</span>baz" but got "fo<span style=\"font-style:oblique\">obar</span>baz"
51569 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["stylewithcss","false"],["italic",""]] "<span style=font-style:italic>fo[o</span><span style=font-style:oblique>b]ar</span>" queryCommandIndeterm("italic") after; assert_equals: Wrong result returned expected false but got true
51601 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["stylewithcss","false"],["italic",""]] "<span style=font-style:oblique>fo[o</span><span style=font-style:italic>b]ar</span>" queryCommandIndeterm("italic") after; assert_equals: Wrong result returned expected false but got true

This failure is a real regression, but a minor one.  It's basically bug 747889 -- CSS for fontSize is a *huge* headache.  I think it should be tolerated for the sake of fixing this bug.

15272 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["fontsize","4"]] "<font size=4>foo[bar]baz</font>" compare innerHTML; assert_equals: Unexpected innerHTML (after normalizing inline style) expected "<font size=\"4\">foobarbaz</font>" but got "<font size=\"4\">foo<font size=\"4\">bar</font>baz</font>"

(Actual patches to come, hopefully tomorrow if no more monkey wrenches crop up.)
This wound up having nothing to do with the actual bug, but hey, cleanup is cleanup.  This isn't just style fixes -- I'm now removing nsIDOM* and nsresult wherever possible in cleanup patches.  Someday maybe editor/ won't have to make anyone's eyes bleed . . .
Attachment #627899 - Flags: review?(ehsan)
Really I wanted to call this "Make nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet marginally less broken", but I thought that wasn't as professional.  ;)  It fixes two small cases where it was wrong, which wouldn't be so worthwhile in itself, but it prevents test regressions in subsequent patches.  It really needs to be rewritten from scratch to not be broken.
Attachment #627901 - Flags: review?(ehsan)
This patch does lots of nice things.  E.g., previously if you bolded
  <b>foo</b>[bar]<b>baz</b>
you'd get
  <b>foo[bar]</b><b>baz</b>
and now you get
  <b>foo[bar]baz</b>.

This does cause the minor regressions noted in comment 3, plus a few more added since then relating to font-style: oblique.
Attachment #627902 - Flags: review?(ehsan)
This fixes the actual bug.  With this series applied, the reproduction steps in comment #0 pass instead of failing.  Try: https://tbpl.mozilla.org/?tree=Try&rev=394707cfbd68
Attachment #627906 - Flags: review?(ehsan)
Comment on attachment 627902 [details] [diff] [review]
Patch part 3, v1 -- Reuse existing style elements more aggressively

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

::: editor/libeditor/base/nsEditor.cpp
@@ +1454,5 @@
> +  // the current implementation requires it.
> +  MOZ_ASSERT(aNodeToKeep && aNodeToMove && aNodeToMove->GetNodeParent());
> +  nsresult res = JoinNodes(aNodeToKeep->AsDOMNode(), aNodeToMove->AsDOMNode(),
> +                           aNodeToMove->GetNodeParent()->AsDOMNode());
> +  MOZ_ASSERT(NS_SUCCEEDED(res));

It seems that this can fail in a number of places...
(In reply to :Ms2ger from comment #8)
> It seems that this can fail in a number of places...

Perhaps -- I was hoping tests would catch it, if so.  But I guess people don't like it when their debug builds crash.  I'm fine if it only gets hit in tests, not actual debug builds people are using, but I guess there's no easy way to do that right now?

Also, it seems that optimized builds fail with that pattern, because res is unused.  I'll fix that when I have a chance, probably tomorrow.
Sorry, I meant to get to this review today, but I couldn't.  I'll review it tomorrow.
Comment on attachment 627899 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLEditRules::ReapplyCachedStyles

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

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +1084,5 @@
> +  nsAutoString value(aValue);
> +  nsresult res = IsCSSEquivalentToHTMLInlineStyleSet(aContent->AsDOMNode(),
> +                                                     aProperty, aAttribute,
> +                                                     isSet, value, aStyleType);
> +  MOZ_ASSERT(NS_SUCCEEDED(res));

So...  What makes you think that IsCSSEquivalentToHTMLInlineStyleSet always returns NS_OK?  It does have code paths which can return other values, and if that happens, you'll end up eating the error message here.  r- because of this (and other similar places in this patch) unless you can demonstrate that the return value is always NS_OK, in which case you should change it to void.  :-)

::: editor/libeditor/html/nsHTMLCSSUtils.h
@@ +186,5 @@
>      * @param aValueString   [IN/OUT] the attribute value (in) the list of css values (out)
>      * @param aStyleType     [IN] SPECIFIED_STYLE_TYPE to query the specified style values
> +    *                            COMPUTED_STYLE_TYPE  to query the computed style values
> +    *
> +    * The nsINode variant returns aIsSet instead of using an out parameter.

Nit: nsIDOMNode.
Attachment #627899 - Flags: review?(ehsan) → review-
Attachment #627901 - Flags: review?(ehsan) → review+
Comment on attachment 627902 [details] [diff] [review]
Patch part 3, v1 -- Reuse existing style elements more aggressively

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

Also, please file follow-up bugs for the regressions...

::: editor/libeditor/base/nsEditor.cpp
@@ +1454,5 @@
> +  // the current implementation requires it.
> +  MOZ_ASSERT(aNodeToKeep && aNodeToMove && aNodeToMove->GetNodeParent());
> +  nsresult res = JoinNodes(aNodeToKeep->AsDOMNode(), aNodeToMove->AsDOMNode(),
> +                           aNodeToMove->GetNodeParent()->AsDOMNode());
> +  MOZ_ASSERT(NS_SUCCEEDED(res));

Indeed, and r- because of that.  Note that this is not only because of debug builds crashing, but (more importantly) because of non-debug builds failing to catch an error.

The same goes with other similar MOZ_ASSERT's on the nsresults elsewhere in the patch.

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +264,5 @@
> +    if (element->IsHTML(aProperty) && IsOnlyAttribute(element, *aAttribute) &&
> +        element->AttrValueIs(kNameSpaceID_None, atom, *aValue, eIgnoreCase)) {
> +      // This is not quite correct, because it excludes cases like
> +      // <font face=000> being the same as <font face=#000000>.
> +      // Property-specific handling is needed.

Please file a follow-up bug for this, and note the bug number in the comment.
Attachment #627902 - Flags: review?(ehsan) → review-
Comment on attachment 627906 [details] [diff] [review]
Patch part 4, v1 -- Do not insert style tags if the style is already applied

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

Looks good, but please add a testcase which tests the steps in comment 0.  r=me with that.

::: editor/libeditor/html/nsHTMLEditor.h
@@ +504,5 @@
>      *                   May be null.  Ignored if aAttribute is null.
>      * @param aIsSet     [OUT] true if <aProperty aAttribute=aValue> effects aNode.
>      * @param outValue   [OUT] the value of the attribute, if aIsSet is true
> +    *
> +    * The nsINode variant returns aIsSet instead of using an out parameter.

Nit: nsIDOMNode.
Attachment #627906 - Flags: review?(ehsan) → review+
(In reply to Aryeh Gregor from comment #9)
> (In reply to :Ms2ger from comment #8)
> > It seems that this can fail in a number of places...
> 
> Perhaps -- I was hoping tests would catch it, if so.  But I guess people
> don't like it when their debug builds crash.  I'm fine if it only gets hit
> in tests, not actual debug builds people are using, but I guess there's no
> easy way to do that right now?

No.  In general we don't generate different code for builds which get tested.  (We even build our release binaries with --enable-tests.)

> Also, it seems that optimized builds fail with that pattern, because res is
> unused.  I'll fix that when I have a chance, probably tomorrow.

You can use mozilla::DebugOnly to fix that, but I'm not convinced that the MOZ_ASSERTs are a good idea, as I've noted above.
Depends on: 760211
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> So...  What makes you think that IsCSSEquivalentToHTMLInlineStyleSet always
> returns NS_OK?

Well, the test suite passes.  0:)

More seriously: I don't, but it should.  There's no reason an error should occur that has to be handled by the caller.  With the patch, if such an error occurs in a test it will crash, which is good, because then we can fix it by handling it properly.  If such an error occurs in a production build, the function will just return false, which IMO is fine -- it doesn't really handle the error, but throwing an incomprehensible exception all the way back up to JavaScript (or to a higher caller that ignores the error) doesn't really handle the error either.

The one thing I admit isn't very nice here is that this will crash debug builds, which is a nuisance to other developers.  Would you be okay with me changing it to an NS_ASSERTION, plus a comment in each case noting how we handle the error?  E.g., in this case, something like

+  // If this fails for some reason, log an assertion and return false
+  NS_ASSERTION(NS_SUCCEEDED(IsCSSEquivalentToHTMLInlineStyleSet(
+    aContent->AsDOMNode(), aProperty, aAttribute, isSet, value, aStyleType)))
+  return isSet;

Similarly for other cases -- asserting and then also making sure something more or less reasonable happens after that (but not necessarily returning nsresult).

Ms2ger and I have changed lots of methods from NS_ENSURE_TRUE(aSomething, NS_ERROR_NULL_POINTER) to MOZ_ASSERT(aSomething) and you've given r+ -- why is that better?  In those cases, it would previously return false and now will quite possibly crash the browser in production builds.  (Maybe we should have MOZ_ASSERT_OR_RETURN or such?)

> ::: editor/libeditor/html/nsHTMLCSSUtils.h
> @@ +186,5 @@
> >      * @param aValueString   [IN/OUT] the attribute value (in) the list of css values (out)
> >      * @param aStyleType     [IN] SPECIFIED_STYLE_TYPE to query the specified style values
> > +    *                            COMPUTED_STYLE_TYPE  to query the computed style values
> > +    *
> > +    * The nsINode variant returns aIsSet instead of using an out parameter.
> 
> Nit: nsIDOMNode.

You mean nsIContent, I guess?

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> Also, please file follow-up bugs for the regressions...

Bug 747889 should cover the only real one -- a fix for that should fix the regression too.  The other regressions are all in cases where I'm not at all sure the spec is actually sane, so I don't think we need to worry about them.

> Please file a follow-up bug for this, and note the bug number in the comment.

Bug 760211.  (I'll add it to the code comment too.)

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Nit: nsIDOMNode.

Again, I guess you must mean nsIContent.
(In reply to Aryeh Gregor from comment #15)
> (In reply to Ehsan Akhgari [:ehsan] from comment #11)
> > So...  What makes you think that IsCSSEquivalentToHTMLInlineStyleSet always
> > returns NS_OK?
> 
> Well, the test suite passes.  0:)
> 
> More seriously: I don't, but it should.  There's no reason an error should
> occur that has to be handled by the caller.  With the patch, if such an
> error occurs in a test it will crash, which is good, because then we can fix
> it by handling it properly.  If such an error occurs in a production build,
> the function will just return false, which IMO is fine -- it doesn't really
> handle the error, but throwing an incomprehensible exception all the way
> back up to JavaScript (or to a higher caller that ignores the error) doesn't
> really handle the error either.

My issue with doing this is that this will result in weird runtime bugs for non-debug builds.  For example, GetCSSEquivalentToHTMLInlineStyleSet can return an error code in a number of cases which would cause IsCSSEquivalentToHTMLInlineStyleSet fail as well, and I don't think it's safe to ignore such errors like that.

> The one thing I admit isn't very nice here is that this will crash debug
> builds, which is a nuisance to other developers.  Would you be okay with me
> changing it to an NS_ASSERTION, plus a comment in each case noting how we
> handle the error?  E.g., in this case, something like

My objection wasn't really about crashing debug builds.  Doing that is fine as long as the error is caused by a simple programming mistake (such as someone passing null to a method which doesn't accept it).  It's not fine for things which can fail because of unknown situations.

> +  // If this fails for some reason, log an assertion and return false
> +  NS_ASSERTION(NS_SUCCEEDED(IsCSSEquivalentToHTMLInlineStyleSet(
> +    aContent->AsDOMNode(), aProperty, aAttribute, isSet, value,
> aStyleType)))
> +  return isSet;

This code is not correct because NS_ASSERTION will be ifdef'ed out in non-debug builds.  :-)

> Similarly for other cases -- asserting and then also making sure something
> more or less reasonable happens after that (but not necessarily returning
> nsresult).
> 
> Ms2ger and I have changed lots of methods from NS_ENSURE_TRUE(aSomething,
> NS_ERROR_NULL_POINTER) to MOZ_ASSERT(aSomething) and you've given r+ -- why
> is that better?  In those cases, it would previously return false and now
> will quite possibly crash the browser in production builds.  (Maybe we
> should have MOZ_ASSERT_OR_RETURN or such?)

See above on why that is OK.

> > ::: editor/libeditor/html/nsHTMLCSSUtils.h
> > @@ +186,5 @@
> > >      * @param aValueString   [IN/OUT] the attribute value (in) the list of css values (out)
> > >      * @param aStyleType     [IN] SPECIFIED_STYLE_TYPE to query the specified style values
> > > +    *                            COMPUTED_STYLE_TYPE  to query the computed style values
> > > +    *
> > > +    * The nsINode variant returns aIsSet instead of using an out parameter.
> > 
> > Nit: nsIDOMNode.
> 
> You mean nsIContent, I guess?

Right.
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> My issue with doing this is that this will result in weird runtime bugs for
> non-debug builds.  For example, GetCSSEquivalentToHTMLInlineStyleSet can
> return an error code in a number of cases which would cause
> IsCSSEquivalentToHTMLInlineStyleSet fail as well, and I don't think it's
> safe to ignore such errors like that.

I agree, but I don't think returning false is ignoring the error.  It's no more likely to cause harmful bugs than returning an error code.  In this case, ReapplyCachedStyles was already ignoring the error code!  At least the new version will assert in debug builds.  And if ReapplyCachedStyles didn't ignore the error code, it would probably propagate an exception up to JS, which might break the page, and would also be incomprehensible because it would just say something like NS_ERROR_NULL_POINTER.

So I think returning false here is just as valid a form of error handling as propagating up an error code (which in this specific case was being ignored anyway).  I think it's an improvement, because at least an NS_ASSERTION stands a chance of being caught in debug builds.  I agree with you on the general point that we have to handle errors properly in production builds too.

> My objection wasn't really about crashing debug builds.  Doing that is fine
> as long as the error is caused by a simple programming mistake (such as
> someone passing null to a method which doesn't accept it).  It's not fine
> for things which can fail because of unknown situations.

Okay, but returning false from a boolean function also seems okay, particularly if the alternative is that it keeps returning nsresult with an out parameter.

> > +  // If this fails for some reason, log an assertion and return false
> > +  NS_ASSERTION(NS_SUCCEEDED(IsCSSEquivalentToHTMLInlineStyleSet(
> > +    aContent->AsDOMNode(), aProperty, aAttribute, isSet, value,
> > aStyleType)))
> > +  return isSet;
> 
> This code is not correct because NS_ASSERTION will be ifdef'ed out in
> non-debug builds.  :-)

In which case it will return false, which IMO is fine in this specific case (although perhaps not in other cases I changed to assert instead of returning nsresult).
(In reply to Aryeh Gregor from comment #17)
> (In reply to Ehsan Akhgari [:ehsan] from comment #16)
> > My issue with doing this is that this will result in weird runtime bugs for
> > non-debug builds.  For example, GetCSSEquivalentToHTMLInlineStyleSet can
> > return an error code in a number of cases which would cause
> > IsCSSEquivalentToHTMLInlineStyleSet fail as well, and I don't think it's
> > safe to ignore such errors like that.
> 
> I agree, but I don't think returning false is ignoring the error.  It's no
> more likely to cause harmful bugs than returning an error code.  In this
> case, ReapplyCachedStyles was already ignoring the error code!  At least the
> new version will assert in debug builds.  And if ReapplyCachedStyles didn't
> ignore the error code, it would probably propagate an exception up to JS,
> which might break the page, and would also be incomprehensible because it
> would just say something like NS_ERROR_NULL_POINTER.
> 
> So I think returning false here is just as valid a form of error handling as
> propagating up an error code (which in this specific case was being ignored
> anyway).  I think it's an improvement, because at least an NS_ASSERTION
> stands a chance of being caught in debug builds.  I agree with you on the
> general point that we have to handle errors properly in production builds
> too.

Oh, ok, ReapplyCachedStyles ignoring the return value is a good argument!

> > > +  // If this fails for some reason, log an assertion and return false
> > > +  NS_ASSERTION(NS_SUCCEEDED(IsCSSEquivalentToHTMLInlineStyleSet(
> > > +    aContent->AsDOMNode(), aProperty, aAttribute, isSet, value,
> > > aStyleType)))
> > > +  return isSet;
> > 
> > This code is not correct because NS_ASSERTION will be ifdef'ed out in
> > non-debug builds.  :-)
> 
> In which case it will return false, which IMO is fine in this specific case
> (although perhaps not in other cases I changed to assert instead of
> returning nsresult).

No.  In non-debug builds, NS_ASSERTION is more or less defined as:

#define NS_ASSERTION(x)

which removed its argument.  In your above code, IsCSSEquivalentToHTMLInlineStyleSet would not be called at all in debug builds.

In general, passing expressions which have side effects to NS_ASSERTION and friends is almost always a mistake.
(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> No.  In non-debug builds, NS_ASSERTION is more or less defined as:
> 
> #define NS_ASSERTION(x)
> 
> which removed its argument.  In your above code,
> IsCSSEquivalentToHTMLInlineStyleSet would not be called at all in debug
> builds.
> 
> In general, passing expressions which have side effects to NS_ASSERTION and
> friends is almost always a mistake.

Oops.  That's what MOZ_ALWAYS_TRUE is for, I guess, right?  Feh.  Well, I can punt on these nsresult changes for now -- more important to get the actual bug fixes checked in.  I really want to incrementally remove nsresult usage somehow, without having to chase down to all the leaf functions.  I was looking at that for one function in editor/ and found that some possible errors came all the way from nsCOMArray having code-paths that returned false on allocation failures when you tried to expand the array . . .
No longer attempts to make ReapplyCachedStyles return void, but now the only failure path is through GetInlinePropertyBase.  The new IsCSSEquivalentToHTMLInlineStyleSet variant still returns a bool, and it does NS_ASSERTION() and NS_ENSURE_SUCCESS(res, false) on error.  This doesn't reduce error-checking, since ReapplyCachedStyles is the only caller and it didn't check the error before anyway.
Attachment #627899 - Attachment is obsolete: true
Attachment #629146 - Flags: review?(ehsan)
Comment on attachment 629146 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsHTMLEditRules::ReapplyCachedStyles

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

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +1084,5 @@
> +  nsAutoString value(aValue);
> +  nsresult res = IsCSSEquivalentToHTMLInlineStyleSet(aContent->AsDOMNode(),
> +                                                     aProperty, aAttribute,
> +                                                     isSet, value, aStyleType);
> +  NS_ASSERTION(NS_SUCCEEDED(res));

This NS_ASSERTION is of course wrong -- it has too few arguments and won't compile.  It was fixed by the new part 3 patch I had locally, so the patchset as a whole compiled, but I shuffled it around so it's correct in the original patch.
This also uses the NS_ASSERTION/NS_ENSURE_SUCCESS pattern.  If something is actually failing here, I want to be alerted, not just have a line printed to the console.

The new SetCSSEquivalentToHTMLStyle variant doesn't return an nsresult.  The only caller is the new IsModifiableNode helper, but the calls that replaces didn't previously check errors at all, so again, this is no real reduction in error checking.
Attachment #627902 - Attachment is obsolete: true
Attachment #629148 - Flags: review?(ehsan)
(In reply to Aryeh Gregor from comment #19)
> (In reply to Ehsan Akhgari [:ehsan] from comment #18)
> > No.  In non-debug builds, NS_ASSERTION is more or less defined as:
> > 
> > #define NS_ASSERTION(x)
> > 
> > which removed its argument.  In your above code,
> > IsCSSEquivalentToHTMLInlineStyleSet would not be called at all in debug
> > builds.
> > 
> > In general, passing expressions which have side effects to NS_ASSERTION and
> > friends is almost always a mistake.
> 
> Oops.  That's what MOZ_ALWAYS_TRUE is for, I guess, right?

Yes.

>  Feh.  Well, I
> can punt on these nsresult changes for now -- more important to get the
> actual bug fixes checked in.  I really want to incrementally remove nsresult
> usage somehow, without having to chase down to all the leaf functions.  I
> was looking at that for one function in editor/ and found that some possible
> errors came all the way from nsCOMArray having code-paths that returned
> false on allocation failures when you tried to expand the array . . .

Ugh!
Comment on attachment 629146 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsHTMLEditRules::ReapplyCachedStyles

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

r=me, with the fix you already pointed out.
Attachment #629146 - Flags: review?(ehsan) → review+
Attachment #629148 - Flags: review?(ehsan) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Verified as fixed with the STR from comment 0 on:
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0
status-firefox15: fixed → verified
Depends on: font_size_bloat
Keywords: regression
Depends on: 1272490
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: