Remove nsIDocShellTreeItem::childOffset

RESOLVED FIXED in mozilla1.9alpha5

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla1.9alpha5
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
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).
(Assignee)

Comment 1

12 years ago
Created attachment 260652 [details] [diff] [review]
Patch rev. 1

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)

Updated

12 years ago
Assignee: nobody → mats.palmgren
(Assignee)

Updated

12 years ago
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 ?
(Assignee)

Comment 3

12 years ago
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+
(Assignee)

Comment 5

12 years ago
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

"indices"

>+    // 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+
(Assignee)

Comment 7

12 years ago
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.

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5

Updated

12 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.