Last Comment Bug 750745 - Allocate nsLineBoxes from their own presshell arena list
: Allocate nsLineBoxes from their own presshell arena list
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
Depends on:
Blocks: 1038488
  Show dependency treegraph
 
Reported: 2012-05-01 08:42 PDT by Mats Palmgren (:mats)
Modified: 2014-07-14 18:25 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (3.39 KB, patch)
2012-05-01 08:42 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 1, rename nsPresArena::AllocateByCode/FreeByCode to AllocateByFrameID/FreeByFrameID (6.99 KB, patch)
2012-05-02 09:08 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 2, introduce AllocateByObjectID/FreeByObjectID for allocating non-frame objects from per-type lists in the PresArena (9.48 KB, patch)
2012-05-02 09:09 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 3, allocate nsLineBoxes from their own presshell arena list (4.25 KB, patch)
2012-05-02 09:10 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2012-05-01 08:42:55 PDT
Created attachment 619940 [details] [diff] [review]
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) 2012-05-01 09:22:57 PDT
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) 2012-05-01 12:34:41 PDT
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 (:mats) 2012-05-01 14:43:10 PDT
(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) 2012-05-01 15:23:56 PDT
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 (:mats) 2012-05-02 09:08:41 PDT
Created attachment 620346 [details] [diff] [review]
part 1, rename nsPresArena::AllocateByCode/FreeByCode to AllocateByFrameID/FreeByFrameID
Comment 6 Mats Palmgren (:mats) 2012-05-02 09:09:36 PDT
Created attachment 620347 [details] [diff] [review]
part 2, introduce AllocateByObjectID/FreeByObjectID for allocating non-frame objects from per-type lists in the PresArena
Comment 7 Mats Palmgren (:mats) 2012-05-02 09:10:52 PDT
Created attachment 620348 [details] [diff] [review]
part 3, allocate nsLineBoxes from their own presshell arena list

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=d69446dad595
Comment 8 Mats Palmgren (:mats) 2012-05-02 09:13:12 PDT
(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 (:mats) 2012-05-02 09:47:12 PDT
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

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