Skip to content

Commit

Permalink
Bug 1619914 - part 1: Make each transaction class grab their members …
Browse files Browse the repository at this point in the history 
…with local variable before touching the DOM tree r=m_kato

Differential Revision: https://phabricator.services.mozilla.com/D69302

--HG--
extra : moz-landing-system : lando
  • Loading branch information
@masayuki-nakano
masayuki-nakano committed Apr 3, 2020
1 parent befee7a commit 3f95e60
Show file tree
Hide file tree
Showing 18 changed files with 398 additions and 334 deletions.
12 changes: 10 additions & 2 deletions editor/libeditor/CSSEditUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,8 +383,12 @@ bool CSSEditUtils::IsCSSEditableProperty(nsINode* aNode, nsAtom* aProperty,
nsresult CSSEditUtils::SetCSSProperty(Element& aElement, nsAtom& aProperty,
const nsAString& aValue,
bool aSuppressTxn) {
nsCOMPtr<nsStyledElement> styledElement = do_QueryInterface(&aElement);
if (NS_WARN_IF(!styledElement)) {
return NS_ERROR_INVALID_ARG;
}
RefPtr<ChangeStyleTransaction> transaction =
ChangeStyleTransaction::Create(aElement, aProperty, aValue);
ChangeStyleTransaction::Create(*styledElement, aProperty, aValue);
if (aSuppressTxn) {
return transaction->DoTransaction();
}
Expand Down Expand Up @@ -416,8 +420,12 @@ nsresult CSSEditUtils::SetCSSPropertyPixels(Element& aElement,
nsresult CSSEditUtils::RemoveCSSProperty(Element& aElement, nsAtom& aProperty,
const nsAString& aValue,
bool aSuppressTxn) {
nsCOMPtr<nsStyledElement> styledElement = do_QueryInterface(&aElement);
if (NS_WARN_IF(!styledElement)) {
return NS_ERROR_INVALID_ARG;
}
RefPtr<ChangeStyleTransaction> transaction =
ChangeStyleTransaction::CreateToRemove(aElement, aProperty, aValue);
ChangeStyleTransaction::CreateToRemove(*styledElement, aProperty, aValue);
if (aSuppressTxn) {
nsresult rv = transaction->DoTransaction();
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
Expand Down
24 changes: 18 additions & 6 deletions editor/libeditor/ChangeAttributeTransaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,36 +63,48 @@ NS_IMETHODIMP ChangeAttributeTransaction::DoTransaction() {

// Now set the attribute to the new value
if (mRemoveAttribute) {
nsresult rv = mElement->UnsetAttr(kNameSpaceID_None, mAttribute, true);
OwningNonNull<Element> element = *mElement;
nsresult rv = element->UnsetAttr(kNameSpaceID_None, mAttribute, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Element::UnsetAttr() failed");
return rv;
}

nsresult rv = mElement->SetAttr(kNameSpaceID_None, mAttribute, mValue, true);
OwningNonNull<Element> element = *mElement;
nsresult rv = element->SetAttr(kNameSpaceID_None, mAttribute, mValue, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Element::SetAttr() failed");
return rv;
}

NS_IMETHODIMP ChangeAttributeTransaction::UndoTransaction() {
if (NS_WARN_IF(!mElement)) {
return NS_ERROR_NOT_AVAILABLE;
}
if (mAttributeWasSet) {
OwningNonNull<Element> element = *mElement;
nsresult rv =
mElement->SetAttr(kNameSpaceID_None, mAttribute, mUndoValue, true);
element->SetAttr(kNameSpaceID_None, mAttribute, mUndoValue, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Element::SetAttr() failed");
return rv;
}
nsresult rv = mElement->UnsetAttr(kNameSpaceID_None, mAttribute, true);
OwningNonNull<Element> element = *mElement;
nsresult rv = element->UnsetAttr(kNameSpaceID_None, mAttribute, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Element::UnsetAttr() failed");
return rv;
}

NS_IMETHODIMP ChangeAttributeTransaction::RedoTransaction() {
if (NS_WARN_IF(!mElement)) {
return NS_ERROR_NOT_AVAILABLE;
}
if (mRemoveAttribute) {
nsresult rv = mElement->UnsetAttr(kNameSpaceID_None, mAttribute, true);
OwningNonNull<Element> element = *mElement;
nsresult rv = element->UnsetAttr(kNameSpaceID_None, mAttribute, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Element::UnsetAttr() failed");
return rv;
}

nsresult rv = mElement->SetAttr(kNameSpaceID_None, mAttribute, mValue, true);
OwningNonNull<Element> element = *mElement;
nsresult rv = element->SetAttr(kNameSpaceID_None, mAttribute, mValue, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Element::SetAttr() failed");
return rv;
}
Expand Down
49 changes: 28 additions & 21 deletions editor/libeditor/ChangeStyleTransaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,38 +24,40 @@ using namespace dom;

// static
already_AddRefed<ChangeStyleTransaction> ChangeStyleTransaction::Create(
Element& aElement, nsAtom& aProperty, const nsAString& aValue) {
nsStyledElement& aStyledElement, nsAtom& aProperty,
const nsAString& aValue) {
RefPtr<ChangeStyleTransaction> transaction =
new ChangeStyleTransaction(aElement, aProperty, aValue, false);
new ChangeStyleTransaction(aStyledElement, aProperty, aValue, false);
return transaction.forget();
}

// static
already_AddRefed<ChangeStyleTransaction> ChangeStyleTransaction::CreateToRemove(
Element& aElement, nsAtom& aProperty, const nsAString& aValue) {
nsStyledElement& aStyledElement, nsAtom& aProperty,
const nsAString& aValue) {
RefPtr<ChangeStyleTransaction> transaction =
new ChangeStyleTransaction(aElement, aProperty, aValue, true);
new ChangeStyleTransaction(aStyledElement, aProperty, aValue, true);
return transaction.forget();
}

ChangeStyleTransaction::ChangeStyleTransaction(Element& aElement,
ChangeStyleTransaction::ChangeStyleTransaction(nsStyledElement& aStyledElement,
nsAtom& aProperty,
const nsAString& aValue,
bool aRemove)
: EditTransactionBase(),
mElement(&aElement),
mStyledElement(&aStyledElement),
mProperty(&aProperty),
mValue(aValue),
mRemoveProperty(aRemove),
mUndoValue(),
mRedoValue(),
mRemoveProperty(aRemove),
mUndoAttributeWasSet(false),
mRedoAttributeWasSet(false) {}

#define kNullCh (char16_t('\0'))

NS_IMPL_CYCLE_COLLECTION_INHERITED(ChangeStyleTransaction, EditTransactionBase,
mElement)
mStyledElement)

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ChangeStyleTransaction)
NS_INTERFACE_MAP_END_INHERITING(EditTransactionBase)
Expand Down Expand Up @@ -141,20 +143,20 @@ void ChangeStyleTransaction::RemoveValueFromListOfValues(
}

NS_IMETHODIMP ChangeStyleTransaction::DoTransaction() {
// TODO: Change mElement to RefPtr<nsStyleElement>.
nsCOMPtr<nsStyledElement> inlineStyles = do_QueryInterface(mElement);
if (NS_WARN_IF(!inlineStyles)) {
return NS_ERROR_INVALID_ARG;
if (NS_WARN_IF(!mStyledElement)) {
return NS_ERROR_NOT_AVAILABLE;
}

nsCOMPtr<nsICSSDeclaration> cssDecl = inlineStyles->Style();
OwningNonNull<nsStyledElement> styledElement = *mStyledElement;
nsCOMPtr<nsICSSDeclaration> cssDecl = styledElement->Style();

// FIXME(bug 1606994): Using atoms forces a string copy here which is not
// great.
nsAutoCString propertyNameString;
mProperty->ToUTF8String(propertyNameString);

mUndoAttributeWasSet = mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::style);
mUndoAttributeWasSet =
mStyledElement->HasAttr(kNameSpaceID_None, nsGkAtoms::style);

nsAutoString values;
nsresult rv = cssDecl->GetPropertyValue(propertyNameString, values);
Expand Down Expand Up @@ -221,7 +223,7 @@ NS_IMETHODIMP ChangeStyleTransaction::DoTransaction() {
uint32_t length = cssDecl->Length();
if (!length) {
nsresult rv =
mElement->UnsetAttr(kNameSpaceID_None, nsGkAtoms::style, true);
styledElement->UnsetAttr(kNameSpaceID_None, nsGkAtoms::style, true);
if (NS_FAILED(rv)) {
NS_WARNING("Element::UnsetAttr(nsGkAtoms::style) failed");
return rv;
Expand All @@ -238,16 +240,18 @@ NS_IMETHODIMP ChangeStyleTransaction::DoTransaction() {

nsresult ChangeStyleTransaction::SetStyle(bool aAttributeWasSet,
nsAString& aValue) {
if (NS_WARN_IF(!mStyledElement)) {
return NS_ERROR_NOT_AVAILABLE;
}

if (aAttributeWasSet) {
OwningNonNull<nsStyledElement> styledElement = *mStyledElement;

// The style attribute was not empty, let's recreate the declaration
nsAutoCString propertyNameString;
mProperty->ToUTF8String(propertyNameString);

nsCOMPtr<nsStyledElement> inlineStyles = do_QueryInterface(mElement);
if (NS_WARN_IF(!inlineStyles)) {
return NS_ERROR_INVALID_ARG;
}
nsCOMPtr<nsICSSDeclaration> cssDecl = inlineStyles->Style();
nsCOMPtr<nsICSSDeclaration> cssDecl = styledElement->Style();

ErrorResult error;
if (aValue.IsEmpty()) {
Expand All @@ -268,7 +272,10 @@ nsresult ChangeStyleTransaction::SetStyle(bool aAttributeWasSet,
"nsICSSDeclaration::SetProperty() failed");
return error.StealNSResult();
}
nsresult rv = mElement->UnsetAttr(kNameSpaceID_None, nsGkAtoms::style, true);

OwningNonNull<nsStyledElement> styledElement = *mStyledElement;
nsresult rv =
styledElement->UnsetAttr(kNameSpaceID_None, nsGkAtoms::style, true);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
"Element::UnsetAttr(nsGkAtoms::style) failed");
return rv;
Expand Down
30 changes: 17 additions & 13 deletions editor/libeditor/ChangeStyleTransaction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "nsString.h" // nsString members

class nsAtom;
class nsStyledElement;

namespace mozilla {

Expand All @@ -25,29 +26,31 @@ class Element;
*/
class ChangeStyleTransaction final : public EditTransactionBase {
protected:
ChangeStyleTransaction(dom::Element& aElement, nsAtom& aProperty,
ChangeStyleTransaction(nsStyledElement& aStyledElement, nsAtom& aProperty,
const nsAString& aValue, bool aRemove);

public:
/**
* Creates a change style transaction. This never returns nullptr.
*
* @param aNode The node whose style attribute will be changed.
* @param aProperty The name of the property to change.
* @param aValue New value for aProperty.
* @param aStyledElement The node whose style attribute will be changed.
* @param aProperty The name of the property to change.
* @param aValue New value for aProperty.
*/
static already_AddRefed<ChangeStyleTransaction> Create(
dom::Element& aElement, nsAtom& aProperty, const nsAString& aValue);
nsStyledElement& aStyledElement, nsAtom& aProperty,
const nsAString& aValue);

/**
* Creates a change style transaction. This never returns nullptr.
*
* @param aNode The node whose style attribute will be changed.
* @param aProperty The name of the property to change.
* @param aValue The value to remove from aProperty.
* @param aStyledElement The node whose style attribute will be changed.
* @param aProperty The name of the property to change.
* @param aValue The value to remove from aProperty.
*/
static already_AddRefed<ChangeStyleTransaction> CreateToRemove(
dom::Element& aElement, nsAtom& aProperty, const nsAString& aValue);
nsStyledElement& aStyledElement, nsAtom& aProperty,
const nsAString& aValue);

NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ChangeStyleTransaction,
EditTransactionBase)
Expand Down Expand Up @@ -106,21 +109,22 @@ class ChangeStyleTransaction final : public EditTransactionBase {
nsresult SetStyle(bool aAttributeWasSet, nsAString& aValue);

// The element to operate upon.
nsCOMPtr<dom::Element> mElement;
RefPtr<nsStyledElement> mStyledElement;

// The CSS property to change.
RefPtr<nsAtom> mProperty;

// The value to set the property to (ignored if mRemoveProperty==true).
nsString mValue;

// true if the operation is to remove mProperty from mElement.
bool mRemoveProperty;

// The value to set the property to for undo.
nsString mUndoValue;
// The value to set the property to for redo.
nsString mRedoValue;

// true if the operation is to remove mProperty from mElement.
bool mRemoveProperty;

// True if the style attribute was present and not empty before DoTransaction.
bool mUndoAttributeWasSet;
// True if the style attribute is present and not empty after DoTransaction.
Expand Down
Loading

0 comments on commit 3f95e60

Please sign in to comment.