1334247 - Remove nsIAnonymousContentCreator::CreateFrameFor

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Bobby Holley (:bholley)]

This whole mechanism is pretty busted and almost unused. The possibility of custom frame creation (and thus arbitrary custom style context resolution) is problematic for Servo, since it needs a general principle to follow.

Luckily, it appears that the only consumers of this are used for native-anonymous text nodes, and Servo only operates on Elements. I have patches to clean up the code so that we can be sure this invariant holds.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Remove unused nsGfxButtonControlFrame::CreateFrameFor. v1

CreateFrameFor only gets called for frames that get constructed by the special
nsCSSFrameConstructor::CreateAnoymousFrames path

There's nothing special about buttons, so they just take the normal
AddFCItemsForAnonymousContent path. That path actually asserts against
a non-trivial CreateFrameFor implementation, but the assert currently
doesn't fire due to the mismatch between aPossiblyLeafFrame (which is the
nsGfxButtonControlFrame used to generate the anonymous content) and aFrame
(which is the inner block frame passed as the container to
AddFCItemsForAnonymousContent).

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add an explicit hook for the nsComboboxControlFrame case, and eliminate nsIAnonymousContentCreator::CreateFrameFor. v1

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a47b1b1733c40942a13fcddfeb8b72619835e76d

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8830883 [details] [diff] [review]
Remove unused nsGfxButtonControlFrame::CreateFrameFor. v1

Ah, I guess this has been dead code since bug 728516.

r=me

I wonder whether we can make that assert actually work right for this case...

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> I wonder whether we can make that assert actually work right for this case...

There's nothing to assert after part 2.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8830884 [details] [diff] [review]
Part 2 - Add an explicit hook for the nsComboboxControlFrame case, and eliminate nsIAnonymousContentCreator::CreateFrameFor. v1

So we just need the special codepath because of the nsComboboxDisplayFrame hanging out in there, right?  That's moderately annoying.

This entire nsCSSFrameConstructor::CreateAnonymousFrames codepath is only used for scrollframes and combobox.  I wonder whether we can just nix it, or separate it into the two separate cases or something... Followup on this might be nice.

>+  nsIComboboxControlFrame* comboboxFrame_ = do_QueryFrame(aParentFrame);
>+  auto comboboxFrame = static_cast<nsComboboxControlFrame*>(comboboxFrame_);

Why can't you just do:

  nsComboboxControlFrame* comboboxFrame = do_QueryFrame(aParentFrame);

by adding the relevant NS_DECL_QUERYFRAME_TARGET to that class?

r=me

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/707385706695
Remove unused nsGfxButtonControlFrame::CreateFrameFor. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e947f0224d95
Add an explicit hook for the nsComboboxControlFrame case, and eliminate nsIAnonymousContentCreator::CreateFrameFor. r=bz

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/707385706695
https://hg.mozilla.org/mozilla-central/rev/e947f0224d95