937298 - deCOM nsIFrame::GetOffsetFromView

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Daniel Holbert [:dholbert]]

nsIFrame::GetOffsetFromView is declared as NS_IMETHOD.

It only has one implementation (in nsFrame.cpp), which always returns NS_OK, and none of the callers bother to check the return value anyway.  So the return value is useless.

We should just make it return void, to reflect reality.

Comment 1

[Daniel Holbert [:dholbert]]

(Note that both of GetOffsetFromView()'s parameters are really outparams. It's possible that one should be promoted to be the actual return value, but I won't do that here, to keep this patch simple.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: Make function return void instead of NS_OK (and drop one useless failure-check)

(In reply to Daniel Holbert [:dholbert] from comment #0)
> and none of the callers bother to check the return value anyway

Actually, there's one caller that bothers to check, in nsFrame.cpp, but the check is useless since it will never fail (since the function always returns NS_OK).

So, this patch just drops that check.

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 830396 [details] [diff] [review]
fix v1: Make function return void instead of NS_OK (and drop one useless failure-check)

>+nsFrame::GetOffsetFromView(nsPoint& aOffset,
>+                           nsView** aView) const

Looks like it fits on one line.  Ditto the declaration in nsIFrame.h

r=mats

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v2: Make function return void instead of NS_OK (and fix one useless failure-check)

Actually, instead of dropping the useless error-check, I should probably fix it to check what it almost certainly wants to be checking.

As noted in the documentation for GetOffsetFromView(), this function indicates an error by returning null in its "view" outparam. (This happens if it internally walks off the top of the frame tree, accumulating offsets, without finding any frame with a view along the way.)

So, the currently-useless error check in nsFrame.cpp probably wants to null-check |view| and fail if it's null. Note that that's what we do elsewhere, e.g.:
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#1015

Comment 5

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> (Note that both of GetOffsetFromView()'s parameters are really outparams.
> It's possible that one should be promoted to be the actual return value, but
> I won't do that here, to keep this patch simple.)

Also, since there's only one implementation, could we move it to nsIFrame and
make it non-virtual?  It looks like the implementation just use stuff that is
available in nsIFrame.  We could add private declarations in non-nsFrame
classes that inherits nsIFrame to prevent accidental use there, if we feel
it's important.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)
> >+nsFrame::GetOffsetFromView(nsPoint& aOffset,
> >+                           nsView** aView) const
> 
> Looks like it fits on one line.  Ditto the declaration in nsIFrame.h

Good point -- I'll fix that up in the existing patch.

(In reply to Mats Palmgren (:mats) from comment #5)
> Also, since there's only one implementation, could we move it to nsIFrame and
> make it non-virtual?

I thought about that, too -- I'll do that in a second patch here.

> We could add private declarations in non-nsFrame
> classes that inherits nsIFrame to prevent accidental use there, if we feel
> it's important.

I don't think that's worth worrying about. (Do you know of any non-nsFrame concrete classes that inherit from nsIFrame? AFAICT nsBox is the only direct child of nsIFrame, and nsFrame is the only direct child of nsBox, so there shouldn't be any other non-nsFrame things that inherit from nsIFrame.)

Comment 7

[Mats Palmgren (inactive)]

It seems I was mistaken because I can't find any.  So nothing to worry about.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 1 v3: Make function return void instead of NS_OK (and fix one useless failure-check) (now with things collapsed to 1 line)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 2: de-virtualize GetOffsetFromView and put its sole impl on nsIFrame

Comment 10

[Daniel Holbert [:dholbert]]

Try run (Linux/B2G only): https://tbpl.mozilla.org/?tree=Try&rev=66b0de7ec022

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2da11f68be
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca4093ff578

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a2da11f68be
https://hg.mozilla.org/mozilla-central/rev/7ca4093ff578