500882 - Give nsIContent a pointer to its primary frame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: work-in-progress patch

Once we have cloning of documents for printing, I think we should do this.  Removes a bunch of fragile code and such....

The attached patch does the deed and passes tests, but we should spend a bit more time looking at callsites and either simplifying them or filing bugs ons simplifying them.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Add the member and accessors to nsIContent

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Remove the primary frame map

Switches to the new getter and setter for the actual work and removes the primary frame map and the primary-frame-finding code.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Switch accessibility to the new API

Please review this carefully.  The API change here is a little subtle:

1)  Getting a primary frame for a null content is no longer safe (used to return
    a null frame).
2)  Getting a primary frame for a content that is not in the same document as
    the presshell we used to get from no longer returns null.

I think I looked over the callsites here and these changes aren't a problem, but please double-check that!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4. Switch content to the new API

See comment 3 for review directions.  Especially in the event stuff...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5. Switch layout to the new api

See comment 3 for review directions.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6. Switch docshell, dom, editor, embedding, toolkit, widget to the new API

See comment 3 for review directions.  In particular, I actually hit a null-content crash running tests with this patchset; the patch in bug 535361 fixes that.  But the same-document thing also needs some attention, especially in the focus manager.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7. Remove the old GetPrimaryFrame API

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 8. Remove the unused (Set)MayHaveFrame vestiges

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 418102 [details] [diff] [review]
Part 1.  Add the member and accessors to nsIContent

>+   * is most closely associated with the content. A frame is more closely
>+   * associated with the content that another frame if the one frame contains

Just noticed this passing through.

s/that/than/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed that.  Serves me right for copy/pasting comments!

Also fixed the missing "is the" in the last paragraph of that comment; that one was totally my fault.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 418108 [details] [diff] [review]
Part 7.  Remove the old GetPrimaryFrame API

sr=dbaron

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418102 [details] [diff] [review]
Part 1.  Add the member and accessors to nsIContent

Since you fixed the small typos in the comment, r=me

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418105 [details] [diff] [review]
Part 4.  Switch content to the new API

Looks good to me. I couldn't see any null checking problems.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418107 [details] [diff] [review]
Part 6.  Switch docshell, dom, editor, embedding, toolkit, widget to the new API

