Issues in Layout that do not fit into any other Layout component or which span multiple Layout components.

Bugs related to the top level presentation objects (pres shell, pres context, and document viewer), the frame constructor, and the base frame classes, as well as general issues with alignment and sizing, all belong here.

(1 attachment)

I've investigated this a bit and I think this is doable,
at least for most common use case, which is:
SomeConcreteFrameClass* f = do_QueryFrame(aFrame); // nsIFrame* aFrame
Those can be done as a static_cast if aFrame->mClass is that of
SomeConcreteFrameClass.
Other cases can be optimized through a table-based approach...

I did some coarse measurements of the kind of do_QueryFrame calls
we do on a few web pages. The number of calls that are eligible
for the above optimization is in the 60-100% range depending
on the page, e.g. https://slashdot.org is ~60% and
https://wikipedia.org/wiki/The_Beatles is ~100%.
Internal XUL pages that we load during startup are typically less
than that, 20-40% or so.
Out of the eligible calls, roughly 55% are successfully optimized.
The total number of eligible calls are quite high; starting a browser
with a single tab loading https://wikipedia.org/wiki/The_Beatles
generates ~1 million calls.
FTR, non-eligible calls are of the form:
SomeQueryFrameInterface* f = do_QueryFrame(aFrame); // nsIFrame* aFrame
SomeConcreteFrameClass* f = do_QueryFrame(aThing); // SomeQueryFrameInterface* aThing
SomeQueryFrameInterface* f = do_QueryFrame(aThing); // SomeOtherQueryFrameInterface* aThing
(I don't think we have many of the last kind though)

Comment on attachment 8906084[details][diff][review]
fix
>Bug 1364815 - Optimize away many (virtual) calls to QueryFrame. r=dholbert
>
>do_QueryFrame from one frame type to another frame type
>can compare mClass first, and if successful just downcast
>the pointer to the target frame type. If unsuccesful,
Typo: s/unsuccesful/unsuccessful/ (needs a double "s" at the end)
>diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
>--- a/layout/generic/nsIFrame.h
>+++ b/layout/generic/nsIFrame.h
>+// Use QueryFrame fast-path for nsIFrame.
>+inline do_QueryFrameHelper<nsIFrame>
>+do_QueryFrame(AutoWeakFrame& s)
>+{
>+ return do_QueryFrameHelper<nsIFrame>(s.GetFrame());
>+}
The comment here is ambiguous, RE what "for nsIFrame" means. Initially, I read it as meaning "Use QueryFrame fast-path, if we have a nsIFrame", and I had a hard time mapping that onto the code.
Please reword to avoid that ambiguity -- e.g. maybe:
// Use nsIFrame's fast-path to avoid QueryFrame:
(I think that's what the comment means to say, anyway.)
>+// Use QueryFrame fast-path for nsIFrame.
>+inline do_QueryFrameHelper<nsIFrame>
>+do_QueryFrame(WeakFrame& s)
Same here.
>diff --git a/layout/generic/nsQueryFrame.h b/layout/generic/nsQueryFrame.h
>--- a/layout/generic/nsQueryFrame.h
>+++ b/layout/generic/nsQueryFrame.h
>+ // Specialization for nsIFrame to nsIFrame subclasses -- if the source
>+ // instance's mClass matches kFrameIID of the destination type then
>+ // downcasting is safe.
>+ template<class Src, class Dst>
>+ struct FastQueryFrame<Src, Dst,
>+ typename mozilla::EnableIf<mozilla::IsBaseOf<nsIFrame, Src>::value>::type,
>+ typename mozilla::EnableIf<mozilla::IsBaseOf<nsIFrame, Dst>::value>::Type>
>+ {
The comment here, "for nsIFrame to nsIFrame subclasses", sounds like it's saying the source type has to be nsIFrame itself (and the destination can be a nsIFrame subclass). But the code looks like it allows both Src and Dst to be subclasses.
So perhaps the comment should say "Specialization for nsIFrame subclasses to other nsIFrame subclasses", perhaps?
>+ static Dst* QueryFrame(Src* aFrame) {
>+ return nsQueryFrame::FrameIID(aFrame->mClass) == Dst::kFrameIID ?
>+ reinterpret_cast<Dst*>(aFrame) : nullptr;
It's not obvious to me why the conversions in the "==" check are valid. It looks like:
* aFrame->mClass has type nsQueryFrame::ClassID
* ...which we cast to type FrameIID...
* ...and that seems like it might be a bogus cast, since those are two **completely different** enum types. They happen to be generated from the same header, but they use different (overlapping) pieces of that header -- one uses the ABSTRACT_ entries and the other does not. So superficially, it's not clear that we can depend on the numbers matching up.
Ah, OK -- I guess this is fine because nsFrameIdList.h happens to list *all* of its ABSTRACT_FRAME_ID() entries at the end, after all of its FRAME_ID() entries. So we are 100% depending on that here, in order for this enum cast to be valid. Could you be sure that dependency is documented somewhere? (Either in nsFrameIdList.h itself (which doesn't have any documentation to suggest that the ordering is significant AFAICT), or in nsQueryFrame alongside these enum types, or here alongside your type-conversion. Or maybe in several of those places.)

> So perhaps the comment should say "Specialization for nsIFrame subclasses to other nsIFrame subclasses"
I chose "Specialization for any nsIFrame type to any nsIFrame type".
To me, "nsIFrame subclasses" sounds like it implies the type nsIFrame
is excluded, which it isn't.
> Could you be sure that dependency is documented somewhere?
I added in nsFrameIdList.h (before the first ABSTRACT_FRAME_ID):
// The following ABSTRACT_FRAME_IDs needs to come after the above
// FRAME_IDs, because we have two separate enums, one that includes
// only FRAME_IDs and another which includes both and we depend on
// FRAME_IDs to have the same number in both.
// See ClassID (the former) and FrameIID in nsQueryFrame.h.
Fwiw, we already have an assertion for this:
http://searchfox.org/mozilla-central/rev/70cfd6ceecacbe779456654b596bbee4f2b8890b/layout/generic/nsFrame.cpp#610