513990 - Add provisions to nsFrameLoader for reflow and resize events

Status: REOPENED

(Core :: Layout, defect)

Description

[Frederic Plourde (:fred23)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

Needed for electrolysis.
Currently, nsSubDocumentFrame only directly notifies its docshell about size changes and reflows but really should notify nsFrameLoader instead, so that the frameLoader can take proper action depending on the case : either updating its docshell in the single-process case, or talking to its subprocess in the IPC case.

Reproducible: Always

Steps to Reproduce:
1.Build e10s and run test-ipc.xul
2.resize any window's borders
3.There should be no update of the client area

Comment 1

[Frederic Plourde (:fred23)]

Attachment: patch v.1

Here's a patch that adds provisions to nsFrameLoader for dynamic update of the childProcess' size and position. It merges MOZ_IPC and "single-process" codepaths inside nsFrameLoader::SetSizeAndPosition, which I exposed in the associated .idl file.

Notice in test-ipc.xul that we don't need to "restart" tab processes anymore to get proper resizing. But I didn't want to remove the 'restart' function, as it may still comes in handy in the future.

Notice also that I'm not really sure what to do with the "mPostedReflowCallback = PR_FALSE;" lines in nsFrameLoader.cpp because from there, I cannot reach mSubDocumentFrame scope in which mPostedReflowCallback lives. So for now, I just commented these lines (which I think is stupid), but the reflow functionality doesn't seem to be affected by that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 399079 [details] [diff] [review]
patch v.1

This doesn't look right.  In particular, we don't want to be doing GetDocShell in the IPC case, do we?  In that case, whatever information we would have gotten from the docshell we should get from the child process (assuming we need to get it at all; I doubt we do).

As for the mPostedReflowCallback thing, I wanted to make sure that any reflows triggered by GetPositionAndSize did not reenter this code.  You probably just want to set it after the call on the frameloader.

The idl should probably list this function as [noscript], right?

Comment 3

[Frederic Plourde (:fred23)]

(In reply to comment #2)
>In that case, whatever information we would have
> gotten from the docshell we should get from the child 
> process (assuming we need to get it at all; I doubt we do).

I'm confused. From what you wrote, I understand that current window size, taken from the presContext (again, itself from docShell) should now be taken from the child process. But you're saying "assuming we need to get it at all [from the child process], I doubt we do". So the question is, where could we possibly get the window's current size and position (after user intervention) from the chrome, if it's not from the docshell ?


> As for the mPostedReflowCallback thing, I wanted to make sure that any reflows
> triggered by GetPositionAndSize did not reenter this code.  You probably just
> want to set it after the call on the frameloader.

Do you mean: after the "GetPositionAndSize()" call in frameloader::SetSizeAndPosition?  If that's the case, I'd have to rearch nsFrameFrame accessors because I have no access to mPostedReflowCallback from nsFrameLoader. mhh, I kinda dislike that.
 

> The idl should probably list this function as [noscript], right?
true, I'll update.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> where could we possibly get the window's current size and position

We don't get the current size.  We just get the current position.  In the subprocess case, I suspect this might just not be needed.  It's certainly reasonable to proceed on that assumption for the moment; if we discover that it is (by writing a test where a iframe's position changes when that iframe has a cross-process browser inside) and seeing whether things work right.

> Do you mean: after the "GetPositionAndSize()" call in
> frameloader::SetSizeAndPosition?

No, I mean after the call so frameloader::SetSizeAndPosition returns.

Comment 5

[Frederic Plourde (:fred23)]

Attachment: patch v.2

I've updated the patch according to our previous discussions.

However, I added "baseWin->Repaint(PR_TRUE);" in TabChild.cpp because the iframe would not refresh its size if I reduced the window's width (by dragging the window's right edge towards the left), though everything was working perfectly if I increased the window's width towards the right.

bz: Tell me if this "Repaint" is not appropriate here and I'll re-update the patch. thanks.

Comment 6

[Frederic Plourde (:fred23)]

Attachment: patch v.3

patch v.2 was bogus, please consider this one.
----------------------------------------------

I've updated the patch according to our previous discussions.

However, I added "baseWin->Repaint(PR_TRUE);" in TabChild.cpp because the
iframe would not refresh its size if I reduced the window's width (by dragging
the window's right edge towards the left), though everything was working
perfectly if I increased the window's width towards the right.

bz: Tell me if this "Repaint" is not appropriate here and I'll re-update the
patch. thanks.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  I think you need to subtract out the used borderpadding in both codepaths from the frame's size, no?  The |return PR_FALSE| in SetSizeAndPosition should be setting the retval instead.  Do we need the manual NS_OUTPARAM annotation on IDL methods?  I didn't think we did.  Why do we even need the return value if we know it's always false?

In the frame code, you need an nsWeakFrame check on |this|, because SetSizeAndPosition can destroy the object, and you're setting mPostedReflowCallback after that.

The Repaint() call looks very suspicious to me.  We shouldn't need that.  I'll try compiling this patch and seeing what's up there.

Your SetPositionAndSize call on the basewindow lost the first two arguments somehow, right?

Comment 8

[Frederic Plourde (:fred23)]

Attachment: patch v.4

(In reply to comment #7)
> you need to subtract out the used borderpadding in both codepaths
> from the frame's size.

done.

> The |return PR_FALSE| in SetSizeAndPosition should
> be setting the retval instead.  

done.

> Why do we even need the return value if
> we know it's always false?

Done. I've completely removed the return value and defaulted PR_FALSE in nsFrameFrame.cpp.

> Do we need the manual NS_OUTPARAM annotation on
> IDL methods?  I didn't think we did.  

well, that's what the ipdl compiler provides us with in the header file.
It's true that the NS_OUTPARAM keyword is defined to 'blank' in the "non static analysis" case. I had just figured out it would be more proper coding to leave it there. But, anyways, I've completely removed the return value, as I said, so this doesn't apply anymore.
 
> In the frame code, you need an nsWeakFrame check on |this|, because
> SetSizeAndPosition can destroy the object, and you're setting
> mPostedReflowCallback after that.

done.

> The Repaint() call looks very suspicious to me.  We shouldn't need that.  I'll
> try compiling this patch and seeing what's up there.

ok, I've removed the Repaint because it seems suspicious to me too.
But let's not forget that repaint *still* doesn't actually occur/work when I decrease the frame's size.
I haven't investigated any further why, but I'll do eventually.

For now, I suggest we file another bug for that particular issue.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> well, that's what the ipdl compiler provides us with in the header file.

But this isn't ipdl; it's xpidl.  The static analysis boxes know to infer the outparam thing for xpidl.  Anyway, as you say this is not relevant.

Comment 10

[Boris Zbarsky [:bzbarsky]]

In the new patch, it looks like when MOZ_IPC is defined the !mChildProcess case will be completely broken.  This should be testable by loading a web page with an html:iframe in it in our xul:iframe, and then in that webpage resizing the iframe (via js, say).  With this last patch, I'd expect that to be broken. You still want a weakframe check in nsSubDocumentFrame::ReflowFinished after calling mFrameLoader->SetPositionAndSize.

Probably better to use UTF-8 for the non-ascii chars, I suspect.

The idl comment should be more like:

  Updates the position and size of the subdocument loaded by this frameloader.

And perhaps name the function updatePositionAndSize.  And document that aIFrame is the nsIFrame for the content node that owns this frameloader?

Comment 11

[Frederic Plourde (:fred23)]

(In reply to comment #10)
> In the new patch, it looks like when MOZ_IPC is defined the !mChildProcess case
> will be completely broken.

mhh, in the MOZ_IPC case, I thought there was no need to reflow in we didn't have any child process, hence, if we didn't have any content to show. Maybe you can shed some light on that last statement of mine ;-) thanks.

Comment 12

[Boris Zbarsky [:bzbarsky]]

In the MOZ_IPC case, we have mChildProcess only if this is a <xul:iframe> in a chrome document and has type="content" (or content-something).  In all other cases (HTML frame/iframe, chrome type child of a chrome document, anything in a non-chrome document) we take the old non-multiprocess codepath.

Comment 13

[Frederic Plourde (:fred23)]

Attachment: Test web page to exercice <html:iframe> codepath in the MOZ_IPC case

Comment 14

[Frederic Plourde (:fred23)]

Attachment: patch v.5

(In reply to comment #10)
> In the new patch, it looks like when MOZ_IPC is defined the !mChildProcess case will be completely broken.  This should be testable by loading a web page with an html:iframe in it in our xul:iframe, and then in that webpage resizing the iframe (via js, say). 

You're right! I've written a short test web page (see attachment #5 [details]) that exercices this "MOZ_IPC with <html:iframe>" case, and the internal content of the iframe was completely broken (absent). So I've reorganized the code and retested, and all three codepaths now runs smoothly, that is: 

  1) !MOZ_IPC : takes former codepath.
  2) MOZ_IPC with mChildProcess    : takes new mChildProcess->move() codepath.
  3) MOZ_IPC withOUT mChildProcess : takes former codepath.

note: I *really* wanted to avoid code duplication, but it wasn't possible without altering the function structure. So I split the code into two private subfunctions : "AjustToBorderAndPadding(...)" and "UpdateBaseWindowPositionAndSize(...)". I prefer that over duplicating portions of code.

> You still want a weakframe check in nsSubDocumentFrame::ReflowFinished after
> calling mFrameLoader->SetPositionAndSize.

Done.

> Probably better to use UTF-8 for the non-ascii chars, I suspect.

Done.

> The idl comment should be more like: "Updates the position and 
> size of the subdocument loaded by this frameloader."

Done.

> And perhaps name the function updatePositionAndSize. And document
> that aIFrame is the nsIFrame for the content node that owns this frameloader?

Done.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 403236 [details] [diff] [review]
patch v.5

>+++ b/content/base/src/nsFrameLoader.cpp
>+nsFrameLoader::UpdatePositionAndSize(nsIFrame *aIFrame)
>+{
>+  nsresult rv;
>+#ifdef MOZ_IPC
>+  if (mChildProcess) {
>+    PRInt32 cx, cy;
>+    AdjustToBorderAndPadding(aIFrame, cx, cy);
>+    mChildProcess->Move(0, 0, cx, cy);
>+  } else {
>+    rv = UpdateBaseWindowPositionAndSize(aIFrame);
>+  }
>+#else
>+  rv = UpdateBaseWindowPositionAndSize(aIFrame);
>+#endif
>+
>+  return rv;

In the MOZ_IPC case when mChildProcess, this returns a random value.  Luckily no one checks the return value...

How about structuring this like so to avoid issues like that:

#ifdef MOZ_IPC
  if (mChildProcess) {
    PRInt32 cx, cy;
    AdjustToBorderAndPadding(aIFrame, cx, cy);
    mChildProcess->Move(0, 0, cx, cy);
    return NS_OK;
  }
#endif
  return UpdateBaseWindowPositionAndSize(aIFrame);

>+nsFrameLoader::UpdateBaseWindowPositionAndSize(nsIFrame *aIFrame)
>+  PRInt32 cx, cy;

These should probably be declared closer to where they're used, but see below.

>+    baseWindow->SetPositionAndSize(0, 0, cx, cy, PR_FALSE);

This is wrong.  You lost the x and y.

>+nsFrameLoader::AdjustToBorderAndPadding(const nsIFrame *aIFrame, PRInt32 & cx, PRInt32 & cy)

This should probably be called GetSubdocumentSize and return an nsIntSize instead of using out params.

>+  // Sadly, XUL smacks the frame size without changing the used
>+  // border and padding, so we can't trust those.  Subtracting
>+  // them might make things negative.

This comment can go away; see below.

>+  nsPresContext* presContext = aIFrame->PresContext();
>+  nsSize innerSize(aIFrame->GetSize());
>+  nsIContent* content = aIFrame->GetContent();
>+  if (content) {
>+    nsCOMPtr<nsIDOMHTMLFrameElement> frameElem = do_QueryInterface(content);
>+    if (!frameElem) {
>+      nsMargin usedBorderPadding = aIFrame->GetUsedBorderAndPadding();
etc

How about:

  nsSize docSizeAppUnits;
  nsCOMPtr<nsIDOMHTMLFrameElement> frameElem = 
    do_QueryInterface(aIFrame->GetContent);
  if (frameElem) {
    docSizeAppUnits = aIFrame->GetSize();
  } else {
    docSizeAppUnits = aIFrame->GetContentRect().Size();
  }

>+      innerSize.width  -= usedBorderPadding.LeftRight();
>+      innerSize.width = PR_MAX(innerSize.width, 0);
>+
>+      innerSize.height -= usedBorderPadding.TopBottom();
>+      innerSize.height = PR_MAX(innerSize.height, 0);

And nix all this stuff.

>+  cx = presContext->AppUnitsToDevPixels(innerSize.width);
>+  cy = presContext->AppUnitsToDevPixels(innerSize.height);

return nsIntSize(presContext->AppUnitsToDevPixels(docSizeAppUnits.width),
                 presContext->AppUnitsToDevPixels(docSizeAppUnits.height));

>+++ b/content/base/src/nsFrameLoader.h
>+  NS_HIDDEN_(void) AdjustToBorderAndPadding(const nsIFrame *aIFrame, PRInt32 & cx, PRInt32 & cy);
>+  NS_HIDDEN_(nsresult) UpdateBaseWindowPositionAndSize(nsIFrame *aIFrame);

Document the methods (and in particular the fact that the latter is only used when we have our own in-process docshell)?

Comment 16

[Frederic Plourde (:fred23)]

Attachment: patch v.6

(In reply to comment #15)
> How about structuring this like so to avoid issues like that:
> 
> #ifdef MOZ_IPC
>   if (mChildProcess) {
>     PRInt32 cx, cy;
>     AdjustToBorderAndPadding(aIFrame, cx, cy);
>     mChildProcess->Move(0, 0, cx, cy);
>     return NS_OK;
>   }
> #endif
>   return UpdateBaseWindowPositionAndSize(aIFrame);

Done.

 
> >+nsFrameLoader::UpdateBaseWindowPositionAndSize(nsIFrame *aIFrame)
> >+  PRInt32 cx, cy;
> 
> These should probably be declared closer to where they're used, but see below.


OK. Doesn't apply anymore now that we use nsIntSize.

 
> >+    baseWindow->SetPositionAndSize(0, 0, cx, cy, PR_FALSE);
> 
> This is wrong.  You lost the x and y.

added x and y to the call. thanks

 
> >+nsFrameLoader::AdjustToBorderAndPadding(const nsIFrame *aIFrame, PRInt32 & cx, PRInt32 & cy)
> 
> This should probably be called GetSubdocumentSize and return an nsIntSize
> instead of using out params.

Done.


> >+  // Sadly, XUL smacks the frame size without changing the used
> >+  // border and padding, so we can't trust those.  Subtracting
> >+  // them might make things negative.
> This comment can go away; see below.

yeah. Done.

 
> >+  nsPresContext* presContext = aIFrame->PresContext();
> >+  nsSize innerSize(aIFrame->GetSize());
> >+  nsIContent* content = aIFrame->GetContent();
> >+  if (content) {
> >+    nsCOMPtr<nsIDOMHTMLFrameElement> frameElem = do_QueryInterface(content);
> >+    if (!frameElem) {
> >+      nsMargin usedBorderPadding = aIFrame->GetUsedBorderAndPadding();
> etc
> 
> How about:
> 
>   nsSize docSizeAppUnits;
>   nsCOMPtr<nsIDOMHTMLFrameElement> frameElem = 
>     do_QueryInterface(aIFrame->GetContent);
>   if (frameElem) {
>     docSizeAppUnits = aIFrame->GetSize();
>   } else {
>     docSizeAppUnits = aIFrame->GetContentRect().Size();
>   }
> 
> >+      innerSize.width  -= usedBorderPadding.LeftRight();
> >+      innerSize.width = PR_MAX(innerSize.width, 0);
> >+
> >+      innerSize.height -= usedBorderPadding.TopBottom();
> >+      innerSize.height = PR_MAX(innerSize.height, 0);
> 

Sounds good to me! Feels good to finally get rid of that!
Done.


> >+++ b/content/base/src/nsFrameLoader.h
> >+  NS_HIDDEN_(void) AdjustToBorderAndPadding(const nsIFrame *aIFrame, PRInt32 & cx, PRInt32 & cy);
> >+  NS_HIDDEN_(nsresult) UpdateBaseWindowPositionAndSize(nsIFrame *aIFrame);
> 
> Document the methods (and in particular the fact that the latter is only used
> when we have our own in-process docshell)?

Done.
but I haven't added any comments in the two newly created function *bodies*
because I think they are self-explanatory. I documented the declaration in the .h

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 404530 [details] [diff] [review]
patch v.6

This looks great.  Can you push to the e10s repo, or should I push this?

Comment 18

[Frederic Plourde (:fred23)]

Thanks!
I'll first update to *tip* and see if the patch still applies smoothly and I'll push. Hmm... might need special privileges though. In that case, then, I'll just let you push it for me.

Comment 19

[Frederic Plourde (:fred23)]

Attachment: Final patch to push with commit info

bz: Ok, I just pulled and updated the tree, and patch v.6 applies without a glitch. However, as I thought, I don't have LDAP access to the repo, so I'll kindly ask you that you push this final (same) patch with commit info for me.
thanks !

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/04562dce2157 with one minor change: moved the new method to the end of the interface, and revved the iid.  Sorry I didn't catch that earlier...

Benjamin, do you think it's worth trying to get the non-MOZ_IPC parts of this landed on m-c?

Comment 21

[Benjamin Smedberg]

It doesn't look that important to me, no, and would probably cause merge headaches.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ok.  I thought it might reduce them, which is why I suggested it.  The goal is easier merging, period.

Comment 23

[Benjamin Smedberg]

This doesn't appear to have actually worked on Linux: we end up with a very small content area until hitting the recover button.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Hmm... presumably because the Move() call is not sufficient on Linux; we probably need to resize the gtk socket too.

I wonder whether we can push that down into the Move call.

Comment 25

[Boris Zbarsky [:bzbarsky]]

So I tried reproducing comment 23 with an FC9 client and OSX as the server (FC9 box is headless) and don't see the issue.

Going to try bringing this up in my fc9 vm, I guess.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I can't reproduce in my fc9 vm either.  Things seem to work just fine.

Comment 27

[Frederic Plourde (:fred23)]

bsmedberg : any news on that front ?  Are you still getting that tiny window issue ?

Comment 28

[Benjamin Smedberg]

Yes, jdm and I are both seeing it.

Comment 29

[Doug Turner (:dougt)]

are we still seeing this?

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.