Last Comment Bug 722406 - Cleanup nsHTMLEditor::SetCaretInTableCell
: Cleanup nsHTMLEditor::SetCaretInTableCell
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: :Ms2ger
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 11:33 PST by :Ms2ger
Modified: 2012-02-01 05:43 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Introduce nsISelection::CollapseNative (2.34 KB, patch)
2012-01-30 11:33 PST, :Ms2ger
bugs: review+
Details | Diff | Splinter Review
Part b: Cleanup nsHTMLEditor::SetCaretInTableCell (5.25 KB, patch)
2012-01-30 11:34 PST, :Ms2ger
ehsan: review+
Details | Diff | Splinter Review

Description :Ms2ger 2012-01-30 11:33:36 PST
Created attachment 592794 [details] [diff] [review]
Part a: Introduce nsISelection::CollapseNative

Editor often calls nsISelection::Collapse, and increasingly has nsINodes to pass, so it makes sense to allow passing an nsINode directly.
Comment 1 :Ms2ger 2012-01-30 11:34:27 PST
Created attachment 592795 [details] [diff] [review]
Part b: Cleanup nsHTMLEditor::SetCaretInTableCell
Comment 2 Olli Pettay [:smaug] 2012-01-30 11:36:54 PST
Comment on attachment 592794 [details] [diff] [review]
Part a: Introduce nsISelection::CollapseNative

update uuid
Comment 3 :Ehsan Akhgari 2012-01-30 12:54:09 PST
Comment on attachment 592795 [details] [diff] [review]
Part b: Cleanup nsHTMLEditor::SetCaretInTableCell

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +4038,5 @@
> +  if (!content || !content->IsHTML()) {
> +    return false;
> +  }
> +
> +  // REVIEW NOTE: this didn't handle th before

Noted.  Please remove this comment.  :-)

@@ +4043,5 @@
> +  if (!nsHTMLEditUtils::IsTableElement(content->AsElement())) {
> +    return false;
> +  }
> +
> +  // Find deepest child

This is actually a lie, isn't it?  ;-)  I'd remove this comment, since it's pretty evident what the loop is doing.

@@ +4049,5 @@
> +  while (node->HasChildren()) {
> +    node = node->GetFirstChild();
> +  }
> +
> +  // Set selection at beginning of deepest node

s/deepest/the found/

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