569583 - nsWindow::GetParent() should use nsWindow::mParent now that we have it

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P5)

Description

[Mats Palmgren (inactive)]

nsWindow::GetParent() should use nsWindow::mParent now that we have it.
Follow-up from bug 530070.

Comment 1

[Amit S. Lakamsani [:asl]]

I'd like to have a go at this bug. Could someone assign it to me?

http://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#989

It seems to be as simple as returning mParent instead of calling GetParentWindow(). Is there more to this (I would imagine there is)?

Comment 2

[Jim Mathies [:jimm]]

Done, thanks!

Comment 3

[Mats Palmgren (inactive)]

(In reply to Amit S. Lakamsani [:asl] from comment #1)
> It seems to be as simple as returning mParent instead of calling
> GetParentWindow(). Is there more to this (I would imagine there is)?

I kind of doubt it's as simple as that ;-)

You'll probably need a local debug build to work on this.
I would probably start by adding
NS_ASSERTION(GetParentWindow(false) == mParent, "")
in nsWindow::GetParent just to get an idea of when/why they differ.
mParent is setup in nsWindow::Create, compare that with 
GetParentWindowBase(false).

Make sure you test with pages that use the Flash plugin.  For example,
dragging a tab containing a page that plays a Flash video from one window
to another window (the video should keep playing without disruption).
(our automated test coverage of plugins is poor)

Other things to test are the context menu and <select> dropdown menu.
(again, our test coverage is poor)

Dragging tabs between windows invokes SetParent, IIRC.

I suggest you keep this logic for now:
  if (mInDtor || mOnDestroyCalled)
    return nullptr;

(rather than trying to null out mParent at the right moment)
We can simplify/remove that in a follow-up bug.

Comment 4

[Amit S. Lakamsani [:asl]]

I apologize for my absence. I cannot take up this bug right now, and will leave it for someone else.

Comment 5

[Harsh Vardhan [:royalharsh95]]

Attachment: Code Cleanup for GetParent()

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8508995 [details] [diff] [review]
Code Cleanup for GetParent()

Review of attachment 8508995 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +987,5 @@
>  }
>  
> +nsIWidget* nsWindow::GetParent()
> +{
> +  if (mWindowType == eWindowType_dialog || mWindowType == eWindowType_toplevel || mWindowType == eWindowType_popup || mWindowType == eWindowType_invisible) {

Please wrap and format this so that it fits within ~80 character line length.

Although, I think you just want to use mIsTopWidgetWindow per the comments here -

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#1030

It would be interesting to push this to try for a full test run to see what shows up.

@@ +991,5 @@
> +  if (mWindowType == eWindowType_dialog || mWindowType == eWindowType_toplevel || mWindowType == eWindowType_popup || mWindowType == eWindowType_invisible) {
> +    return nullptr;
> +  }
> +  return mParent;
> +}   

nit - white space

Comment 7

[Harsh Vardhan [:royalharsh95]]

Attachment: Code Cleanup in nsWindow.cpp

Comment 8

[Jim Mathies [:jimm]]

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8fb6fe9e8845
remote:   https://tbpl.mozilla.org/?tree=Try&rev=8fb6fe9e8845

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #8)
> remote:  
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8fb6fe9e8845
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=8fb6fe9e8845

We've got bc1 test failures here, most of which indicate:
application crashed [@ nsCOMPtr<nsIWidget>::assign_assuming_AddRef(nsIWidget *)]

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8509054 [details] [diff] [review]
Code Cleanup in nsWindow.cpp

We need to sort out those test failures.

Comment 11

[Jim Mathies [:jimm]]

There's some functionality in GetParentWindowBase which we haven't replicated here - 

http://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#1037

That added check might be tripping up these tests. Just a guess.

Comment 12

[Mats Palmgren (inactive)]

It might be safer to add that check also in GetParent() for now (as I suggested
in comment 3).

Comment 13

[Harsh Vardhan [:royalharsh95]]

Couldn't drag tabs between windows and after playing youtube video whereas right clicking on the video-playing tab and selecting "Move to new Window" works.

Comment 14

[Harsh Vardhan [:royalharsh95]]

Attachment: GetParent now returns mParent in nsWindow.cpp

Comment 15

[Harsh Vardhan [:royalharsh95]]

I think the assertions which I had set, like this, NS_ASSERTION(GetParentWindow(false) == mParent, "")
in nsWindow::GetParent() was the reason I couldn't drag tabs from one window to another window.

Comment 16

[Jim Mathies [:jimm]]

You can't request checkin until you have review.

Comment 17

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07a467d295d5

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 8615129 [details] [diff] [review]
GetParent now returns mParent in nsWindow.cpp

Review of attachment 8615129 [details] [diff] [review]:
-----------------------------------------------------------------

cleanup the whitespace and I think we're good to go here.

::: widget/windows/nsWindow.cpp
@@ +989,5 @@
>  
>  nsIWidget* nsWindow::GetParent(void)
>  {
> +  nsIWidget* widget = 0;
> +  

nit - you have white space throughout this patch, please clean that up first. Look at your patch in splinter view to see this.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 8615129 [details] [diff] [review]
GetParent now returns mParent in nsWindow.cpp

Review of attachment 8615129 [details] [diff] [review]:
-----------------------------------------------------------------

cleanup the whitespace and I think we're good to go here.

::: widget/windows/nsWindow.cpp
@@ +988,5 @@
>  }
>  
>  nsIWidget* nsWindow::GetParent(void)
>  {
> +  nsIWidget* widget = 0;

use nullptr here as well. Also, below you have one if block without braces, and one if block with braces.

@@ +989,5 @@
>  
>  nsIWidget* nsWindow::GetParent(void)
>  {
> +  nsIWidget* widget = 0;
> +  

nit - you have white space throughout this patch, please clean that up first. Look at your patch in splinter view to see this.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8615129 [details] [diff] [review]
GetParent now returns mParent in nsWindow.cpp

AddRef-ing the result here seems wrong.  This method doesn't currently AddRef,
nor does GetParent() on other platforms.

Comment 21

[Jim Mathies [:jimm]]

(In reply to Mats Palmgren (:mats) from comment #20)
> Comment on attachment 8615129 [details] [diff] [review]
> GetParent now returns mParent in nsWindow.cpp
> 
> AddRef-ing the result here seems wrong.  This method doesn't currently
> AddRef,
> nor does GetParent() on other platforms.

Good lord, you're right. What's that doing in there?

Comment 22

[Harsh Vardhan [:royalharsh95]]

Oh, I read this doc : https://developer.mozilla.org/en/docs/NS_IF_ADDREF

Comment 23

[Harsh Vardhan [:royalharsh95]]

Attachment: GetParent now returns mParent in nsWindow.cpp

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8617348 [details] [diff] [review]
GetParent now returns mParent in nsWindow.cpp

Review of attachment 8617348 [details] [diff] [review]:
-----------------------------------------------------------------

closer but not quite there yet!

::: widget/windows/nsWindow.cpp
@@ +988,5 @@
>  }
>  
>  nsIWidget* nsWindow::GetParent(void)
>  {
> +  nsIWidget* widget = nullptr;

get rid of this

@@ +992,5 @@
> +  nsIWidget* widget = nullptr;
> +  if (mIsTopWidgetWindow) {
> +    return nullptr;
> +  }
> +  if (mInDtor || mOnDestroyCalled) { 

nit - white space on the end of this line.

@@ +996,5 @@
> +  if (mInDtor || mOnDestroyCalled) { 
> +    return nullptr;
> +  }
> +  widget = (nsIWidget *)mParent;
> +  return widget;

here you can just return mParent, no need for the assignment.

Comment 25

[Harsh Vardhan [:royalharsh95]]

Attachment: GetParent now returns mParent in nsWindow.cpp

Comment 26

[Tracy Walker [:tracy]]

patch r+ over a year ago.  what to do here?

Comment 27

[jbonnafo]

Hello,

What would be needed in order to have the submitted patch integrated ?

Thanks,

Comment 28

[Timothy Nikkel (:tnikkel)]

Should we land this Jim?

Comment 29

[jbonnafo]

Hello,

If needed, i am happy to help to get the patch proposed by Harsh Vardhan landing.

Thanks,

Comment 30

[Jim Mathies [:jimm]]

Yeah this can land after some testing and a try run. I'd prefer someone with checkin perms who's interested in dealing with any fallout.

Comment 31

[Timothy Nikkel (:tnikkel)]

Try run is clean:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25848fb44f6e51ad95bccfb98ae9e5e0a638f7fd

(I'm not planning on working on this bug any further.)

Comment 32

[jbonnafo]

Hello,

Please tell me if i can help on this issue.

Thanks,

Comment 33

[jbonnafo]

Hello,

What should be the next steps in order to make this fix landing ?

Thanks,

Comment 34

[Jim Mathies [:jimm]]

(In reply to jbonnafo from comment #33)
> Hello,
> 
> What should be the next steps in order to make this fix landing ?
> 
> Thanks,

Just needs to be checked in.

Comment 35

[Iris Hsiao [:ihsiao]]

please rebase the trunk.
--
adding 569583 to series file
renamed 569583 -> bug569583_returnsmParent.diff
applying bug569583_returnsmParent.diff
patching file widget/windows/nsWindow.cpp
Hunk #1 FAILED at 983
1 out of 1 hunks FAILED -- saving rejects to file widget/windows/nsWindow.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug569583_returnsmParent.diff

Comment 36

[jbonnafo]

Attachment: GetParent now returns mParent in nsWindow.cpp, following code rebase

Patch proposal following code rebase.

Comment 37

[Jim Mathies [:jimm]]

Comment on attachment 8824548 [details] [diff] [review]
GetParent now returns mParent in nsWindow.cpp, following code rebase

this doesn't return anything!

Comment 38

[jbonnafo]

Attachment: GetParent returns nsWindow::mParent in nsWindow.cpp, r:jimm

Amended patch proposal to return a value.

Comment 39

[Jim Mathies [:jimm]]

Lets see if this sticks.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e56d8197334
GetParent returns nsWindow::mParent in nsWindow.cpp. r=jimm

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e56d8197334