Closed Bug 1527519 Opened 2 years ago Closed 2 years ago

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


(Core :: Layout, enhancement, P3)




Tracking Status
firefox67 --- fixed


(Reporter: TYLin, Assigned: TYLin)




(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.

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.

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 ;)

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.

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.

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.

Flags: needinfo?(svoisen)

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

Flags: needinfo?(svoisen)
Priority: P4 → P3
See Also: → 1526097

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: nobody → aethanyc
Pushed by
Part 1 - Make do_QueryFrame more const-friendly, and mark nsIFrame::IsBlockFrameOrSubclass() as a const method. r=mats
Part 2 - Replace some nsLayoutUtils::GetAsBlock() with nsIFrame::IsBlockFrameOrSubclass(). r=mats
Part 3 - Replace remaining nsLayoutUtils::GetAsBlock() with do_QueryFrame(), and delete nsLayoutUtils::GetAsBlock(). r=mats
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.