750745 - Allocate nsLineBoxes from their own presshell arena list

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: wip

Allocate nsLineBoxes from their own presshell arena list.

I think we can just reserve an ID for it and call AllocateFrame/FreeFrame
directly.  Perhaps we should rename those methods AllocateByID or something?

Comment 1

[Zack Weinberg (:zwol)]

Comment on attachment 619940 [details] [diff] [review]
wip

Does adding nsLineBox_id to the queryframe enumeration have any negative consequences?  One that comes to mind: does QueryFrame() now attempt to convert things to lineboxes?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Adding nsLineBox to the frame IDs is a bit confusing, although it would work.

I suggest renaming nsPresArena::AllocateByCode to nsPresArena::AllocateByFrameID, and add a separate nsPresArena::AllocateByObjectID that takes a separate enum defined in nsPresArena.h. Make that enum start at NON_FRAME_MARKER. change NON_FRAME_MARKER to be 0x20000000, and allocate a new NON_OBJECT_MARKER at 0x40000000.

Comment 3

[Mats Palmgren (inactive)]

(In reply to Zack Weinberg (:zwol) from comment #1)
> does QueryFrame() now attempt to convert things to lineboxes?

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsQueryFrame.h#258

Something like this wouldn't even compile:
  nsLineBox* line = do_QueryFrame(aFrame);

An explicit call on a frame compiles:
  void* ptr = aFrame->QueryFrame(nsQueryFrame::nsLineBox_id);

but would always return null AFAICT from the macros at the top.
It also stands out, and you would have to cast the result to be useful
so I'm pretty sure something like that would get caught in review.
(since we use do_QueryFrame everywhere else)

  nsLineBox* line; line->QueryFrame(nsQueryFrame::nsLineBox_id);

wouldn't compile since nsLineBox doesn't implement nsQueryFrame or use any
of the implementation macros.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Adding nsLineBox to the frame IDs is a bit confusing, although it would work.

Fair, but I don't think we have any direct use of the enum values in
any QueryFrame usage, we always use do_QueryFrame and macros for that.

> I suggest renaming nsPresArena::AllocateByCode to
> nsPresArena::AllocateByFrameID, and add a separate
> nsPresArena::AllocateByObjectID that takes a separate enum defined in
> nsPresArena.h. Make that enum start at NON_FRAME_MARKER. change
> NON_FRAME_MARKER to be 0x20000000, and allocate a new NON_OBJECT_MARKER at
> 0x40000000.

Sounds OK but we also need to add those methods on nsIPresShell (which is
already huge).  The benefit is it's clearly unrelated to nsQueryFrame from an
API point of view.  I'll take this path, adding comments about the dependency
of the magic numbers in both places.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This path can also be easily extended to the other callers of AllocateMisc. In fact, it's probably worth just converting them all to this scheme now.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 1, rename nsPresArena::AllocateByCode/FreeByCode to AllocateByFrameID/FreeByFrameID

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 2, introduce AllocateByObjectID/FreeByObjectID for allocating non-frame objects from per-type lists in the PresArena

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 3, allocate nsLineBoxes from their own presshell arena list

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=d69446dad595

Comment 8

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> This path can also be easily extended to the other callers of AllocateMisc.
> In fact, it's probably worth just converting them all to this scheme now.

OK, I've marked those methods @deprecated for now.

Comment 9

[Mats Palmgren (inactive)]

There was an unrelated error in the first Try push, here's a better one hopefully:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=272574ed8c60

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c44e1693ddd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e44cbf62c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e86db0a107

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c44e1693ddd9
https://hg.mozilla.org/mozilla-central/rev/a9e44cbf62c2
https://hg.mozilla.org/mozilla-central/rev/66e86db0a107