Don't descend into child frames looking for views in ReparentFrameViewTo if the frame doesn't have the NS_FRAME_HAS_CHILD_WITH_VIEW bit set

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8737627 [details] [diff] [review]
patch
Attachment #8737627 - Flags: review?(mats)
Comment on attachment 8737627 [details] [diff] [review]
patch

Good catch!
Attachment #8737627 - Flags: review?(mats) → review+
It might also be worth making ReparentFrameViewTo return a bool, true if it finds a frame
with a view, and then remove the NS_FRAME_HAS_CHILD_WITH_VIEW bit in the else branch if
none of the calls there returned true?

Or, if we decide it's not worth it, we should at least make ReparentFrameViewTo void
since it always returns NS_OK.
Also, does the initial comment:

  // XXX What to do about placeholder views for "position: fixed" elements?
  // They should be reparented too.

make any sense to you?

It sure don't to me - I don't know what "placeholder views" might be (perhaps
a nsPlaceholderFrame with a view?).  I don't think we have such things so
the comment is just confusing and should be removed IMO.
(Assignee)

Comment 5

2 years ago
(In reply to Mats Palmgren (:mats) from comment #4)
> Also, does the initial comment:
> 
>   // XXX What to do about placeholder views for "position: fixed" elements?
>   // They should be reparented too.
> 
> make any sense to you?
> 
> It sure don't to me - I don't know what "placeholder views" might be (perhaps
> a nsPlaceholderFrame with a view?).  I don't think we have such things so
> the comment is just confusing and should be removed IMO.

I would have guessed "nsPlaceholderFrame with a view". Here's where that comment was added (in 2001)

http://52.25.115.98/viewvc/main/mozilla/layout/generic/nsHTMLContainerFrame.cpp?r1=3.159&r2=3.160&

At the bottom of that link we have the existing comment
  // XXX If it's fixed positioned, then create a widget so it floats
which describes a world very very different from what we have now. So I agree, let's remove it.
(Assignee)

Comment 6

2 years ago
(In reply to Mats Palmgren (:mats) from comment #3)
> It might also be worth making ReparentFrameViewTo return a bool, true if it
> finds a frame
> with a view, and then remove the NS_FRAME_HAS_CHILD_WITH_VIEW bit in the
> else branch if
> none of the calls there returned true?

Hmm, if we do this we should do the same for nsContainerFrame::PositionChildViews and AdjustViews in nsGfxScrollFrame.cpp as they do a similar frame tree walk.

Seems like it might be worth it, the extra code to do it is small, and the overhead should be very minimal for one extra branch.
I was thinking that the situation where that happens is so extremely rare on the web
that even a tiny bit of overhead might make it a sub-optimization.
(Assignee)

Comment 8

2 years ago
I also removed this useless comment (in a patch that I will check in):

  // Does aFrame have a view?
  if (aFrame->HasView()) {
(Assignee)

Comment 9

2 years ago
(In reply to Mats Palmgren (:mats) from comment #7)
> I was thinking that the situation where that happens is so extremely rare on
> the web
> that even a tiny bit of overhead might make it a sub-optimization.

Yeah. I don't feel strongly either way.
(Assignee)

Comment 10

2 years ago
Created attachment 8737698 [details] [diff] [review]
remove comments patch

I don't think this needs review since Mats suggested it.
(Assignee)

Comment 11

2 years ago
Created attachment 8737699 [details] [diff] [review]
Make ReparentFrameViewTo return void

I'll just make it return void, doesn't make the going the bool route any harder.
Attachment #8737699 - Flags: review?(mats)

Updated

2 years ago
Attachment #8737699 - Flags: review?(mats) → review+

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d479d7f6493
https://hg.mozilla.org/mozilla-central/rev/019d03cdbf2b
https://hg.mozilla.org/mozilla-central/rev/c2c09dd63bff
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.