Remove nsIAnonymousContentCreator::CreateFrameFor

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8830883 [details] [diff] [review]
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).
Attachment #8830883 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

2 years ago
Created attachment 8830884 [details] [diff] [review]
Part 2 - Add an explicit hook for the nsComboboxControlFrame case, and eliminate nsIAnonymousContentCreator::CreateFrameFor. v1
Attachment #8830884 - Flags: review?(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...
Attachment #8830883 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

2 years ago
(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 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
Attachment #8830884 - Flags: review?(bzbarsky) → review+

Comment 7

2 years ago
Pushed by bholley@mozilla.com:
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
(Assignee)

Updated

2 years ago
Blocks: 1334358

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/707385706695
https://hg.mozilla.org/mozilla-central/rev/e947f0224d95
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.