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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

3 months ago

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

Comment 2

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

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

Comment 7

3 months ago

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

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

Comment 8

3 months 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

Updated

3 months ago
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED

Comment 11

3 months 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

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.