Bug 46550 – [chromium] Changes to consolidate plugin zoom

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

[John Abd-El-Malek]

[chromium] Changes to consolidate plugin zoom

[John Abd-El-Malek]

Created attachment 68807 [details]
Patch

[WebKit Review Bot]

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.

[John Abd-El-Malek]

Created attachment 68808 [details]
Patch

[John Abd-El-Malek]

Created attachment 68957 [details]
Patch

[David Levin]

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.

[John Abd-El-Malek]

thanks, fixing now.  btw the lint checker didn't catch this.

[John Abd-El-Malek]

Created attachment 68998 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[John Abd-El-Malek]

(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

[John Abd-El-Malek]

Created attachment 69122 [details]
Patch

[John Abd-El-Malek]

Created attachment 69261 [details]
Patch

[John Abd-El-Malek]

Created attachment 69263 [details]
Patch

[John Abd-El-Malek]

Created attachment 69265 [details]
Patch

[John Abd-El-Malek]

Darin: the patch has been updated per your suggestion, moving fullFramePluginZoomLevelChanged to WebViewImpl.

[Darin Fisher (:fishd, Google)]

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

[John Abd-El-Malek]

Created attachment 69372 [details]
Patch

[John Abd-El-Malek]

Committed r68869: <http://trac.webkit.org/changeset/68869>

[David Levin]

Comment on attachment 69372 [details]
Patch

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