[chromium] Add the ability to turn off autoresize.
Created attachment 124770 [details] Patch
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.
Created attachment 124775 [details] Patch
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.
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...
Created attachment 129146 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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
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?
Created attachment 129336 [details] Patch
(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.)
(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?
(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".
(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 :-(
Created attachment 129393 [details] Patch
(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/).
Committed as http://trac.webkit.org/changeset/109288
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);
Reopening to attach new patch.
Created attachment 129675 [details] Patch
Comment on attachment 129675 [details] Patch r=me
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
(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.)
Comment on attachment 129675 [details] Patch Clearing flags on attachment: 129675 Committed r109350: <http://trac.webkit.org/changeset/109350>
All reviewed patches have been landed. Closing bug.