Bug 87149

Summary: Move setFrameFlatteningEnabled from layoutTestController to window.internals.settings
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: WebKit2Assignee: Jesus Sanchez-Palencia <jesus>
Status: RESOLVED FIXED    
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
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87993    
Bug Blocks: 87284    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch
none
Patch
none
Patch none

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 1 Jesus Sanchez-Palencia 2012-05-22 13:45:40 PDT
Created attachment 143355 [details]
Patch
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 7 Jesus Sanchez-Palencia 2012-05-23 07:47:24 PDT
Created attachment 143569 [details]
Patch
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 10 Jesus Sanchez-Palencia 2012-05-23 11:10:41 PDT
Created attachment 143599 [details]
Patch
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 13 Jesus Sanchez-Palencia 2012-05-25 11:53:55 PDT
Created attachment 144117 [details]
Patch
Comment 14 Jesus Sanchez-Palencia 2012-05-29 13:20:36 PDT
Created attachment 144607 [details]
Patch
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 19 Tony Chang 2013-01-18 11:35:05 PST
Created attachment 183520 [details]
Patch
Comment 20 Tony Chang 2013-01-18 12:20:10 PST
I rewrote this patch now that bug 87993 is 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 22 Tony Chang 2013-01-22 10:20:02 PST
Created attachment 184003 [details]
Patch for landing
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 24 Tony Chang 2013-01-23 15:02:32 PST
Created attachment 184323 [details]
Patch for landing
Comment 25 Tony Chang 2013-01-23 15:07:50 PST
Created attachment 184324 [details]
Patch
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 29 Tony Chang 2013-01-24 12:15:26 PST
Created attachment 184551 [details]
Patch
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 32 Tony Chang 2013-01-28 11:10:45 PST
Created attachment 185013 [details]
Patch
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.