1147412 - Clean up SetInlinePropertyOnTextNode, GetInlinePropertyBase, RemoveInlinePropertyImpl, GetComputedStyle, IsCSSInvertible, Get*Property, GetCSSInlinePropertyBase, etc.

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Clean up nsHTMLEditor::SetInlinePropertyOnTextNode

I changed SetInlinePropertyOnTextNode from nsIDOMCharacterData to Text, not nsGenericDOMDataNode, because it's probably the intent judging by the name.  I don't think we really support comments anyway.  (Why does editor not use nsIDOMText when it wants text?)

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Clean up nsHTMLEditor::SetInlinePropertyOnTextNode

Missing a fix.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Make methods take nsINode*, not just nsIContent*

The old versions of these take nsIDOMNode*, so making these take nsIContent* makes porting unnecessarily annoying.  Anyway, it makes perfect sense to call these on a document -- they should just return false (and do AFAICT).

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 - Fix completely broken nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet implementation

I broke this in bug 757371 part 1 by adding an nsIContent* variant of this method that took a const aValue (as in param) instead of non-const (in-out param).  This means callers that expected aValue to be updated broke silently when ported to nsINode*, which cost me an hour or two of time to debug.  This patch just adds a non-const variant as well.  Is there a better way to do this?  Maybe I should just get rid of the const variant and require callers to copy the param themselves?

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 - Clean up nsHTMLEditor::GetInlinePropertyBase

Switching to the nsINode* versions of certain callees uncovered a bug, because that version has extra asserts: two callers were in some cases passing non-null aAttribute but null aValue, which theoretically might have caused a crash.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 6 - Get rid of unused method variant

(I skipped part 5 for now because it needs revision.)

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 7 - Remove nsHTMLCSSUtils::GetComputedStyle(nsIDOMElement*)

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 8 - Clean up nsHTMLCSSUtils::IsCSSInvertible

The misspelling has bugged me for ages!

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8583106 [details] [diff] [review]
Part 1 - Clean up nsHTMLEditor::SetInlinePropertyOnTextNode

Seems to cause test failures.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 - Clean up nsHTMLEditor::SetInlinePropertyOnTextNode

I swapped patches 1 and 2 so this one can use the methods fixed by the other one.  Original comment repeated here to save confusion:

(In reply to :Aryeh Gregor from comment #1)
> I changed SetInlinePropertyOnTextNode from nsIDOMCharacterData to Text, not
> nsGenericDOMDataNode, because it's probably the intent judging by the name. 
> I don't think we really support comments anyway.  (Why does editor not use
> nsIDOMText when it wants text?)

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 5 - Clean up nsHTMLEditor::RemoveInlinePropertyImpl

Would be nice to have range-based for loops for nsIContentIterator, but they don't seem as simple to make as I would have expected.  "auto& node" is the right variable type here when iterating through arrayOfNodes?  Maybe "auto&& node"?

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 - Clean up nsHTMLEditor::GetInlinePropertyBase

(In reply to :Aryeh Gregor from comment #5)
> Switching to the nsINode* versions of certain callees uncovered a bug,
> because that version has extra asserts: two callers were in some cases
> passing non-null aAttribute but null aValue, which theoretically might have
> caused a crash.

Actually, it was the assert that was wrong.  Sigh.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 9 - Convert some nsHTMLEditor members to Element

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 10 - Clean up nsHTMLCSSUtils::Get*Property, GetCSSInlinePropertyBase

For the error return in MarginPropertyAtomForIndent, I chose the error return value of marginLeft arbitrarily, just so it wouldn't crash.

I changed NodeIsBlockStatic to allow any nsINode*, not just Element*, to ease porting from the nsIDOMNode* variant.

This is the last patch in the series.  I'd post a try run, but try seems to be closed.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec75ae18bacf

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 - Fix completely broken nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet implementation

New version removes preexisting incorrect assert, which fails when there's no document.  (I wish we had a better way to deal with the case where there's no document -- there are tons of error paths that are just for that case, which is pathological anyway.)  The assert was apparently triggered in some cases by later patches.

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 9 - Convert some nsHTMLEditor members to Element

Ridiculous typo fixed.  This should finally pass all tests, hopefully.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1bab90be5f7

Comment 18

[(no longer active)]

Comment on attachment 8583790 [details] [diff] [review]
Part 1 - Make methods take nsINode*, not just nsIContent*

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

::: editor/libeditor/nsHTMLCSSUtils.cpp
@@ +355,5 @@
>  				   // nsHTMLEditUtils::SupportsAlignAttr
>  				   // brade: but it also checks for tbody,
>  				   // tfoot, thead Let's add the following
>  				   // elements here even if ALIGN has not the
>  				   // same meaning for them

