Closed Bug 1425467 Opened 2 years ago Closed 2 years ago

mInlineEditedCell should be Element, not nsIDOMElement

Categories

(Core :: DOM: Editor, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(1 file)

To reduce QI for table editor.
Priority: -- → P3
Comment on attachment 8937062 [details]
Bug 1425467 - mInlineEditedCell should be Element.

https://reviewboard.mozilla.org/r/207774/#review213788

::: editor/libeditor/HTMLInlineTableEditor.cpp:148
(Diff revision 1)
> -    nsCOMPtr<nsIDOMNode> tableNode = GetEnclosingTable(mInlineEditedCell);
> +    Element* tableElement = GetEnclosingTable(mInlineEditedCell);
> -    nsCOMPtr<nsIDOMElement> tableElement = do_QueryInterface(tableNode);
>      int32_t rowCount, colCount;
>      rv = GetTableSize(tableElement, &rowCount, &colCount);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      bool hideUI = false;
> -    bool hideResizersWithInlineTableUI = (GetAsDOMNode(mResizedObject) == tableElement);
> +    bool hideResizersWithInlineTableUI = (mResizedObject == tableElement);
>  
>      if (anonclass.EqualsLiteral("mozTableAddColumnBefore"))
>        InsertTableColumn(1, false);
>      else if (anonclass.EqualsLiteral("mozTableAddColumnAfter"))
>        InsertTableColumn(1, true);
>      else if (anonclass.EqualsLiteral("mozTableAddRowBefore"))
>        InsertTableRow(1, false);
>      else if (anonclass.EqualsLiteral("mozTableAddRowAfter"))
>        InsertTableRow(1, true);
>      else if (anonclass.EqualsLiteral("mozTableRemoveColumn")) {
>        DeleteTableColumn(1);
>  #ifndef DISABLE_TABLE_DELETION
>        hideUI = (colCount == 1);
>  #endif
>      }
>      else if (anonclass.EqualsLiteral("mozTableRemoveRow")) {
>        DeleteTableRow(1);
>  #ifndef DISABLE_TABLE_DELETION
>        hideUI = (rowCount == 1);
>  #endif
>      }

Hmm, these if/elseif blocks may cause destroying the tableElement (via mutation observers). So, please set it to nullptr with comment before the if block or use RefPtr.

I worry about touching outdated raw pointer at maintaining this method.

::: editor/libeditor/HTMLInlineTableEditor.cpp:237
(Diff revision 1)
> -  nsCOMPtr<nsIDOMNode> tableNode = GetEnclosingTable(mInlineEditedCell);
> +  Element* tableElement = GetEnclosingTable(mInlineEditedCell);
> -  nsCOMPtr<nsIDOMElement> tableElement = do_QueryInterface(tableNode);
>    int32_t rowCount, colCount;
>    rv = GetTableSize(tableElement, &rowCount, &colCount);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    SetAnonymousElementPosition(xHoriz-10, yCell-7,  mAddColumnBeforeButton);

Same here. Please set it to nullptr with comment or use RefPtr.
Attachment #8937062 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/bd452595f41a
mInlineEditedCell should be Element. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/bd452595f41a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.