Closed Bug 616746 Opened 9 years ago Closed 9 years ago

remove HTML option/optgroup stuff from nsTreeContentView

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: tnikkel, Assigned: matjk7)

References

Details

Attachments

(1 file, 9 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #510497 - Flags: review?(tnikkel)
Comment on attachment 510497 [details] [diff] [review]
patch

Thanks for the patch!

@@ -222,27 +220,16 @@ nsTreeContentView::SetSelection(nsITreeS
   if (!mSelection || !mUpdateSelection)
     return NS_OK;
 
   mUpdateSelection = PR_FALSE;
 
-  mSelection->SetSelectEventsSuppressed(PR_TRUE);
-  for (PRUint32 i = 0; i < mRows.Length(); ++i) {
-    Row* row = mRows[i];
-    nsCOMPtr<nsIDOMHTMLOptionElement> optEl = do_QueryInterface(row->mContent);
-    if (optEl) {
-      PRBool isSelected;
-      optEl->GetSelected(&isSelected);
-      if (isSelected)
-        mSelection->ToggleSelect(i);
-    }
-  }
   mSelection->SetSelectEventsSuppressed(PR_FALSE);

mUpdateSelection is only ever set to true in SerializeOption, which your patch removes, so there is even more deadcode here. Could you remove mUpdateSelection too?

@@ -537,28 +524,17 @@ nsTreeContentView::GetCellText(PRInt32 a
 
   // Check for a "label" attribute - this is valid on an <treeitem>
   // or an <option>, with a single implied column.
   if (row->mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, _retval)
       && !_retval.IsEmpty())
     return NS_OK;

There is an option in the comment here, let's get rid of it. With your patch applied could you search for option or optgroup or html in nsTreeContentView.cpp/h? Let's get rid of as much code as we can. :)

@@ -537,28 +524,17 @@ nsTreeContentView::GetCellText(PRInt32 a
+  if (rowTag == nsGkAtoms::treeitem &&
            row->mContent->IsXUL()) {
     nsIContent* realRow =
       nsTreeUtils::GetImmediateChild(row->mContent, nsGkAtoms::treerow);
     if (realRow) {
       nsIContent* cell = GetCell(realRow, aCol);
       if (cell)
         cell->GetAttr(kNameSpaceID_None, nsGkAtoms::label, _retval);
     }

Let's factor out the row->mContent->IsXUL() check and just return early if it fails. (After your patch there should only be XUL in our rows, but I'm a little paranoid in this file.)

@@ -824,23 +790,16 @@ nsTreeContentView::ContentStatesChanged(
                                         nsIContent* aContent1,
                                         nsIContent* aContent2,
                                         nsEventStates aStateMask)
 {
   if (!aContent1 || !mSelection ||
       !aContent1->IsHTML() ||
       !aStateMask.HasState(NS_EVENT_STATE_CHECKED))
     return;
-
-  if (aContent1->Tag() == nsGkAtoms::option) {
-    // update the selected state for this node
-    PRInt32 index = FindContent(aContent1);
-    if (index >= 0)
-      mSelection->ToggleSelect(index);
-  }
 }

Looks like this function is a no-op now. Let's not re-implement it at all in nsTreeContentView, the stub document observer should have a default one that does nothing.

void
 nsTreeContentView::ContentRemoved(nsIDocument *aDocument,
                                   nsIContent* aContainer,
                                   nsIContent* aChild,
                                   PRInt32 aIndexInContainer,
                                   nsIContent* aPreviousSibling)
 {
   NS_ASSERTION(aChild, "null ptr");
 
   // Make sure this notification concerns us.
   // First check the tag to see if it's one that we care about.
   nsIAtom *tag = aChild->Tag();
 
-  if (aChild->IsHTML()) {
-    if (tag != nsGkAtoms::option &&
-        tag != nsGkAtoms::optgroup)
-      return;
-  }
-  else if (aChild->IsXUL()) {
+  if (aChild->IsXUL()) {
     if (tag != nsGkAtoms::treeitem &&
         tag != nsGkAtoms::treeseparator &&
         tag != nsGkAtoms::treechildren &&
         tag != nsGkAtoms::treerow &&
         tag != nsGkAtoms::treecell)
       return;
     // We don't consider XUL nodes under non-XUL nodes.
     if (!aContainer->IsXUL())

I think it would be nicer just have a early return if (!aChild->IsXUL() || !aContainer->IsXUL()) and then do the tag check after that. Early returns are easier to reason about then nested ifs. Similarly in AttributeChanged and ContentInserted.

@@ -1214,22 +1147,16 @@ nsTreeContentView::Serialize(nsIContent*
     if (content->IsXUL() && containerIsXUL) {

Let's just return early when containerIsXUL is true and skip this check on every iteration of the loop. Same thing for GetIndexInSubtree.

 nsTreeContentView::EnsureSubtree(PRInt32 aIndex)
-  if (row->mContent->Tag() == nsGkAtoms::optgroup)
-    child = row->mContent;
-  else {
-    child =
-      nsTreeUtils::GetImmediateChild(row->mContent, nsGkAtoms::treechildren);
+  child = nsTreeUtils::GetImmediateChild(row->mContent, nsGkAtoms::treechildren);
     if (! child) {
       return 0;
     }
-  }

The indentation is off here.

Looks good, there is just more code to remove (which is a good thing)!
Fixed your review comments locally. Just two questions:

-How do I get Mercurial to spit out a patch with previously committed changes (the patch posted here) as well as changes made after that? If I do hg diff now I just get a patch which applies on top of this one.

> Let's just return early when containerIsXUL is true and skip this check on
> every iteration of the loop. Same thing for GetIndexInSubtree.

-You meant false here, right?
(In reply to comment #3)
> Fixed your review comments locally. Just two questions:
> 
> -How do I get Mercurial to spit out a patch with previously committed changes
> (the patch posted here) as well as changes made after that? If I do hg diff now
> I just get a patch which applies on top of this one.

Ok so you are not using mercurial queues and you have the patch currently posted in this bug as a changeset in your repo? First of all the hg diff output is useful to me as a reviewer, it lets me see what changed since the last patch (we call it an interdiff). So save that output and post it in this bug (along with the new patch).

To integrate the two sets of changes I would recommend creating a new changeset for these further changes you have made: hg commit. So you now have two changesets in your repo. Use hg log to find out the revision ids. Then do hg diff -r <revision before your changes> -r <your last revision>. That should produce a diff with all your changes in one. Save that. Then do hg strip <rev> for both your revisions, most recent one first. This removes those revisions from your repo. Then do hg import <saved combined diff>.

That's the way to do it without mercurial queues. If you want to learn mercurial queues then it is easier.
 
> > Let's just return early when containerIsXUL is true and skip this check on
> > every iteration of the loop. Same thing for GetIndexInSubtree.
> 
> -You meant false here, right?

Yes.
(In reply to comment #4)
> First of all the hg diff output is useful to me as a reviewer,
> it lets me see what changed since the last patch (we call it an interdiff).
Although Bugzilla can sometimes do this for you.

> That's the way to do it without mercurial queues.
Technically hg strip is part of the mq extension ;-)

Had you not done anything else to the tree, you could have rolled back your commit and continued working. Or you can pretend that it's CVS and never commit ;-)
Attached patch interdiff (obsolete) — Splinter Review
Attachment #511442 - Flags: review?(tnikkel)
Attached patch combined patches (obsolete) — Splinter Review
Comment on attachment 511443 [details] [diff] [review]
combined patches

Thanks for the interdiff, it was quite useful.

 NS_IMETHODIMP
 nsTreeContentView::SetSelection(nsITreeSelection* aSelection)
 {
   NS_ENSURE_TRUE(!aSelection || CanTrustTreeSelection(aSelection),
                  NS_ERROR_DOM_SECURITY_ERR);
 
   mSelection = aSelection;
-  if (!mSelection || !mUpdateSelection)
+  if (!mSelection)
     return NS_OK;
 
-  mUpdateSelection = PR_FALSE;
-
-  mSelection->SetSelectEventsSuppressed(PR_TRUE);
-  for (PRUint32 i = 0; i < mRows.Length(); ++i) {
-    Row* row = mRows[i];
-    nsCOMPtr<nsIDOMHTMLOptionElement> optEl = do_QueryInterface(row->mContent);
-    if (optEl) {
-      PRBool isSelected;
-      optEl->GetSelected(&isSelected);
-      if (isSelected)
-        mSelection->ToggleSelect(i);
-    }
-  }
   mSelection->SetSelectEventsSuppressed(PR_FALSE);
 
   return NS_OK;
 }

mUpdateSelection can only be set to true in SerializeOption. So after you remove that function mUpdateSelection will always be false. Hence (!mSelection || !mUpdateSelection) will always be true, so we return and any code after that is dead code, we should remove all of it.

@@ -531,35 +515,23 @@ nsTreeContentView::GetCellText(PRInt32 a
-  else if (rowTag == nsGkAtoms::treeitem &&
-           row->mContent->IsXUL()) {
+  if (rowTag == nsGkAtoms::treeitem) 

I think we should keep the IsXUL check.

@@ -852,40 +795,34 @@ nsTreeContentView::AttributeChanged(nsID
+  // We don't consider XUL nodes under non-XUL nodes.
+  if (!aElement->IsXUL() || !!aElement->GetParent()->IsXUL())

Just say that we don't consider non-XUL nodes, we don't need the extra complication thanks to your patch. Same thing in ContentInserted and ContentRemoved.

@@ -852,40 +795,34 @@ nsTreeContentView::AttributeChanged(nsID
+  if (tag != nsGkAtoms::treecol &&
+      tag != nsGkAtoms::treeitem &&
+      tag != nsGkAtoms::treeseparator &&
+      tag != nsGkAtoms::treerow &&
+      tag != nsGkAtoms::treecell)
+    return;

Since your changing this anyway I'm going to pull a roc and ask for { } around the body of the if because of the multiline if condition. Same thing in ContentInserted and ContentRemoved.

 nsTreeContentView::Serialize(nsIContent* aContent, PRInt32 aParentIndex,
                              PRInt32* aIndex, nsTArray<Row*>& aRows)
 {
   // Don't allow XUL nodes under non-XUL nodes.
   PRBool containerIsXUL = aContent->IsXUL();
 
+  if (!containerIsXUL)
+    return;
+

The variable containerIsXUL is now useless, just check aContent->IsXUL() directly in the if. Same for GetIndexInSubtree.

 nsTreeContentView::Serialize(nsIContent* aContent, PRInt32 aParentIndex, 
-    if (content->IsXUL() && containerIsXUL) {
-      if (tag == nsGkAtoms::treeitem)
-        SerializeItem(content, aParentIndex, aIndex, aRows);
-      else if (tag == nsGkAtoms::treeseparator)
-        SerializeSeparator(content, aParentIndex, aIndex, aRows);
-    }
-    else if (content->IsHTML()) {
-      if (tag == nsGkAtoms::option)
-        SerializeOption(content, aParentIndex, aIndex, aRows);
-      else if (tag == nsGkAtoms::optgroup)
-        SerializeOptGroup(content, aParentIndex, aIndex, aRows);
-    }
+    if (tag == nsGkAtoms::treeitem)
+      SerializeItem(content, aParentIndex, aIndex, aRows);
+    else if (tag == nsGkAtoms::treeseparator)
+      SerializeSeparator(content, aParentIndex, aIndex, aRows);
     *aIndex += aRows.Length() - count;
   }

We still want the content->IsXUL() check, right? Same for GetIndexInSubtree.

Thanks!
Attached patch interdiff 2 (obsolete) — Splinter Review
Attachment #511567 - Flags: review?(tnikkel)
Attached patch combined patches (obsolete) — Splinter Review
Attachment #511443 - Attachment is obsolete: true
Comment on attachment 511567 [details] [diff] [review]
interdiff 2

@@ -216,20 +216,16 @@ NS_IMETHODIMP
 nsTreeContentView::SetSelection(nsITreeSelection* aSelection)
 {
   NS_ENSURE_TRUE(!aSelection || CanTrustTreeSelection(aSelection),
                  NS_ERROR_DOM_SECURITY_ERR);
 
   mSelection = aSelection;
   if (!mSelection)
     return NS_OK;
-
-  mSelection->SetSelectEventsSuppressed(PR_FALSE);
-
-  return NS_OK;
 }

We don't need the if (!mSelection) anymore.

 // Recursively serialize content, starting with aContent.
 void
 nsTreeContentView::Serialize(nsIContent* aContent, PRInt32 aParentIndex,
                              PRInt32* aIndex, nsTArray<Row*>& aRows)
 {
-  // Don't allow XUL nodes under non-XUL nodes.
+  // Don't allow non-XUL nodes.
   PRBool containerIsXUL = aContent->IsXUL();
 
-  if (!containerIsXUL)
+  if (!aContent->IsXUL())
     return;

The variable containerIsXUL is unused now, right?
 
   ChildIterator iter, last;
   for (ChildIterator::Init(aContent, &iter, &last); iter != last; ++iter) {
     nsIContent* content = *iter;
     nsIAtom *tag = content->Tag();
     PRInt32 count = aRows.Length();
 
-    if (tag == nsGkAtoms::treeitem)
+    if (tag == nsGkAtoms::treeitem && content->IsXUL())
       SerializeItem(content, aParentIndex, aIndex, aRows);
-    else if (tag == nsGkAtoms::treeseparator)
+    else if (tag == nsGkAtoms::treeseparator && content->IsXUL())
       SerializeSeparator(content, aParentIndex, aIndex, aRows);
     *aIndex += aRows.Length() - count;
   }
 }

Let's put the if (tag ...) nested inside an if (content->IsXUL()), I think the logic is easier to read that way.

void
 nsTreeContentView::GetIndexInSubtree(nsIContent* aContainer,
                                      nsIContent* aContent, PRInt32* aIndex)
 {
   PRBool containerIsXUL = aContainer->IsXUL();
   PRUint32 childCount = aContainer->GetChildCount();
   
-  if (!containerIsXUL)
+  if (!aContent->IsXUL())
     return;

I think containerIsXUL is unused now.

 nsTreeContentView::GetIndexInSubtree(nsIContent* aContainer,
 -    if (tag == nsGkAtoms::treeitem) {
+    if (tag == nsGkAtoms::treeitem && content->IsXUL()) {
       if (! content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::hidden,
                                  nsGkAtoms::_true, eCaseMatters)) {
         (*aIndex)++;
         if (content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::container,
                                  nsGkAtoms::_true, eCaseMatters) &&
             content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::open,
                                  nsGkAtoms::_true, eCaseMatters)) {
           nsIContent* child =
             nsTreeUtils::GetImmediateChild(content, nsGkAtoms::treechildren);
           if (child)
             GetIndexInSubtree(child, aContent, aIndex);
         }
       }
     }
-    else if (tag == nsGkAtoms::treeseparator) {
+    else if (tag == nsGkAtoms::treeseparator && content->IsXUL()) {
       if (! content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::hidden,
                                  nsGkAtoms::_true, eCaseMatters))
         (*aIndex)++;
     }
   }
 }

I think we should only have the content->IsXUL() check once. So either wrap this code in a larger if (content->IsXUL()) or do "if (!content->IsXUL()) continue;" before it.

Looking good otherwise.
> We don't need the if (!mSelection) anymore.

And do need an unconditional return at the end of the function.
Attached patch interdiff 3 (obsolete) — Splinter Review
Attachment #511619 - Flags: review?(tnikkel)
Attached patch latest patch (obsolete) — Splinter Review
Attachment #511568 - Attachment is obsolete: true
Comment on attachment 511621 [details] [diff] [review]
latest patch

@@ -852,40 +790,35 @@ nsTreeContentView::AttributeChanged(nsID
+  // We don't consider non-XUL nodes.
+  if (!aElement->IsXUL() || !!aElement->GetParent()->IsXUL())
+    return;

Looks like you have an extra ! here. And also you need to null check aElement->GetParent().
Attached patch latest patch (obsolete) — Splinter Review
Not going to ask for yet another review since I ended up stealing your code :P

I also noticed the try-server run busted on Windows, which is odd because I can build on Windows just fine.
Attachment #511621 - Attachment is obsolete: true
I had more things in my try run then just this, your patch wasn't to blame for the Windows bustage.
Sorry for the delay. This basically looks good, but I want to give it a final once over. But I'm busy with FF4 work right now.
Comment on attachment 511804 [details] [diff] [review]
latest patch

 nsTreeContentView::GetIndexInSubtree(nsIContent* aContainer,
                                      nsIContent* aContent, PRInt32* aIndex)
 {
-  PRBool containerIsXUL = aContainer->IsXUL();
   PRUint32 childCount = aContainer->GetChildCount();
+  
+  if (!aContent->IsXUL())
+    return;
+
   for (PRUint32 i = 0; i < childCount; i++) {
     nsIContent *content = aContainer->GetChildAt(i);
 
     if (content == aContent)
       break;
 
     nsIAtom *tag = content->Tag();
 
-    if (content->IsXUL() && containerIsXUL) {
+    if (content->IsXUL()) {
       if (tag == nsGkAtoms::treeitem) {
         if (! content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::hidden,
                                    nsGkAtoms::_true, eCaseMatters)) {
           (*aIndex)++;
           if (content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::container,
                                    nsGkAtoms::_true, eCaseMatters) &&
               content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::open,
                                    nsGkAtoms::_true, eCaseMatters)) {

The

+  if (!aContent->IsXUL())
+    return;

should have aContainer instead of aContent, right?

Otherwise this is good!
Blocks: 645203
Attached patch final patch (obsolete) — Splinter Review
Attachment #510497 - Attachment is obsolete: true
Attachment #511442 - Attachment is obsolete: true
Attachment #511567 - Attachment is obsolete: true
Attachment #511619 - Attachment is obsolete: true
Attachment #511804 - Attachment is obsolete: true
Attachment #522048 - Flags: review?(tnikkel)
Attachment #510497 - Flags: review?(tnikkel)
Attachment #511442 - Flags: review?(tnikkel)
Attachment #511567 - Flags: review?(tnikkel)
Attachment #511619 - Flags: review?(tnikkel)
Comment on attachment 522048 [details] [diff] [review]
final patch

Great! Thanks for this! Sorry for taking so long on the review. Let me know if you need help with anything.
Attachment #522048 - Flags: review?(tnikkel) → review+
Whiteboard: [needs landing]
Assignee: nobody → matjk7
Keywords: checkin-needed
The patch doesn't apply cleanly on tip anymore.
Keywords: checkin-needed
Matheus, do you think you could update the patch? Looks like some large refactorings changed this underneath you, sorry!
Attachment #522048 - Attachment is obsolete: true
Thanks.
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/2e5235cbade6
Keywords: checkin-needed
Whiteboard: [needs landing] → fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/2e5235cbade6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.