Bug 9221 – resize property doesn't work on iframes

• Component: Forms
• Status: RESOLVED FIXED

[Adele Peterson]

resize property doesn't work on iframes

<iframe style="overflow:auto; resize:both" src="about:blank"></iframe>

[Christian Biesinger]

Created attachment 183277 [details]
Patch

[Christian Biesinger]

Comment on attachment 183277 [details]
Patch

clearing review flag because there's a couple small things I need to address first...

[Christian Biesinger]

Created attachment 184113 [details]
Patch

[Christian Biesinger]

OK, I think this is ready for review now.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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. :)

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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.

[Simon Fraser (smfr)]

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 ?

[Levi Weintraub]

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.

[Christian Biesinger]

I was wondering that too. You're probably right and I should move that check into canResize(), so I'll do that.

[Christian Biesinger]

Created attachment 184291 [details]
Patch

[WebKit Review Bot]

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.

[Christian Biesinger]

(In reply to comment #13)
> Attachment 184291 [details] did not pass style-queue:

filed bug 107724 about that error

[Eric Seidel (no email)]

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?

[Christian Biesinger]

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)

[Eric Seidel (no email)]

(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.

[Christian Biesinger]

Created attachment 184369 [details]
Patch

[Christian Biesinger]

Created attachment 184372 [details]
Patch

*sigh*, let's try again with --binary

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

What are you passing --binary to?  webkit-patch upload should do all the magic for you?

[Christian Biesinger]

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.

[Eric Seidel (no email)]

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. :-)

[Eric Seidel (no email)]

Comment on attachment 184372 [details]
Patch

Lgtm

[Ojan Vafai]

Yay! <3 this feature.

[WebKit Review Bot]

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

[Levi Weintraub]

Comment on attachment 184372 [details]
Patch

Hard to say what exactly went wrong here.. Giving it another kick and hoping the patch still applies.

[WebKit Review Bot]

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

[Christian Biesinger]

Created attachment 184582 [details]
Patch

[Christian Biesinger]

Comment on attachment 184582 [details]
Patch

Looks like there was a merge conflict in TestExpectations. Fixed now.

[WebKit Review Bot]

Comment on attachment 184582 [details]
Patch

Clearing flags on attachment: 184582

Committed r140749: <http://trac.webkit.org/changeset/140749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.