Bug 46550

Summary: [chromium] Changes to consolidate plugin zoom
Product: WebKit Reporter: John Abd-El-Malek <jam>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description John Abd-El-Malek 2010-09-24 21:25:32 PDT
[chromium] Changes to consolidate plugin zoom
Comment 1 John Abd-El-Malek 2010-09-24 21:27:49 PDT
Created attachment 68807 [details]
Patch
Comment 2 WebKit Review Bot 2010-09-24 21:34:15 PDT
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.
Comment 3 John Abd-El-Malek 2010-09-24 21:38:34 PDT
Created attachment 68808 [details]
Patch
Comment 4 John Abd-El-Malek 2010-09-27 14:18:18 PDT
Created attachment 68957 [details]
Patch
Comment 5 David Levin 2010-09-27 16:45:30 PDT
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.
Comment 6 John Abd-El-Malek 2010-09-27 17:30:40 PDT
thanks, fixing now.  btw the lint checker didn't catch this.
Comment 7 John Abd-El-Malek 2010-09-27 17:31:06 PDT
Created attachment 68998 [details]
Patch
Comment 8 Darin Fisher (:fishd, Google) 2010-09-28 14:30:27 PDT
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
Comment 9 John Abd-El-Malek 2010-09-28 15:05:28 PDT
(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
Comment 10 John Abd-El-Malek 2010-09-28 15:56:13 PDT
Created attachment 69122 [details]
Patch
Comment 11 John Abd-El-Malek 2010-09-29 15:36:30 PDT
Created attachment 69261 [details]
Patch
Comment 12 John Abd-El-Malek 2010-09-29 15:41:44 PDT
Created attachment 69263 [details]
Patch
Comment 13 John Abd-El-Malek 2010-09-29 15:51:36 PDT
Created attachment 69265 [details]
Patch
Comment 14 John Abd-El-Malek 2010-09-29 15:52:15 PDT
Darin: the patch has been updated per your suggestion, moving fullFramePluginZoomLevelChanged to WebViewImpl.
Comment 15 Darin Fisher (:fishd, Google) 2010-09-30 13:04:46 PDT
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
Comment 16 John Abd-El-Malek 2010-09-30 13:30:56 PDT
Created attachment 69372 [details]
Patch
Comment 17 John Abd-El-Malek 2010-10-01 00:32:40 PDT
Committed r68869: <http://trac.webkit.org/changeset/68869>
Comment 18 David Levin 2010-10-14 17:12:18 PDT
Comment on attachment 69372 [details]
Patch

Based on a comment in the bug, this was committed so I'm clearing the r?