Bug 9221

Summary: resize property doesn't work on iframes
Product: WebKit Reporter: Adele Peterson <adele>
Component: FormsAssignee: Christian Biesinger <cbiesinger>
Status: RESOLVED FIXED    
Severity: Normal CC: arv, ben, dglazkov, ehsan, eric, gyuyoung.kim, ian, leviw, ojan.autocc, ojan, rakuco, rniwa, simon.fraser, webkit.review.bot
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Adele Peterson 2006-06-01 17:09:04 PDT 
resize property doesn't work on iframes

<iframe style="overflow:auto; resize:both" src="about:blank"></iframe>
Comment 1 Christian Biesinger 2013-01-17 14:21:21 PST 
Created attachment 183277 [details]
Patch
Comment 2 Christian Biesinger 2013-01-17 14:27:11 PST 
Comment on attachment 183277 [details]
Patch

clearing review flag because there's a couple small things I need to address first...
Comment 3 Christian Biesinger 2013-01-22 20:06:15 PST 
Created attachment 184113 [details]
Patch
Comment 4 Christian Biesinger 2013-01-22 20:06:44 PST 
OK, I think this is ready for review now.
Comment 5 WebKit Review Bot 2013-01-22 20:08:41 PST 
Attachment 184113 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderIFrame.cpp:88:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
LayoutTests/platform/chromium-mac/fast/css/resize-corner-tracking-transformed-iframe-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Eric Seidel (no email) 2013-01-22 20:42:16 PST 
Comment on attachment 184113 [details]
Patch

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

This looks great.  I will need to read it again when I have less wine in my system. :)

> Source/WebCore/rendering/RenderLayer.cpp:2322
> +    return renderer()
> +        && (renderer()->hasOverflowClip() || renderer()->isRenderIFrame());

We don't wrap to 80c in webkit.  We wrap for readability. :)
Comment 7 WebKit Review Bot 2013-01-22 20:56:59 PST 
Comment on attachment 184113 [details]
Patch

Attachment 184113 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16045949

New failing tests:
fast/css/resize-corner-tracking-transformed-iframe.html
fast/css/resize-corner-tracking.html
Comment 8 Eric Seidel (no email) 2013-01-22 22:07:41 PST 
Comment on attachment 184113 [details]
Patch

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

Looks like we need another round for style and for the 2 tests it fails.

> Source/WebCore/rendering/RenderBlock.cpp:2770
> +    const PaintPhase phase = paintInfo.phase;

We don't use const like this.  const& sure, but we don't generally use const for values.
Comment 9 Simon Fraser (smfr) 2013-01-23 10:00:32 PST 
Comment on attachment 184113 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:2322
>> +    return renderer()
>> +        && (renderer()->hasOverflowClip() || renderer()->isRenderIFrame());
> 
> We don't wrap to 80c in webkit.  We wrap for readability. :)

Why does't this check renderer()->style()->resize() != RESIZE_NONE ?
Comment 10 Levi Weintraub 2013-01-23 10:08:06 PST 
Comment on attachment 184113 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2328
> -    if (!inResizeMode() || !renderer()->hasOverflowClip() || !renderer()->node())
> +    if (!inResizeMode() || !canResize() || !renderer()->node())

