Summary: | [chromium] Changes to consolidate plugin zoom | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Abd-El-Malek <jam> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | John Abd-El-Malek <jam> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | fishd, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||
Attachments: |
|
Description
John Abd-El-Malek
2010-09-24 21:25:32 PDT
Created attachment 68807 [details]
Patch
Attachment 68807 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1570: Place brace on its own line for function definitions. [whitespace/braces] [4]
WebKit/chromium/src/WebViewImpl.cpp:1571: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
WebKit/chromium/src/WebViewImpl.cpp:1574: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
WebKit/chromium/src/WebViewImpl.cpp:1575: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 4 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 68808 [details]
Patch
Created attachment 68957 [details]
Patch
Comment on attachment 68957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68957&action=review Due to WebKit/chromium/public/* changes, the needs Darin's review but I noticed something when I glanced at it. > WebKit/chromium/src/WebViewImpl.cpp:1532 > + if (zoomLevel < m_minimumZoomLevel) { fyi, no {} for single line statements. thanks, fixing now. btw the lint checker didn't catch this. Created attachment 68998 [details]
Patch
Comment on attachment 68998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68998&action=review > WebKit/chromium/public/WebView.h:60 > + static const double textSizeMultiplierRatio; these will break the multi-dll build. you need to use WEBKIT_API to export these. > WebKit/chromium/public/WebView.h:195 > + virtual void fullFramePluginZoomLevelChanged(double zoomLevel) = 0; this function seems pretty awkward and specialized to live on WebView. > WebKit/chromium/public/WebView.h:197 > + // Helper functions to conver between zoom level and zoom factor. zoom conver -> convert > WebKit/chromium/public/WebView.h:199 > + static double zoomLevelToZoomFactor(double zoomLevel); these break the multi-dll build. requires WEBKIT_API tags > WebKit/chromium/public/WebViewClient.h:352 > + // Zoom ---------------------------------------------------------------- add new line below here > WebKit/chromium/public/WebViewClient.h:355 > + virtual void zoomLimitsChanged(double minimumLevel, double maximumLevel) { } add new line below here > WebKit/chromium/src/WebViewImpl.cpp:1566 > m_zoomLevel = zoomLevel; this doesn't respect the min/max zoom levels (In reply to comment #8) > (From update of attachment 68998 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68998&action=review > > > WebKit/chromium/public/WebView.h:60 > > + static const double textSizeMultiplierRatio; > > these will break the multi-dll build. you need to use WEBKIT_API to export these. > > > WebKit/chromium/public/WebView.h:195 > > + virtual void fullFramePluginZoomLevelChanged(double zoomLevel) = 0; > > this function seems pretty awkward and specialized to live on WebView. Any suggestion where to put it? The plugin needs to inform the WebView that the zoom level that it thinks is in use has changed. > > > WebKit/chromium/public/WebView.h:197 > > + // Helper functions to conver between zoom level and zoom factor. zoom > > conver -> convert > > > WebKit/chromium/public/WebView.h:199 > > + static double zoomLevelToZoomFactor(double zoomLevel); > > these break the multi-dll build. requires WEBKIT_API tags > > > WebKit/chromium/public/WebViewClient.h:352 > > + // Zoom ---------------------------------------------------------------- > > add new line below here > > > WebKit/chromium/public/WebViewClient.h:355 > > + virtual void zoomLimitsChanged(double minimumLevel, double maximumLevel) { } > > add new line below here > > > WebKit/chromium/src/WebViewImpl.cpp:1566 > > m_zoomLevel = zoomLevel; > > this doesn't respect the min/max zoom levels Created attachment 69122 [details]
Patch
Created attachment 69261 [details]
Patch
Created attachment 69263 [details]
Patch
Created attachment 69265 [details]
Patch
Darin: the patch has been updated per your suggestion, moving fullFramePluginZoomLevelChanged to WebViewImpl. Comment on attachment 69265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69265&action=review > WebKit/chromium/public/WebPluginContainer.h:82 > + virtual void zoomChanged(double factor) = 0; can this also pass zoom level? maybe name zoomLevelChanged > WebKit/chromium/src/WebPluginContainerImpl.cpp:354 > + nit: extra new line R=me with those changes Created attachment 69372 [details]
Patch
Committed r68869: <http://trac.webkit.org/changeset/68869> Comment on attachment 69372 [details]
Patch
Based on a comment in the bug, this was committed so I'm clearing the r?
|