Implement setFrameFlatteningEnabled in window.internals and remove duplicated code from WebKitTestRunner and DumpRenderTree.
Created attachment 143355 [details] Patch
(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.
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.
Comment on attachment 143355 [details] Patch Attachment 143355 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12767011
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.
Thanks for the reviews, I will upload a new patch soon.
Created attachment 143569 [details] Patch
Comment on attachment 143569 [details] Patch Attachment 143569 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12772246
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.
Created attachment 143599 [details] Patch
(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?
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.
Created attachment 144117 [details] Patch
Created attachment 144607 [details] Patch
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
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.
(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!
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.
Created attachment 183520 [details] Patch
I rewrote this patch now that bug 87993 is resolved.
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.
Created attachment 184003 [details] Patch for landing
Comment on attachment 184003 [details] Patch for landing Actually, let me see if I can add a TestWebKitAPI test for this on Mac.
Created attachment 184323 [details] Patch for landing
Created attachment 184324 [details] Patch
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.
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
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
Created attachment 184551 [details] Patch
Comment on attachment 184551 [details] Patch Oops. Tony is asking @ap for review. I'd cancel my r+.
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
Created attachment 185013 [details] Patch
Can I get OWNERS approval for the change to WebKit2?
Comment on attachment 185013 [details] Patch Clearing flags on attachment: 185013 Committed r142499: <http://trac.webkit.org/changeset/142499>
All reviewed patches have been landed. Closing bug.