Please fix the indentation.

Comment 19

[(no longer active)]

Comment on attachment 8585172 [details] [diff] [review]
Part 3 - Fix completely broken nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet implementation

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

Ouch!  Unfortunately, I don't know of a better way to do this in C++...

Comment 20

[(no longer active)]

Comment on attachment 8583960 [details] [diff] [review]
Part 5 - Clean up nsHTMLEditor::RemoveInlinePropertyImpl

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

::: editor/libeditor/nsHTMLEditorStyle.cpp
@@ +1377,2 @@
>              if (mHTMLCSSUtils->IsCSSInvertable(aProperty, aAttribute)) {
> +              nsAutoString value(NS_LITERAL_STRING("-moz-editor-invert-value"));

Nit: please use NS_NAMED_LITERAL_STRING instead if possible.

@@ +1399,5 @@
>            }
>          }
> +
> +        // Loop through the list, remove the property on each node
> +        for (auto& node : arrayOfNodes) {

I think auto& is the right type here.

@@ +1414,5 @@
> +              // in the ancestors of startNode carrying specified styles;
> +              // assume it comes from a rule and let's try to insert a span
> +              // "inverting" the style
> +              mHTMLCSSUtils->IsCSSInvertable(aProperty, aAttribute)) {
> +            nsAutoString value(NS_LITERAL_STRING("-moz-editor-invert-value"));

Ditto.

Comment 21

[(no longer active)]

Comment on attachment 8583982 [details] [diff] [review]
Part 10 - Clean up nsHTMLCSSUtils::Get*Property, GetCSSInlinePropertyBase

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

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +942,5 @@
>    }
>    return NS_OK;
>  }
>  
>  nsIAtom* MarginPropertyAtomForIndent(nsHTMLCSSUtils* aHTMLCSSUtils, nsIDOMNode* aNode) {

Nit: Can you please mark this function as static?  Thanks!

@@ +944,5 @@
>  }
>  
>  nsIAtom* MarginPropertyAtomForIndent(nsHTMLCSSUtils* aHTMLCSSUtils, nsIDOMNode* aNode) {
> +  nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> +  NS_ENSURE_TRUE(node || !aNode, nsGkAtoms::marginLeft);

If I audited the callers right, none of the should ever pass in a null node.  How about we make the second arg a reference and just assume it is non-null?  Can wait until a follow-up.

::: editor/libeditor/nsHTMLEditor.cpp
@@ +824,5 @@
>  
>    nsCOMPtr<nsINode> p = aNode->GetParentNode();
>  
>    while (p) {
> +    if (NodeIsBlockStatic(p)) {

Hmm, removing the IsElement() check here seems wrong...  Please fix it?

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> Hmm, removing the IsElement() check here seems wrong...  Please fix it?

Why?  NodeIsBlockStatic will only return true for elements, right?  (If not, a bunch of code I recently wrote is wrong!)

Comment 23

[(no longer active)]

(In reply to :Aryeh Gregor from comment #22)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #21)
> > Hmm, removing the IsElement() check here seems wrong...  Please fix it?
> 
> Why?  NodeIsBlockStatic will only return true for elements, right?  (If not,
> a bunch of code I recently wrote is wrong!)

Hmm, yeah you're right.  I don't remember why I said this, must have been confused!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc6084a5e9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/39728090c010
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2067f1154da
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3744869c546
https://hg.mozilla.org/integration/mozilla-inbound/rev/a46d1fc419ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3a5e7926bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/340010655de0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdffe75327da
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5c572f23b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/41984351a65b

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=900a21d93e6f

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ecc6084a5e9b
https://hg.mozilla.org/mozilla-central/rev/39728090c010
https://hg.mozilla.org/mozilla-central/rev/b2067f1154da
https://hg.mozilla.org/mozilla-central/rev/f3744869c546
https://hg.mozilla.org/mozilla-central/rev/a46d1fc419ca
https://hg.mozilla.org/mozilla-central/rev/2e3a5e7926bc
https://hg.mozilla.org/mozilla-central/rev/340010655de0
https://hg.mozilla.org/mozilla-central/rev/fdffe75327da
https://hg.mozilla.org/mozilla-central/rev/ce5c572f23b8
https://hg.mozilla.org/mozilla-central/rev/41984351a65b

Comment 27

[Nicholas Nethercote [inactive]]

This really doesn't look right:

> GK_ATOM(_moz_abspos, "_moz_activated")