Closed Bug 376562 Opened 17 years ago Closed 17 years ago

Remove nsIDocShellTreeItem::childOffset


(Core :: DOM: Navigation, defect)

Not set





(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)



(1 file)

Followup from bug 162283

The number return by nsIDocShellTreeItem::GetChildOffset is not a valid
child index for its parent's child list (even though it's documented
as being that). This caused the crash in bug 162283.

This bug is for removing the remaining use in ESM::HandleAccessKey()
at least (which theoretically can cause a crash too, I think).
Attached patch Patch rev. 1Splinter Review
This patch replaces nsIDocShellTreeItem::childOffset with
nsIDocShell::SetChildOffset since this offset is now only used internally
in nsDocShell.cpp.

I think we could take the ESM part of this patch on branches if we want
since it doesn't require the interface changes.
Assignee: nobody → mats.palmgren
Attachment #260652 - Attachment description: wip → Patch rev. 1
Attachment #260652 - Flags: review?(Olli.Pettay)
How much work would it be to get rid of mChildOffset ?
Not too hard, but risky.  The docshell could simply iterate the parent's
child list to figure out its index and then use that to index the history
items.  The problem is that history items are never removed but docshells
are, so they become out-of-sync.
Bug 162283 has a WIP that removes the corresponding history item so
that they would be in sync, but Boris thought that would be a risky change.
See bug 162283 comment 71 - 73.
Comment on attachment 260652 [details] [diff] [review]
Patch rev. 1

>     nsresult res = AddChildLoader(childAsDocLoader);
>     NS_ENSURE_SUCCESS(res, res);
>+    NS_ASSERTION(mChildList.Count() > 0,
>+                 "child list must not be empty after a successful add");
>     // Set the child's index in the parent's children list 
>     // XXX What if the parent had different types of children?
>     // XXX in that case docshell hierarchy and SH hierarchy won't match.
>-    aChild->SetChildOffset(mChildList.Count() - 1);
>+    {
>+        nsCOMPtr<nsIDocShell> childDocShell = do_QueryInterface(aChild);
>+        if (childDocShell)
>+            childDocShell->SetChildOffset(mChildList.Count() - 1);
>+    }

Why adding the block?

>@@ -424,14 +424,19 @@ interface nsIDocShell : nsISupports
>+  /**
>+   * Set the offset of this child in its container.
>+   */
>+  [noscript] void setChildOffset(in unsigned long offset);

This is ugly, but I don't know where else to put this.

r=me if you explain or fix the block-thingie.
Attachment #260652 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 260652 [details] [diff] [review]
Patch rev. 1

> Why adding the block?

We don't need 'childDocShell' beyond that point.
Attachment #260652 - Flags: superreview?(bzbarsky)
Comment on attachment 260652 [details] [diff] [review]
Patch rev. 1

>Index: docshell/base/nsDocShell.h
>+    // with lower indicies


>+    // as an index into the parent's child list (see bug 162283).  It MUST not
>+    // for that purpose.

"not be used for"?

>Index: content/events/src/nsEventStateManager.cpp
> // Note: for the in parameter aChildOffset,
> // -1 stands for not bubbling from the child docShell
> // 0 -- childCount - 1 stands for the child docShell's offset
> // which bubbles up the access key handling

That comment no longer has bearing on reality.  Re-document in terms of aBubbledFrom?  Probably just need to explain what the arg means.

sr=bzbarsky with those changes.
Attachment #260652 - Flags: superreview?(bzbarsky) → superreview+
Fixed spelling.  Added a full-fledged doc comment on ESM::HandleAccessKey()
describing all params and how the method works.

Checked in to trunk at 2007-05-17 20:49 PDT.

Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.