REGRESSION(r218437): [GTK][WPE] Test /webkit2/WebKitWebExtension/isolated-world is failing since r218437

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Ms2ger (he/him; ⌚ UTC+1/+2)]

.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 318860 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 318860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318860&action=review

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:151
> -        "document.open(1, 2, 3);";
> +        "window.open(1, 2, 3);";

What mean the parameters 1, 2, 3 here?

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Carlos Alberto Lopez Perez from comment #2)
> Comment on attachment 318860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318860&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:151
> > -        "document.open(1, 2, 3);";
> > +        "window.open(1, 2, 3);";
> 
> What mean the parameters 1, 2, 3 here?

The forwarding to window.open() only happens with at least three arguments. No idea why this used such a roundabout approach; CC'ing some people from bug 103377.

[Carlos Alberto Lopez Perez]

(In reply to Ms2ger from comment #3)
> (In reply to Carlos Alberto Lopez Perez from comment #2)
> > Comment on attachment 318860 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318860&action=review
> > 
> > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:151
> > > -        "document.open(1, 2, 3);";
> > > +        "window.open(1, 2, 3);";
> > 
> > What mean the parameters 1, 2, 3 here?
> 
> The forwarding to window.open() only happens with at least three arguments.
> No idea why this used such a roundabout approach; CC'ing some people from
> bug 103377.

Just doing window.open() seems to work here to make the test pass

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #4)
> (In reply to Ms2ger from comment #3)
> > (In reply to Carlos Alberto Lopez Perez from comment #2)
> > > Comment on attachment 318860 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=318860&action=review
> > > 
> > > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:151
> > > > -        "document.open(1, 2, 3);";
> > > > +        "window.open(1, 2, 3);";
> > > 
> > > What mean the parameters 1, 2, 3 here?
> > 
> > The forwarding to window.open() only happens with at least three arguments.
> > No idea why this used such a roundabout approach; CC'ing some people from
> > bug 103377.
> 

Ah! I see what you mean.. previously (before r218437) 3 parameter where needed to pass the call from document.open to window.open()

> Just doing window.open() seems to work here to make the test pass

Now simply calling window.open without any parameter seems enough

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 318865 [details]
Patch

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Since I don't know what purpose the function call served, I just removed it entirely.

[Carlos Alberto Lopez Perez]

Comment on attachment 318865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318865&action=review

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:150
> -        "window.open = function () { alert('Main World'); }\n"
> -        "document.open(1, 2, 3);";
> +        "alert('Main World');";

Looks fine to me, but I prefer to wait for cgarcia r+ here (as I'm a bit confused about the previous window.open usage here and maybe there is something i'm overlooking)

[Carlos Garcia Campos]

I find the bug title a bit confusing, it's not that the test has a way of running pop ups, that was the way to test isolated worlds, it was actually a "port" of the layout test http/tests/security/isolatedWorld/document-open.html.

[Carlos Garcia Campos]

Comment on attachment 318865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318865&action=review

Thanks for looking at this failure, it has been in my TODO since it started to fail.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:161
> -        "window.open = function () { alert('Isolated World'); }\n"
> -        "document.open(1, 2, 3);";
> +        "alert('Isolated World');";

I'm not sure this is testing what we want, running this alert in the main world would still show an alert with the text 'Isolated World'. Here we were testing that property open of window can be set in an isolated world and it can be used, without affecting the normal world. Maybe we can simply keep the window.open line but use window open directly instead of document.open?

[Carlos Garcia Campos]

Comment on attachment 318865 [details]
Patch

Sorry, I used the wrong r flag. r- because I don't think the test is correct.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 319173 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 319173 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 319173 [details]
Patch

Clearing flags on attachment: 319173

Committed r221240: <http://trac.webkit.org/changeset/221240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.