Bug 77452 – [chromium] Add the ability to turn off autoresize.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Levin]

[chromium] Add the ability to turn off autoresize.

[David Levin]

Created attachment 124770 [details]
Patch

[WebKit Review Bot]

Attachment 124770 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Created attachment 124775 [details]
Patch

[WebKit Review Bot]

Attachment 124775 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Titov]

Comment on attachment 124775 [details]
Patch

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

Couple of questions (perhaps just needs clarification):

> Source/WebKit/chromium/src/WebViewImpl.cpp:2120
> +    ASSERT(!m_shouldAutoResize || minSize == maxSize);

I would understand:
ASSERT(enable || minSize == maxSize);
.. because it would check incoming parameters for consistency.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2131
> +        resize(m_minAutoSize);

It would be good to add a comment describing the mechanism of turning off auto-resize, to clarify why the 'new size' parameter and a resize to it is needed.
Otherwise, first look at API brings a question, why not have:

enableAutoSize(minSize, maxSize)
disableAutoSize() // w/o prameters

since when auto-resize if off, the layout should be seeded with the current window size...

[David Levin]

Created attachment 129146 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 129146 [details]
Patch

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

New failing tests:
fast/autoresize/turn-off-autoresize.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 129146 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:261
> +    // content within the given bounds. When turning off auto-resize, the

why require this?  it seems more sensible for this function to ignore min/maxSize
when disabling auto-resize mode as that is telling the WebView to do nothing special.
doesn't the WebView already know its size?

[David Levin]

Created attachment 129336 [details]
Patch

[David Levin]

(In reply to comment #9)
> (From update of attachment 129146 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129146&action=review
> 
> > Source/WebKit/chromium/public/WebView.h:261
> > +    // content within the given bounds. When turning off auto-resize, the
> 
> why require this?  it seems more sensible for this function to ignore min/maxSize
> when disabling auto-resize mode as that is telling the WebView to do nothing special.
> doesn't the WebView already know its size?

When autoresize is off initially, all sizing is driven by the RenderViewHost.

When autoresize is on, all sizing is driven by the WebView.

When autoresize is turned off, there may be resize changes in flight from the WebView to the RenderViewHost, so they may be out of sync. By setting the size on the WebView, we start the process of having the RenderViewHost drive the sizing again (and this size change will propagate back to RenderWidgetHostImpl::OnMsgUpdateRect as needed so that everything gets in sync).

I hope that makes sense. (Let me know if I should improve the comment where I tried to explain this.)

(The latest patch only changes the description text in the test to match changes done to the html so now it will pass.)

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 129146 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129146&action=review
> > 
> > > Source/WebKit/chromium/public/WebView.h:261
> > > +    // content within the given bounds. When turning off auto-resize, the
> > 
> > why require this?  it seems more sensible for this function to ignore min/maxSize
> > when disabling auto-resize mode as that is telling the WebView to do nothing special.
> > doesn't the WebView already know its size?
> 
> When autoresize is off initially, all sizing is driven by the RenderViewHost.
> 
> When autoresize is on, all sizing is driven by the WebView.
> 
> When autoresize is turned off, there may be resize changes in flight from the WebView to the RenderViewHost, so they may be out of sync. By setting the size on the WebView, we start the process of having the RenderViewHost drive the sizing again (and this size change will propagate back to RenderWidgetHostImpl::OnMsgUpdateRect as needed so that everything gets in sync).
> 
> I hope that makes sense. (Let me know if I should improve the comment where I tried to explain this.)
> 
> (The latest patch only changes the description text in the test to match changes done to the html so now it will pass.)

Would an alternative be for the embedder to call WebWidget::resize() after disabling auto-size mode?

[David Levin]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (From update of attachment 129146 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=129146&action=review
> > > 
> > > > Source/WebKit/chromium/public/WebView.h:261
> > > > +    // content within the given bounds. When turning off auto-resize, the
> > > 
> > > why require this?  it seems more sensible for this function to ignore min/maxSize
> > > when disabling auto-resize mode as that is telling the WebView to do nothing special.
> > > doesn't the WebView already know its size?
> > 
> > When autoresize is off initially, all sizing is driven by the RenderViewHost.
> > 
> > When autoresize is on, all sizing is driven by the WebView.
> > 
> > When autoresize is turned off, there may be resize changes in flight from the WebView to the RenderViewHost, so they may be out of sync. By setting the size on the WebView, we start the process of having the RenderViewHost drive the sizing again (and this size change will propagate back to RenderWidgetHostImpl::OnMsgUpdateRect as needed so that everything gets in sync).
> > 
> > I hope that makes sense. (Let me know if I should improve the comment where I tried to explain this.)
> > 
> > (The latest patch only changes the description text in the test to match changes done to the html so now it will pass.)
> 
> Would an alternative be for the embedder to call WebWidget::resize() after disabling auto-size mode?

They could do that but if they didn't, who knows what would happen.

I could also separate this api into enable and disable. Get rid of the bool and make disable take "new_size".

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> I could also separate this api into enable and disable. Get rid of the bool and make disable take "new_size".

That sounds pretty good to me.  I know it means more busy work though :-(

[David Levin]

Created attachment 129393 [details]
Patch

[David Levin]

(In reply to comment #14)
> (In reply to comment #13)
> > I could also separate this api into enable and disable. Get rid of the bool and make disable take "new_size".
> 
> That sounds pretty good to me.  I know it means more busy work though :-(

All done.

I though about the resize being combined with disabling auto-resize. It really only happens due to the fact that chromium has multiple processes but the WebKit api has no notion of that, so I moved the resize to the chromium side of things and out of the WebKit api (which can be seen here https://chromiumcodereview.appspot.com/9517010/).

[David Levin]

Committed as http://trac.webkit.org/changeset/109288

[Kentaro Hara]

Comment on attachment 129393 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:2285
> +        enableAutoResizeMode(enable, minSize, maxSize);

webkit_unit_tests start to fail. I guess the failure is due to this line. The line should be

    enableAutoResizeMode(minSize, maxSize);

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 129675 [details]
Patch

[Hajime Morrita]

Comment on attachment 129675 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 129675 [details]
Patch

Rejecting attachment 129675 [details] from commit-queue.

New failing tests:
fast/workers/storage/use-same-database-in-page-and-workers.html
Full output: http://queues.webkit.org/results/11770436

[David Levin]

(In reply to comment #18)
> (From update of attachment 129393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129393&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2285
> > +        enableAutoResizeMode(enable, minSize, maxSize);
> 
> webkit_unit_tests start to fail. I guess the failure is due to this line. The line should be
> 
>     enableAutoResizeMode(minSize, maxSize);

Wow. Thanks for figuring that out. (You could have just rolled it out too and left it for me.)

[WebKit Review Bot]

Comment on attachment 129675 [details]
Patch

Clearing flags on attachment: 129675

Committed r109350: <http://trac.webkit.org/changeset/109350>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.