If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

clean up GetFrameForPoint

RESOLVED FIXED in Future

Status

()

Core
Event Handling
P4
normal
RESOLVED FIXED
16 years ago
12 years ago

People

(Reporter: dbaron, Assigned: Eli Friedman)

Tracking

Trunk
Future
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

GetFrameForPoint uses nsresult in a silly way (kinda like NS_COMFALSE, but not
quite so bad).  It should probably just be NS_IMETHOD_(PRBool) and return
PR_TRUE when it finds the point and PR_FALSE when it doesn't find the point.
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → Future

Updated

16 years ago
QA Contact: madhur → rakeshmishra

Updated

15 years ago
QA Contact: rakeshmishra → trix
(Assignee)

Comment 1

12 years ago
Taking bug and generalizing it to include changing to frame coordinates instead
of the ridiculous sort-of-parent-frame coordinates  (I'm figuring it's okay to
take this bug because it hasn't been touched since 2002.)
Assignee: dbaron → sharparrow1
Status: ASSIGNED → NEW
Summary: remove bogus nsresult return value from GetFrameForPoint → clean up GetFrameForPoint
Bring it on!!!
Actually I think it should just return the frame found, or nsnull.
(Assignee)

Comment 4

12 years ago
Created attachment 195593 [details] [diff] [review]
Patch v1

I went with 
virtual nsIFrame* GetFrameForPoint(const nsPoint&    aPoint,
				   nsFramePaintLayer aWhichLayer)
Attachment #195593 - Flags: review?(roc)
mmm MMMM mmm that looks good.

+      hit = kid->GetFrameForPoint(aPoint - kid->GetOffsetTo(this),
+                                  NS_FRAME_PAINT_LAYER_FOREGROUND);
+      if (!hit) {
+        hit = kid->GetFrameForPoint(aPoint - kid->GetOffsetTo(this),
+                                    NS_FRAME_PAINT_LAYER_FLOATS);
+        if (!hit) {
+          hit = kid->GetFrameForPoint(aPoint - kid->GetOffsetTo(this),
+                                      NS_FRAME_PAINT_LAYER_BACKGROUND);
         }
       }
     } else {
-      rv = kid->GetFrameForPoint(tmp, aWhichLayer, &hit);
+      hit = kid->GetFrameForPoint(aPoint - kid->GetOffsetTo(this),  aWhichLayer);

factor out aPoint - kid->GetOffsetTo(this) into common variable?

I think you need to change nsBox::GetDebugBoxAt to be consistent with this.
(Assignee)

Comment 6

12 years ago
(In reply to comment #5)
> factor out aPoint - kid->GetOffsetTo(this) into common variable?
Sure.  Changed in my tree.

> I think you need to change nsBox::GetDebugBoxAt to be consistent with this.
I think I'm just going to pass frame parent coordinates to that function; I
haven't looked at or used the XUL frame debug code, so I don't really want to
mess around with it.
It looks like it'd be really easy to convert.
(Assignee)

Comment 8

12 years ago
Created attachment 196098 [details] [diff] [review]
Patch v2

Changed as requested.
Attachment #195593 - Attachment is obsolete: true
Attachment #196098 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Attachment #195593 - Flags: review?(roc)
Comment on attachment 196098 [details] [diff] [review]
Patch v2

Yabba dabba doo!
Attachment #196098 - Flags: superreview+
Attachment #196098 - Flags: review?(roc)
Attachment #196098 - Flags: review+
(Assignee)

Updated

12 years ago
Blocks: 295941
(Assignee)

Comment 10

12 years ago
Would you please check this in?
Will do, now that I'm back at work for the day.

I'm going away for a couple of weeks and will have limited availability during
that time, though I'll try to stay current on bugmail. (See
http://weblogs.mozillazine.org/roc/)
checked in. Much thanks!
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 13

12 years ago
SVG also needs updates for this change.  Are you interested in taking care of that?
(Assignee)

Comment 14

12 years ago
Created attachment 196619 [details] [diff] [review]
Supplemental SVG Fix

I'm not sure how I missed the SVG implementation of GetFrameForPoint; here are
the necessary changes.	(BTW, were you talking about that or
GetFrameForPointSVG?)
Attachment #196619 - Flags: review?(roc)

Comment 15

12 years ago
Looks like you attached the wrong file.  No, I wasn't talking about
GetFrameForPointSVG - just the use/implementation of GetFrameForPoint in
nsSVGForeignObjectFrame and nsSVGOuterSVGFrame.

Comment 16

12 years ago
Comment on attachment 196098 [details] [diff] [review]
Patch v2

>+    nsIFrame* frame;
>+    if (frame = selectedBox->GetFrameForPoint(aPoint -
>+                                                selectedBox->GetOffsetTo(this),
>+                                              aWhichLayer))
>+      return frame;
That's ugly coding... perhaps do it this way:
nsIFrame *frame = selectedBox->GetFrameForPoint ...
if (frame)
  return frame;
(Assignee)

Updated

12 years ago
Attachment #196619 - Flags: review?(roc)
(Assignee)

Comment 17

12 years ago
Created attachment 196729 [details] [diff] [review]
Real Supplemental Fix

Oops.  I don't know how I attached that.  Would you mind taking care of this?
Attachment #196619 - Attachment is obsolete: true
Attachment #196729 - Flags: review?(tor)

Comment 18

12 years ago
> Oops.  I don't know how I attached that.  Would you mind taking care of this?

Don't we need that mRect.x/y adjustment you removed in some cases?

(Assignee)

Comment 19

12 years ago
GetFrameForPoint now uses frame coordinates instead of frame parent coordinates.
 No correction in terms of mRect is needed; x and y should be the same as they
were before.

Updated

12 years ago
Attachment #196729 - Flags: review?(tor) → review+
(Assignee)

Updated

12 years ago
Attachment #196729 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 20

12 years ago
Reopening for SVG fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #196729 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 21

12 years ago
Would you mind checking this in?

Comment 22

12 years ago
Checked in, thanks.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 23

12 years ago
*** Bug 309756 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.