>+++ b/widget/tests/TestWinTSF.cpp
>@@ -3065,18 +3065,18 @@ nsresult
> TestApp::GetSelCon(nsISelectionController** aSelCon)
> {
>   nsCOMPtr<nsIDocShell> docShell;
>   nsresult nsr = mWindow->GetDocShell(getter_AddRefs(docShell));
>   if (NS_SUCCEEDED(nsr) && docShell) {
>     nsCOMPtr<nsIPresShell> presShell;
>     nsr = docShell->GetPresShell(getter_AddRefs(presShell));
>     if (NS_SUCCEEDED(nsr) && presShell) {
>-      nsIFrame* frame = presShell->GetPrimaryFrameFor(
>-          nsCOMPtr<nsIContent>(do_QueryInterface(mCurrentNode)));
>+      nsIFrame* frame = 
>+        nsCOMPtr<nsIContent>(do_QueryInterface(mCurrentNode))->GetPrimaryFrame();
Not sure if this test could use a null check for mCurrentNode.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Not sure, but tests pass with this patch, so I think it's ok.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 418102 [details] [diff] [review]
Part 1.  Add the member and accessors to nsIContent

+   * In the case of absolutely positioned elements and floated elements, this
+   * frame out of flow frame, not the placeholder.

This frame *is* the out of flow

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 418103 [details] [diff] [review]
Part 2. Remove the primary frame map

@@ -409,16 +409,17 @@ +      child->SetPrimaryFrame(frame);

What's this needed for? r+ assuming a satisfactory explanation :-)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 418104 [details] [diff] [review]
Part 3.  Switch accessibility to the new API

Part 3:

   nsCOMPtr<nsIContent> content = GetSelectState(&state);
   if (state & nsIAccessibleStates::STATE_COLLAPSED) {
-    nsCOMPtr<nsIPresShell> presShell(GetPresShell());
-    if (!presShell) {
-      return nsnull;
-    }
-    return presShell->GetPrimaryFrameFor(content);
+    return content->GetPrimaryFrame();

Do we know content is non-null here?

sr+ with a null check or explanation

Comment 19

[Boris Zbarsky [:bzbarsky]]

> This frame *is* the out of flow

Yep, see comment 10.

> What's this needed for?

So that the child will have a non-null primary frame...  Frameset kids don't get their frames constructed by the frame constructor; they're completely constructed by nsHTMLFramesetFrame::Init.  As in, without that line we crash on a null pointer later on.  ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Or did you want a comment explaining that in the frameset code?

> Do we know content is non-null here?

I suspect we might, but I don't know for sure.  Certainly the callee function can return null if someone screws up the accessibility tree.  I'll add a null-check.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #20)
> Or did you want a comment explaining that in the frameset code?

No, that's OK I guess.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Part 4:
+  NS_ASSERTION(GetDocument(), "Have a frame but no document?");
+  NS_ASSERTION(GetDocument()->GetPrimaryShell(),
+               "Have a frame but not presshell?");
+  GetDocument()->GetPrimaryShell()->FrameNeedsReflow(frame,
+                                                     nsIPresShell::eTreeChange,
+                                                     NS_FRAME_IS_DIRTY);

In general should we recommend getting the presshell/prescontext through the document or through the frame, if you have both available?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh, good point.  In this case I should have just gotten the presshell off the frame; that guarantees that I'm reflowing the frame in the right presshell.  I'll fix that.

Where you get it from should probably depend on what you're using it for, conceptually.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Part 5:
+  if (mPresShell->GetDocument() != mPresShell->GetDocument()) {
+    // Just bail out
+    return NS_OK;

Something's wrong here? One of those should be mContent I guess

+      nsWeakFrame frame =
+        mContent->GetCurrentDoc() == mPresContext->PresShell()->GetDocument() ?
+          mContent->GetPrimaryFrame() : nsnull;

You've got this pattern in a few places. Should we have a GetPrimaryFrame method on nsPresContext that does this? With a comment that suggests using nsIContent::GetPrimaryFrame if you know (or don't care) which document the element belongs to?

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 418104 [details] [diff] [review]
Part 3.  Switch accessibility to the new API

>@@ -1505,17 +1505,17 @@ nsDocAccessible::FireTextChangeEventForT
>     aInfo->mChangeEnd - start; // text has been removed
> 
>   if (length > 0) {
>     nsCOMPtr<nsIPresShell> shell(do_QueryReferent(mWeakShell));
>     if (!shell)
>       return;

it sounds shell is not needed here.

>     PRUint32 renderedStartOffset, renderedEndOffset;
>-    nsIFrame* frame = shell->GetPrimaryFrameFor(aContent);
>+    nsIFrame* frame = aContent->GetPrimaryFrame();
>     if (!frame)
>       return;

>@@ -106,17 +106,17 @@ nsXULTabAccessible::GetStateInternal(PRU
> 
>   // In the past, tabs have been focusable in classic theme
>   // They may be again in the future
>   // Check style for -moz-user-focus: normal to see if it's focusable
>   *aState &= ~nsIAccessibleStates::STATE_FOCUSABLE;
>   nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>   nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(mWeakShell));
>   if (presShell && content) {
>-    nsIFrame *frame = presShell->GetPrimaryFrameFor(content);
>+    nsIFrame *frame = content->GetPrimaryFrame();
>     if (frame) {

presShell is not needed as well

Comment 26

[Boris Zbarsky [:bzbarsky]]

> it sounds shell is not needed here.
...
> presShell is not needed as well

Indeed.  Removed.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Something's wrong here? One of those should be mContent I guess

Yes.

> Should we have a GetPrimaryFrame method on nsPresContext that does this?

I can do that.  Would you prefer that?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #27)
> I can do that.  Would you prefer that?

Yes, slightly.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: prescontext diff to add the new API

I'm going to fold this into part 2.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: The right prescontext diff

Comment 31

[Boris Zbarsky [:bzbarsky]]

Er, and I had to add |#include "nsIContent.h"| to nsPresContext.h to get it to compile.  I could alternately take that function out of line.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 using the new prescontext thing

Comment 33

[Boris Zbarsky [:bzbarsky]]

I made one more change to part 1, which was adding:

    NS_PRECONDITION(!aFrame || !mPrimaryFrame,
                    "Losing track of existing primary frame");

to SetPrimaryFrame, so we keep catching cases like bug 536623.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Pushed to tryserver yet again, and discovered a crash (not 100% reproducible) in nsBoxObject::GetFrame on a null mContent.  Added a null-check.

Also merged to the changes from bug 508473, which meant restoring nsFrameManager::NotifyDestroyingFrame, since that now handles getting rid of the undisplayed content entries as needed.  I'll file a followup bug on cleaning it up more as needed.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/f6207bf52da3
  http://hg.mozilla.org/mozilla-central/rev/e4438ed238da
  http://hg.mozilla.org/mozilla-central/rev/7bd4d3a35bd3
  http://hg.mozilla.org/mozilla-central/rev/5824db6f9c71
  http://hg.mozilla.org/mozilla-central/rev/8b6f32659aa6
  http://hg.mozilla.org/mozilla-central/rev/a071473caf24
  http://hg.mozilla.org/mozilla-central/rev/e9921507713b
  http://hg.mozilla.org/mozilla-central/rev/d4802dd1cca9

And then

  http://hg.mozilla.org/mozilla-central/rev/4bbc5aa8a770

to fix debug orange due to the assert from comment 33 failing for <iframe>s (which actually call SetPrimaryFrame twice with the same value) by also checking for aFrame == mPrimaryFrame.  And then

  http://hg.mozilla.org/mozilla-central/rev/971844427505

to disable that assertion for now because http://www.mozilla.org/newlayout/samples/test8.html hits it (due to the combobox, and BuildScrollFrame setting primary frames eagerly, I bet).  Will file a followup to reenable the assertion.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Filed bug 536716 on reenabling the assertion.

Comment 37

[Timothy Nikkel (:tnikkel)]

From part 2:

     static const FrameConstructionData sInlineMathData =
       FCDATA_DECL(FCDATA_FORCE_NULL_ABSPOS_CONTAINER |
-                  FCDATA_WRAP_KIDS_IN_BLOCKS |
-                  FCDATA_SKIP_FRAMEMAP |
-                  FCDATA_IS_LINE_PARTICIPANT,
+                  FCDATA_WRAP_KIDS_IN_BLOCKS,
                   NS_NewMathMLmathInlineFrame);

Was getting rid of FCDATA_IS_LINE_PARTICIPANT intentional?

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Was getting rid of FCDATA_IS_LINE_PARTICIPANT intentional?

Not at all.  Thanks for catching this; saves me debugging some crashtest asserts I was running into!

Pushed http://hg.mozilla.org/mozilla-central/rev/3d730cfd955b to fix that.