Unify nsLayoutUtils::GetAsBlock() and nsIFrame::IsBlockFrameOrSubclass()
Categories
(Core :: Layout, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: TYLin, Assigned: TYLin)
References
Details
Attachments
(3 files)
We now have nsLayoutUtils::GetAsBlock() and nsIFrame::IsBlockFrameOrSubclass() where both methods do basically the same thing.
Maybe we can use nsIFrame::GetAsBlock() as mats suggested in bug 1525628 comment 15. If this sounds good, I can provide a patch.
|
||
Comment 1•5 years ago
|
||
Or, we could use IsBlockFrameOrSubclass() when you don't need
the actual nsBlockFrame pointer, and do_QueryFrame when you do.
if (nsBlockFrame* block = do_QueryFrame(aFrame)) { ...
is good enough to not warrant GetAsBlock() IMO. But, it is kind
of awkward to use when you have multiple terms:
if (IsSomethingElse() &&
static_cast<nsBlockFrame*>(do_QueryFrame(aFrame))) { ...
In that case, GetAsBlock() or IsBlockFrameOrSubclass() is easier to read.
do_QueryFrame(aFrame) has the advantage that it handles null, whereas
aFrame->GetAsBlock() would crash. We could make it static I guess,
but nsIFrame::GetAsBlock(aFrame) is also kind of ugly.
I don't have strong opinions either way, but I do want to replace
nsLayoutUtils::GetAsBlock(). I think I'm slightly in favor of
IsBlockFrameOrSubclass+do_QueryFrame and replacing existing
nsLayoutUtils::GetAsBlock() with either of those.
|
Assignee | |
Comment 2•5 years ago
|
||
We could make it static I guess, but nsIFrame::GetAsBlock(aFrame) is also kind of ugly.
Agree, if we want it static, perhaps nsBlockFrame::From(nsIFrame*) is better and feels more Rusty ;)
|
||
Comment 3•5 years ago
|
||
nsBlockFrame::FromFrame / FromFrameOrNull would be the closest to the current Element::FromNode / Element::FromNodeOrNull and co. if we want to go that route.
Anyhow not worth bikeshedding too much about it, probably.
|
|
||
Comment 4•5 years ago
|
||
The slight disadvantage of those compared to do_QueryFrame is
that you'd have to #include nsBlockFrame.h to use them.
I still don't see why introducing a static method as
an alias for do_QueryFrame is warranted though.
|
|
||
Comment 5•5 years ago
|
||
Sure, to be clear I'm 100% fine with do_QueryFrame. I just pointed out that TYLin's suggestion is closer to what DOM does.
Is this backlog, P3, or for a contributor to take, P5?
P4 doesn't make sense here.
|
|
Assignee | |
Comment 7•5 years ago
|
||
I want to take this later, so change it to P3.
|
|
Assignee | |
Comment 8•5 years ago
|
||
This patch makes do_QueryFrame() accept const frame pointer e.g.
"const nsIFrame*", and also helps eliminate a few const_cast in Part 3.
Note that the fast path of do_QueryFrame is const-correct, but the slow
path is not (due to nsIFrame::QueryFrame() returns void*).
For example,
const nsIFrame* f;
nsBlockFrame* a = do_QueryFrame(f); // fast path, compile error.
nsIAnonymousContentCreator* b = do_QueryFrame(f); // slow path, still compiles.
|
|
Assignee | |
Comment 9•5 years ago
|
||
Depends on D19860
|
|
Assignee | |
Comment 10•5 years ago
|
||
Depends on D19861
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 11•5 years ago
|
||
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7e808f7e5cf9 Part 1 - Make do_QueryFrame more const-friendly, and mark nsIFrame::IsBlockFrameOrSubclass() as a const method. r=mats https://hg.mozilla.org/integration/autoland/rev/430adf8ca6b2 Part 2 - Replace some nsLayoutUtils::GetAsBlock() with nsIFrame::IsBlockFrameOrSubclass(). r=mats https://hg.mozilla.org/integration/autoland/rev/08b5d795ff19 Part 3 - Replace remaining nsLayoutUtils::GetAsBlock() with do_QueryFrame(), and delete nsLayoutUtils::GetAsBlock(). r=mats
|
||
Comment 12•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7e808f7e5cf9
https://hg.mozilla.org/mozilla-central/rev/430adf8ca6b2
https://hg.mozilla.org/mozilla-central/rev/08b5d795ff19
Description
•