|Summary:||Move setFrameFlatteningEnabled from layoutTestController to window.internals.settings|
|Product:||WebKit||Reporter:||Jesus Sanchez-Palencia <jesus>|
|Component:||WebKit2||Assignee:||Jesus Sanchez-Palencia <jesus>|
|Severity:||Normal||CC:||ap, buildbot, cmarcelo, ddkilzer, gns, gyuyoung.kim, joepeck, menard, mifenton, morrita, philn, rafael.lobo, rakuco, rniwa, rwlbuis, simon.fraser, tonikitoo, tony, webkit.review.bot, xan.lopez, yong.li.webkit|
|Version:||528+ (Nightly build)|
|Bug Depends on:||87993|
Description Jesus Sanchez-Palencia 2012-05-22 11:20:57 PDT
Implement setFrameFlatteningEnabled in window.internals and remove duplicated code from WebKitTestRunner and DumpRenderTree.
Comment 2 Jesus Sanchez-Palencia 2012-05-22 13:58:03 PDT
(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 3 Caio Marcelo de Oliveira Filho 2012-05-22 14:10:09 PDT
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 4 Build Bot 2012-05-22 14:21:08 PDT
Comment on attachment 143355 [details] Patch Attachment 143355 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12767011
Comment 5 Gyuyoung Kim 2012-05-22 18:15:48 PDT
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.
Comment 6 Jesus Sanchez-Palencia 2012-05-23 06:59:00 PDT
Thanks for the reviews, I will upload a new patch soon.
Comment 8 Gustavo Noronha (kov) 2012-05-23 09:33:31 PDT
Comment on attachment 143569 [details] Patch Attachment 143569 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12772246
Comment 9 Alexey Proskuryakov 2012-05-23 10:47:08 PDT
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.
Comment 11 Jesus Sanchez-Palencia 2012-05-24 08:19:39 PDT
(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 12 Eric Seidel (no email) 2012-05-24 16:44:00 PDT
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.
Comment 15 Ojan Vafai 2012-05-31 14:29:20 PDT
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 16 Alexey Proskuryakov 2012-05-31 14:47:55 PDT
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.
Comment 17 Jesus Sanchez-Palencia 2012-06-04 05:59:42 PDT
(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!
Comment 18 Alexey Proskuryakov 2012-06-04 08:45:55 PDT
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.
Comment 21 Hajime Morrita 2013-01-20 17:45:43 PST
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.
Comment 23 Tony Chang 2013-01-22 10:21:44 PST
Comment on attachment 184003 [details] Patch for landing Actually, let me see if I can add a TestWebKitAPI test for this on Mac.
Comment 26 Tony Chang 2013-01-23 15:10:05 PST
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 27 Build Bot 2013-01-23 22:44:35 PST
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 28 Build Bot 2013-01-24 00:12:37 PST
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
Comment 30 Hajime Morrita 2013-01-24 17:07:35 PST
Comment on attachment 184551 [details] Patch Oops. Tony is asking @ap for review. I'd cancel my r+.
Comment 31 Build Bot 2013-01-26 00:23:50 PST
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
Comment 33 Tony Chang 2013-02-11 10:35:51 PST
Can I get OWNERS approval for the change to WebKit2?
Comment 34 WebKit Review Bot 2013-02-11 13:06:30 PST
Comment on attachment 185013 [details] Patch Clearing flags on attachment: 185013 Committed r142499: <http://trac.webkit.org/changeset/142499>
Comment 35 WebKit Review Bot 2013-02-11 13:06:38 PST
All reviewed patches have been landed. Closing bug.