Bug 87149 – Move setFrameFlatteningEnabled from layoutTestController to window.internals.settings

• Component: WebKit2
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

Implement setFrameFlatteningEnabled in window.internals and remove duplicated code from WebKitTestRunner and DumpRenderTree.

[Jesus Sanchez-Palencia]

Created attachment 143355 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #1)
> Created an attachment (id=143355) [details]
> Patch

A quick disclaimer: private APIs were removed from Mac and Win ports because I didn't find on trunk places where they were used. It might be wrong but I need people from other ports to have a look at this and let me know their opinion.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 143355 [details]
Patch

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

I have a couple of comments.

> Source/WebKit/mac/ChangeLog:9
> +        Remove Mac private API to get/set the frame flattening setting, since it was only there
> +        for the DRT.

Make sure that Apple developers are OK with this change.

> Source/WebKit/win/Interfaces/IWebPreferencesPrivate.idl:83
> +    HRESULT unused4();

It's OK to call these unused1() and unused2(), and renamed the existing to 3 and 4. To keep them sorted in the file.

[Build Bot]

Comment on attachment 143355 [details]
Patch

Attachment 143355 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12767011

[Gyuyoung Kim]

In order to build error on gtk, win ports, you should add new symbol to symbol filter file.

 win : Source/WebKit2/win/WebKit2.def
 gtk : Source/autotools/symbols.filter

CC'ing Alexey.

[Jesus Sanchez-Palencia]

Thanks for the reviews, I will upload a new patch soon.

[Jesus Sanchez-Palencia]

Created attachment 143569 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 143569 [details]
Patch

Attachment 143569 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12772246

[Alexey Proskuryakov]

I cannot confidently say whether we need the WebKit1 SPI and/or the WebKitFrameFlatteningEnabled preference, CC'ing some folks who may have the answer.

WebKit2 bundle SPI that you remove in this patch seems unnecessary indeed.

[Jesus Sanchez-Palencia]

Created attachment 143599 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> I cannot confidently say whether we need the WebKit1 SPI and/or the WebKitFrameFlatteningEnabled preference, CC'ing some folks who may have the answer.
> 
> WebKit2 bundle SPI that you remove in this patch seems unnecessary indeed.

Hey guys, any opinions here?

[Eric Seidel (no email)]

Comment on attachment 143599 [details]
Patch

I would take the approach of moving the logic, but *not* removing the preference/SPI from any port.  Leave it to the port maintainers to trim down their own SPIs.  You could put FIXME's there, but I think tryihng to change another port's SPI/API is more trouble than it's worth.

[Jesus Sanchez-Palencia]

Created attachment 144117 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 144607 [details]
Patch

[Ojan Vafai]

Comment on attachment 144607 [details]
Patch

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

> Source/WebKit/win/WebPreferences.h:419
> +    // FIXME: These can be removed after the move from layouttestcontroller

Nit: here and above, i think this needs a bit more qualification. Maybe something like "These can be removed if there are no Apple-internal calls to this. See http://webkit.org/b/87148."

You don't need to say the "after the move..." bit because this is the commit that makes that change. Otherwise, it sounds like it's blocked on something else getting committed.

> Tools/ChangeLog:9
> +        During the InternalsObject reset the behavior is changed back to the platform

nit: s/During/during

[Alexey Proskuryakov]

Comment on attachment 144607 [details]
Patch

I think that this is still blocked on Apple comments. Removing coverage without removing an API is evil.

Also, note the new blocking bug. Perhaps moving preference setters to internals was a bad idea in the first place.

[Jesus Sanchez-Palencia]

(In reply to comment #16)
> (From update of attachment 144607 [details])
> I think that this is still blocked on Apple comments. Removing coverage without removing an API is evil.

It's been 2 weeks you CC'ed some Apple guys already, and so far we are still waiting for such comments, Alexey. Would it be possible to get an ETA from Apple as we would like to move forward?

> 
> Also, note the new blocking bug. Perhaps moving preference setters to internals was a bad idea in the first place.

I'm sorry, I'm not following you here. Which new blocking bug?

thanks!

[Alexey Proskuryakov]

Sorry, I added it as blocking in other InternalSettings bugs, but forgot to add it here.

I don't think that we should proceed with this patch before bug 87993 gets resolved.

[Tony Chang]

Created attachment 183520 [details]
Patch

[Tony Chang]

I rewrote this patch now that bug 87993 is resolved.

[Hajime Morrita]

Comment on attachment 183520 [details]
Patch

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

> Tools/ChangeLog:3
> +        Move setFrameFlatteningEnabled from layoutTestController to window.internals()

Nit: can you say window.internals.settings?
It will clarify the point.

[Tony Chang]

Created attachment 184003 [details]
Patch for landing

[Tony Chang]

Comment on attachment 184003 [details]
Patch for landing

Actually, let me see if I can add a TestWebKitAPI test for this on Mac.

[Tony Chang]

Created attachment 184323 [details]
Patch for landing

[Tony Chang]

Created attachment 184324 [details]
Patch

[Tony Chang]

Comment on attachment 184324 [details]
Patch

Alexey, can you review the latest patch? I had to make a change in WebKit2 so we can still test the preference API.

[Build Bot]

Comment on attachment 184324 [details]
Patch

Attachment 184324 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16065908

New failing tests:
platform/mac/fast/frames/flattening/set-preference.html

[Build Bot]

Comment on attachment 184324 [details]
Patch

Attachment 184324 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16097048

New failing tests:
platform/mac/fast/frames/flattening/set-preference.html

[Tony Chang]

Created attachment 184551 [details]
Patch

[Hajime Morrita]

Comment on attachment 184551 [details]
Patch

Oops. Tony is asking @ap for review. I'd cancel my r+.

[Build Bot]

Comment on attachment 184551 [details]
Patch

Attachment 184551 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16118722

New failing tests:
fast/frames/flattening/frameset-flattening-advanced.html
compositing/iframes/scrolling-iframe.html
compositing/iframes/iframe-resize.html
compositing/iframes/nested-iframe-scrolling.html
compositing/iframes/connect-compositing-iframe2.html
css2.1/20110323/absolute-replaced-height-014.htm
compositing/iframes/iframe-in-composited-layer.html
editing/input/scroll-viewport-page-up-down.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
css3/flexbox/flexitem.html
compositing/iframes/overlapped-iframe-iframe.html
fast/forms/basic-textareas.html
compositing/iframes/overlapped-iframe.html
compositing/visible-rect/iframe-and-layers.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/page-cache-layer-tree.html
css2.1/20110323/absolute-replaced-height-035.htm
css2.1/20110323/absolute-replaced-height-007.htm
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-copy-on-scroll.html
compositing/visible-rect/iframe-no-layers.html
css2.1/20110323/absolute-replaced-height-028.htm
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/iframes/resizer.html
editing/input/option-page-up-down.html
css2.1/20110323/absolute-replaced-height-021.htm

[Tony Chang]

Created attachment 185013 [details]
Patch

[Tony Chang]

Can I get OWNERS approval for the change to WebKit2?

[WebKit Review Bot]

Comment on attachment 185013 [details]
Patch

Clearing flags on attachment: 185013

Committed r142499: <http://trac.webkit.org/changeset/142499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.