Why doesn't this check RESIZE_NONE? It's the only case where we check canResize but not that. It doesn't seem like we should be resizing the layer if that style is set, and it would allow us to move the check into canResize.
Comment 11 Christian Biesinger 2013-01-23 10:39:02 PST 
I was wondering that too. You're probably right and I should move that check into canResize(), so I'll do that.
Comment 12 Christian Biesinger 2013-01-23 13:04:18 PST 
Created attachment 184291 [details]
Patch
Comment 13 WebKit Review Bot 2013-01-23 13:07:18 PST 
Attachment 184291 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/resize-corner-tracking-transformed-iframe.html', u'LayoutTests/fast/css/resize-corner-tracking.html', u'LayoutTests/platform/chromium-linux/fast/css/resize-corner-tracking-expected.png', u'LayoutTests/platform/chromium-linux/fast/css/resize-corner-tracking-transformed-iframe-expected.png', u'LayoutTests/platform/chromium-mac/fast/css/resize-corner-tracking-transformed-iframe-expected.png', u'LayoutTests/platform/chromium-win/fast/css/resize-corner-tracking-expected.txt', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/compositing/iframes/resizer-expected.txt', u'LayoutTests/platform/chromium/fast/css/resize-corner-tracking-expected.txt', u'LayoutTests/platform/chromium/fast/css/resize-corner-tracking-transformed-expected.txt', u'LayoutTests/platform/chromium/fast/css/resize-corner-tracking-transformed-iframe-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/fast/css/resize-corner-tracking-expected.txt', u'LayoutTests/platform/mac/fast/css/resize-corner-tracking-transformed-iframe-expected.png', u'LayoutTests/platform/mac/fast/css/resize-corner-tracking-transformed-iframe-expected.txt', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderIFrame.cpp', u'Source/WebCore/rendering/RenderIFrame.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderWidget.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/css/resize-corner-tracking-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Christian Biesinger 2013-01-23 13:10:42 PST 
(In reply to comment #13)
> Attachment 184291 [details] did not pass style-queue:

filed bug 107724 about that error
Comment 15 Eric Seidel (no email) 2013-01-23 14:47:33 PST 
Comment on attachment 184291 [details]
Patch

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

This looks great.

> Source/WebCore/rendering/RenderLayer.cpp:2323
> +    if (!renderer())
> +        return false;
> +    return (renderer()->hasOverflowClip() || renderer()->isRenderIFrame()) && renderer()->style()->resize() != RESIZE_NONE;

Remind me why iframe is special here?  I guess FrameView's always clip so they just have "implicit" clip and don't set hasOverflowClip?
Comment 16 Christian Biesinger 2013-01-23 14:55:31 PST 
Yes, that was my thinking. Should I add a comment to that effect?


(I also have a local patch to fix the LayoutTests changelog, but I was going to wait with uploading that until I have final review comments and bot results)
Comment 17 Eric Seidel (no email) 2013-01-23 15:01:47 PST 
(In reply to comment #16)
> Yes, that was my thinking. Should I add a comment to that effect?
> 
> 
> (I also have a local patch to fix the LayoutTests changelog, but I was going to wait with uploading that until I have final review comments and bot results)

Comments about "why" are always good.  If you're uploading a new patch anyway that seems appropriate.

Thanks for taking this on.
Comment 18 Christian Biesinger 2013-01-23 17:59:06 PST 
Created attachment 184369 [details]
Patch
Comment 19 Christian Biesinger 2013-01-23 18:12:19 PST 
Created attachment 184372 [details]
Patch

*sigh*, let's try again with --binary
Comment 20 WebKit Review Bot 2013-01-23 18:14:10 PST 
Attachment 184372 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/resize-corner-tracking-transformed-iframe.html', u'LayoutTests/fast/css/resize-corner-tracking.html', u'LayoutTests/platform/chromium-linux/fast/css/resize-corner-tracking-expected.png', u'LayoutTests/platform/chromium-linux/fast/css/resize-corner-tracking-transformed-iframe-expected.png', u'LayoutTests/platform/chromium-mac/fast/css/resize-corner-tracking-transformed-iframe-expected.png', u'LayoutTests/platform/chromium-win/fast/css/resize-corner-tracking-expected.txt', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/compositing/iframes/resizer-expected.txt', u'LayoutTests/platform/chromium/fast/css/resize-corner-tracking-expected.txt', u'LayoutTests/platform/chromium/fast/css/resize-corner-tracking-transformed-expected.txt', u'LayoutTests/platform/chromium/fast/css/resize-corner-tracking-transformed-iframe-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/fast/css/resize-corner-tracking-expected.txt', u'LayoutTests/platform/mac/fast/css/resize-corner-tracking-transformed-iframe-expected.png', u'LayoutTests/platform/mac/fast/css/resize-corner-tracking-transformed-iframe-expected.txt', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderIFrame.cpp', u'Source/WebCore/rendering/RenderIFrame.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderWidget.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/css/resize-corner-tracking-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Eric Seidel (no email) 2013-01-23 18:54:53 PST 
What are you passing --binary to?  webkit-patch upload should do all the magic for you?
Comment 22 Christian Biesinger 2013-01-23 19:07:41 PST 
I didn't use webkit-upload-patch for that last upload... maybe I should have, but it felt simpler to do it manually that time.
Comment 23 Eric Seidel (no email) 2013-01-23 21:47:57 PST 
You might check out
Webkit-patch help -a
To see all the hidden commands. Upload is the all-sing-all-dance, but there may be more advanced/older commands which better fit your favorite workflow. :-)
Comment 24 Eric Seidel (no email) 2013-01-23 21:49:53 PST 
Comment on attachment 184372 [details]
Patch

Lgtm
Comment 25 Ojan Vafai 2013-01-23 22:50:31 PST 
Yay! <3 this feature.
Comment 26 WebKit Review Bot 2013-01-24 11:58:43 PST 
Comment on attachment 184372 [details]
Patch

Rejecting attachment 184372 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 184372, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
s).
Hunk #2 succeeded at 2372 (offset 10 lines).
Hunk #3 succeeded at 2860 (offset 10 lines).
Hunk #4 succeeded at 3274 (offset 10 lines).
Hunk #5 succeeded at 3285 (offset 10 lines).
Hunk #6 succeeded at 4252 (offset 10 lines).
patching file Source/WebCore/rendering/RenderLayer.h
patching file Source/WebCore/rendering/RenderWidget.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16112006
Comment 27 Levi Weintraub 2013-01-24 12:08:04 PST 
Comment on attachment 184372 [details]
Patch

Hard to say what exactly went wrong here.. Giving it another kick and hoping the patch still applies.
Comment 28 WebKit Review Bot 2013-01-24 12:28:04 PST 
Comment on attachment 184372 [details]
Patch

Rejecting attachment 184372 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 184372, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
s).
Hunk #2 succeeded at 2372 (offset 10 lines).
Hunk #3 succeeded at 2860 (offset 10 lines).
Hunk #4 succeeded at 3274 (offset 10 lines).
Hunk #5 succeeded at 3285 (offset 10 lines).
Hunk #6 succeeded at 4252 (offset 10 lines).
patching file Source/WebCore/rendering/RenderLayer.h
patching file Source/WebCore/rendering/RenderWidget.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16082826
Comment 29 Christian Biesinger 2013-01-24 14:46:54 PST 
Created attachment 184582 [details]
Patch
Comment 30 Christian Biesinger 2013-01-24 14:47:51 PST 
Comment on attachment 184582 [details]
Patch

Looks like there was a merge conflict in TestExpectations. Fixed now.
Comment 31 WebKit Review Bot 2013-01-24 16:45:47 PST 
Comment on attachment 184582 [details]
Patch

Clearing flags on attachment: 184582

Committed r140749: <http://trac.webkit.org/changeset/140749>
Comment 32 WebKit Review Bot 2013-01-24 16:45:54 PST 
All reviewed patches have been landed.  Closing bug.