The default bug view has changed. See this FAQ.

Allocate nsLineBoxes from their own presshell arena list

RESOLVED FIXED in mozilla15

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 3

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

Updated

5 years ago
Attachment #619940 - Attachment is obsolete: true
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.
(Assignee)

Comment 5

5 years ago
Created attachment 620346 [details] [diff] [review]
part 1, rename nsPresArena::AllocateByCode/FreeByCode to AllocateByFrameID/FreeByFrameID
Attachment #620346 - Flags: review?(roc)
(Assignee)

Comment 6

5 years ago
Created attachment 620347 [details] [diff] [review]
part 2, introduce AllocateByObjectID/FreeByObjectID for allocating non-frame objects from per-type lists in the PresArena
Attachment #620347 - Flags: review?(roc)
(Assignee)

Comment 7

5 years ago
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
Attachment #620348 - Flags: review?(roc)
(Assignee)

Comment 8

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

Comment 9

5 years ago
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
Attachment #620346 - Flags: review?(roc) → review+
Attachment #620347 - Flags: review?(roc) → review+
Attachment #620348 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
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
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/c44e1693ddd9
https://hg.mozilla.org/mozilla-central/rev/a9e44cbf62c2
https://hg.mozilla.org/mozilla-central/rev/66e86db0a107
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 1038488
You need to log in before you can comment on or make changes to this bug.