Closed Bug 500882 Opened 11 years ago Closed 10 years ago

Give nsIContent a pointer to its primary frame

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 2 obsolete files)

258.46 KB, patch
Details | Diff | Splinter Review
2.94 KB, patch
smaug
: review+
Details | Diff | Splinter Review
59.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
32.34 KB, patch
surkov
: review+
Details | Diff | Splinter Review
44.88 KB, patch
smaug
: review+
Details | Diff | Splinter Review
30.47 KB, patch
roc
: review+
smaug
: review+
Details | Diff | Splinter Review
7.66 KB, patch
roc
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
12.77 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
90.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
Depends on: 487667
Blocks: lazyfc
Blocks: 305898
Blocks: 518656
Blocks: 534226
Depends on: 535361
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #418102 - Flags: superreview?(roc)
Attachment #418102 - Flags: review?(Olli.Pettay)
Switches to the new getter and setter for the actual work and removes the primary frame map and the primary-frame-finding code.
Attachment #418103 - Flags: review?(roc)
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!
Attachment #418104 - Flags: superreview?(roc)
Attachment #418104 - Flags: review?(surkov.alexander)
See comment 3 for review directions.  Especially in the event stuff...
Attachment #418105 - Flags: superreview?(roc)
Attachment #418105 - Flags: review?(Olli.Pettay)
See comment 3 for review directions.
Attachment #418106 - Flags: review?(roc)
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.
Attachment #418107 - Flags: review?(roc)
Attachment #418107 - Flags: review?(Olli.Pettay)
Attachment #418108 - Flags: superreview?(dbaron)
Attachment #418108 - Flags: review?(roc)
Attachment #418108 - Attachment description: Remove the old GetPrimaryFrame API → Part 7. Remove the old GetPrimaryFrame API
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/
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 on attachment 418108 [details] [diff] [review]
Part 7.  Remove the old GetPrimaryFrame API

sr=dbaron
Attachment #418108 - Flags: superreview?(dbaron) → superreview+
Attachment #418102 - Flags: review?(Olli.Pettay) → review+
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 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.
Attachment #418105 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #418107 - Flags: review?(Olli.Pettay) → review+
Attachment #418109 - Flags: review?(Olli.Pettay) → review+
Not sure, but tests pass with this patch, so I think it's ok.
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
Attachment #418102 - Flags: superreview?(roc) → superreview+
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 :-)
Attachment #418103 - Flags: review?(roc) → review+
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
Attachment #418104 - Flags: superreview?(roc) → superreview+
> 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.  ;)
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.
(In reply to comment #20)
> Or did you want a comment explaining that in the frameset code?

No, that's OK I guess.
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?
Attachment #418105 - Flags: superreview?(roc) → superreview+
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.
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 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
Attachment #418104 - Flags: review?(surkov.alexander) → review+
> it sounds shell is not needed here.
...
> presShell is not needed as well

Indeed.  Removed.
> 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?
(In reply to comment #27)
> I can do that.  Would you prefer that?

Yes, slightly.
I'm going to fold this into part 2.
Attachment #418927 - Flags: review?(roc)
Attachment #418927 - Attachment is obsolete: true
Attachment #418928 - Flags: review?(roc)
Attachment #418927 - Flags: review?(roc)
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.
Attachment #418106 - Attachment is obsolete: true
Attachment #418932 - Flags: review?(roc)
Attachment #418106 - Flags: review?(roc)
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.
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 536716
Blocks: 536718
Filed bug 536716 on reenabling the assertion.
Blocks: 536772
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?
> 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.
Target Milestone: --- → mozilla1.9.3a1
Depends on: 537041
Depends on: 538063
No longer depends on: 538063
Depends on: 547692
Blocks: 526853
Blocks: 538062
No longer blocks: 538062
Depends on: 538062
You need to log in before you can comment on or make changes to this bug.