1527519 - Unify nsLayoutUtils::GetAsBlock() and nsIFrame::IsBlockFrameOrSubclass()

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

[Mats Palmgren (inactive)]

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.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

[Emilio Cobos Álvarez (:emilio)]

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

[Mats Palmgren (inactive)]

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

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Is this backlog, P3, or for a contributor to take, P5?

P4 doesn't make sense here.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I want to take this later, so change it to P3.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1527519 Part 1 - Make do_QueryFrame more const-friendly, and mark nsIFrame::IsBlockFrameOrSubclass() as a const method.

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.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1527519 Part 2 - Replace some nsLayoutUtils::GetAsBlock() with nsIFrame::IsBlockFrameOrSubclass().

Depends on D19860

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1527519 Part 3 - Replace remaining nsLayoutUtils::GetAsBlock() with do_QueryFrame(), and delete nsLayoutUtils::GetAsBlock().

Depends on D19861

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/7e808f7e5cf9
https://hg.mozilla.org/mozilla-central/rev/430adf8ca6b2
https://hg.mozilla.org/mozilla-central/rev/08b5d795ff19