Bug 87874 – [chromium] Expose device scale factor in WebPluginContainer

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

[Josh Horwich]

[chromium] Expose device scale factor in WebPluginContainer

[Josh Horwich]

Created attachment 144872 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

What's this for?

[Dana Jansens]

Comment on attachment 144872 [details]
Patch

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

> Source/WebKit/chromium/public/WebPluginContainer.h:115
> +    virtual float getDeviceScaleFactor() = 0;

If we're returning the defaultDeviceScaleFactor, let's call this function getDefaultDeviceScaleFactor?

That said I'm not exactly positive which (default?)deviceScaleFactor should be given out here. Josh maybe you can explain how its being used, James will know what's right.

[Josh Horwich]

(In reply to comment #3)
> What's this for?

Context is allowing Pepper to expose the scale factor (between pixels on the display device and "DIPs" aka Density-Independent 'Pixels') to Pepper plugins that wish to render at display device resolution in cases where 1 DIP != 1 physical pixel.

Pepper today sends information to plugins in DIPs so existing plugins, or plugins that don't wish to render in device native resolution for whatever reason, will continue to have their output scaled to the correct size in cases where the scale factor between device pixels and DIPs is not 1.

http://code.google.com/p/chromium/issues/detail?id=114673 is the issue on the chromium tracker that this patch will help in addressing.

(In reply to comment #4)
As for calling it "defaultDeviceScaleFactor" vs "deviceScaleFactor" - I had named it without the 'default' qualifier because I believe Pepper plugins care about the actual scale factor, not whether it is the default or not.

I suppose it gets a bit confusing since, at least in the current implementation, "page zoom" does not result in a change to scale factor in any way, but rather an expansion of the number of pixels (DIPs) a plugin is given - for example if I have a plugin with "width=400" and I zoom the page to "125%" the plugin sees a DidChangeView with a width of 500. This is great for plugins that reasonably handle scaling (e.g. render things in terms of percentages of their view area) as it results in still-crisp output and actually zoomed content. It is less ideal for plugins that use fixed-size (in terms of pixels) assets or fixed locations for rendering as it results in a user-initiated page zoom not generating larger (easier-to-read) plugin contents. This patch or my planned chromium-side patch would not change this behavior - I'm just pointing it out for completeness - e.g. that I want Pepper to have access to the scale factor between device pixels and DIPs, not the page zoom.

[Adam Barth]

Comment on attachment 144872 [details]
Patch

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

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:255
> +float WebPluginContainerImpl::getDeviceScaleFactor()
> +{
> +    Page* page = m_element->document()->page();
> +    if (!page)
> +        return 1.0;
> +    Settings* settings = page->settings();
> +    if (!settings)
> +        return 1.0;
> +    return settings->defaultDeviceScaleFactor();
> +}

Can we hold off on this API until http://lists.webkit.org/pipermail/webkit-dev/2012-May/020847.html gets resolved?  It's likely that this will need to change or at least be renamed.

[Adam Barth]

Comment on attachment 144872 [details]
Patch

Marking r- for now.  Please feel free to renominate once WebKit gets it's scale factors sorted out into something sane.

[Josh Horwich]

(In reply to comment #6)
> (From update of attachment 144872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144872&action=review
> 
> > Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:255
> > +float WebPluginContainerImpl::getDeviceScaleFactor()
> > +{
> > +    Page* page = m_element->document()->page();
> > +    if (!page)
> > +        return 1.0;
> > +    Settings* settings = page->settings();
> > +    if (!settings)
> > +        return 1.0;
> > +    return settings->defaultDeviceScaleFactor();
> > +}
> 
> Can we hold off on this API until http://lists.webkit.org/pipermail/webkit-dev/2012-May/020847.html gets resolved?  It's likely that this will need to change or at least be renamed.

Yes, I think holding off until the dust settles from that discussion would be wise. Thank you for pointing it out to me.

[Josh Horwich]

Created attachment 146096 [details]
Simpler patch that uses page->deviceScaleFactor instead

[James Robinson]

Comment on attachment 146096 [details]
Simpler patch that uses page->deviceScaleFactor instead

What's the notification path for when this value changes?  Should be be push (from WebKit to the plugin), instead of pull (the plugin asks WebKit) ?

[Rick Byers]

Any update on this?  The "dust has settled" for abarth@'s has happened now, right?

+rjkroege/thakis who are working on dynamically changing the scale factor.  We may want to land this without support for dynamic changing first (if it's not ready), and then update.

[James Robinson]

I think we should design an API that supports dynamic switching from the start.  To me the most obvious way to do that is to have the model be push (from embedder to plugin) rather than pull.

[Nico Weber]

rbyers: Dynamic switching for mac is done & checked in (except for composited pages), dynamic switching for aura is done and in review (again except for composited pages).

Might make sense to be similar to the npapi model here https://wiki.mozilla.org/NPAPI:ContentsScaleFactor which is both push and pull.

[Josh Horwich]

(In reply to comment #12)
> I think we should design an API that supports dynamic switching from the start.  To me the most obvious way to do that is to have the model be push (from embedder to plugin) rather than pull.

Typical usage in Pepper for a change in a plugin's view size/clip/origin is for the embedder to push - via calling the plugin instance's DidChangeView with a view resource that has all the new metrics. I believe we would want such a push to happen if the device_scale_factor changes dynamically - I'm not familiar enough yet with dynamic switching to know if this (PluginInstance::ViewChanged, which pushes to the plugin) already happens or not.

Semi-related question, I'm looking to also expose to the plugin the mapping between logical pixels and CSS pixels in the view resource, so a plugin could accurately scale contents/event locations/etc. for cases where page zoom or page scale is not 100% in chromium. Initial testing by using page->pageScaleFactor() showed it stay at 1.0 when zooming. Is there some easy way to get the current logical pixel (DIP) :: CSS pixel mapping from WebPluginContainer?

[Adam Barth]

> Any update on this?  The "dust has settled" for abarth@'s has happened now, right?

Yep.  This wiki page was the outcome:

http://trac.webkit.org/wiki/ScalesAndZooms

@jamesr: Do you think we should post a link to that wiki page to webkit-dev?

[James Robinson]

(In reply to comment #14)
> Semi-related question, I'm looking to also expose to the plugin the mapping between logical pixels and CSS pixels in the view resource, so a plugin could accurately scale contents/event locations/etc. for cases where page zoom or page scale is not 100% in chromium. Initial testing by using page->pageScaleFactor() showed it stay at 1.0 when zooming. Is there some easy way to get the current logical pixel (DIP) :: CSS pixel mapping from WebPluginContainer?

Page zoom does not change the pageScaleFactor, see https://trac.webkit.org/wiki/ScalesAndZooms.  There's a pageZoomFactor() - try that.

[Josh Horwich]

(In reply to comment #16)
> (In reply to comment #14)
> > Semi-related question, I'm looking to also expose to the plugin the mapping between logical pixels and CSS pixels in the view resource, so a plugin could accurately scale contents/event locations/etc. for cases where page zoom or page scale is not 100% in chromium. Initial testing by using page->pageScaleFactor() showed it stay at 1.0 when zooming. Is there some easy way to get the current logical pixel (DIP) :: CSS pixel mapping from WebPluginContainer?
> 
> Page zoom does not change the pageScaleFactor, see https://trac.webkit.org/wiki/ScalesAndZooms.  There's a pageZoomFactor() - try that.

Thank you - pageZoomFactor is in fact letting me understand the mapping between logical & CSS pixels.

[Josh Horwich]

Created attachment 148701 [details]
Similar patch, with additional getPageZoomFactor

[Josh Horwich]

(In reply to comment #11)
> Any update on this?  The "dust has settled" for abarth@'s has happened now, right?
> 
> +rjkroege/thakis who are working on dynamically changing the scale factor.  We may want to land this without support for dynamic changing first (if it's not ready), and then update.

FYI, I've been testing this with dynamic switching (along with the related patch in chromium, https://chromiumcodereview.appspot.com/10544168/ and a HiDPI-aware test plugin), and I've confirmed that when I toggle monitor scale (under ash, using ctrl-shift-home which sends ash::AcceleratorAction::MONITOR_TOGGLE_SCALE), the plugin does get a DidChangeView call, so a HiDPI-aware plugin can in fact react to dynamic changing - assuming other methods of dynamic changing are similar to that.

[Josh Horwich]

Comment on attachment 148701 [details]
Similar patch, with additional getPageZoomFactor

Please take a look

[James Robinson]

Comment on attachment 148701 [details]
Similar patch, with additional getPageZoomFactor

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

> Source/WebKit/chromium/src/WebPluginContainerImpl.h:118
> +    virtual float getDeviceScaleFactor();
> +    virtual float getPageZoomFactor();

style note:  in WebKit, getters do not have the "get" prefix, they are just named after the thing they are getting.

> Source/WebKit/chromium/public/WebPluginContainer.h:37
> +#define WEBPLUGINCONTAINER_HAS_GETDEVICESCALEFACTOR 1
> +#define WEBPLUGINCONTAINER_HAS_GETPAGEZOOMFACTOR 1

personally I'd fold these into one #define

although I'm not sure you need an #define here at all - unless there are implementations of the WebPluginContainer implementation outside of the WebKit repository?

> Source/WebKit/chromium/public/WebPluginContainer.h:117
> +    virtual float getDeviceScaleFactor() = 0;
> +    virtual float getPageZoomFactor() = 0;

same note here re: naming

how does this interact with zoomLevelChanged() ? is the idea with this API that somebody is supposed to call zoomLevelChanged() when the page / device scale change, and if so how does the zoomLevel parameter on zoomLevelChanged() interact with these? or is that call talking about a different concept?

[Josh Horwich]

Comment on attachment 148701 [details]
Similar patch, with additional getPageZoomFactor

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

>> Source/WebKit/chromium/src/WebPluginContainerImpl.h:118
>> +    virtual float getPageZoomFactor();
> 
> style note:  in WebKit, getters do not have the "get" prefix, they are just named after the thing they are getting.

Will fix

>> Source/WebKit/chromium/public/WebPluginContainer.h:37
>> +#define WEBPLUGINCONTAINER_HAS_GETPAGEZOOMFACTOR 1
> 
> personally I'd fold these into one #define
> 
> although I'm not sure you need an #define here at all - unless there are implementations of the WebPluginContainer implementation outside of the WebKit repository?

The goal was to remove patch order dependency between WebKit and chromium - e.g. chromium would have calls to the new methods bracketed in #ifdef WEBPLUGINCONTAINHER_HAS_xxx and assume 1.0 otherwise. Similar to some defines like WEBWIDGET_HAS_SETCOMPOSITORSURFACEREADY in chromium/public/WebWidget.h 

I could remove them altogether and just sequence the patches if you wish (WebKit first, then chromium), please let me know.

>> Source/WebKit/chromium/public/WebPluginContainer.h:117
>> +    virtual float getPageZoomFactor() = 0;
> 
> same note here re: naming
> 
> how does this interact with zoomLevelChanged() ? is the idea with this API that somebody is supposed to call zoomLevelChanged() when the page / device scale change, and if so how does the zoomLevel parameter on zoomLevelChanged() interact with these? or is that call talking about a different concept?

naming: will fix

My intent with adding [get]pageZoomFactor was to make the data available from a PPB_View resource along with the device scale factor. There is no strict need for this now, I thought it simpler to add both at once rather than one-at-a-time in Pepper.    Existing plugins can continue to implement PP_ZoomDev to query and control page zoom - or for those that don't want to implement PP_ZoomDev, but do wish to respect the current page zoom, they can fetch it from PPB_View.

http://codereview.chromium.org/10544168/ has the chromium side of this patch, to provide more context.

Note that I don't have a specific immediate need for this, so I can easily drop it from the patches (and rely on PP_ZoomDev)

[James Robinson]

We normally only put #defines in if there's no way to resolve the compile issues just by patch ordering, so if there's an ordering that can avoid the need please do that.

I don't have any idea what PP_ZoomDev is or how it applies.  Can you please answer the question just in terms of the WebPluginContainer interface (which doesn't know anything about pepper)?  I can't tell from your answer whether the zoomLevelChanged() is related to the page zoom level or not, or if so how.

[Josh Horwich]

(In reply to comment #23)
> We normally only put #defines in if there's no way to resolve the compile issues just by patch ordering, so if there's an ordering that can avoid the need please do that.

Will do - I'll just remove these, and wait on this patch from WebKit before applying my chromium one.

> 
> I don't have any idea what PP_ZoomDev is or how it applies.  Can you please answer the question just in terms of the WebPluginContainer interface (which doesn't know anything about pepper)?  I can't tell from your answer whether the zoomLevelChanged() is related to the page zoom level or not, or if so how.

Sorry, the zoomLevel / zoomFactor stuff is a bit confusing to follow at times. To answer your original questions:
- this doesn't interact with WebPluginContainer::zoomLevelChanged - that method's reason for being is AFAICT to allow a full-frame plugin to control the indicated zoom level, allowing such plugins to surface their own "zoom" UI and keep other (browser) zoom UI in sync. the zoomLevel parameter does not affect the value returned by my API
- the idea of this (pageScaleFactor) API is to allow the plugin to know the page scale factor in order to map between logical pixels and CSS pixels. This API does not require that anyone calls zoomLevelChanged when the page/device scale change

Please let me know if that helps clarify things, and/or if this just makes things more confusing than it already is ... and/or if I should just leave the pageScaleFactor part out of this patch.

[Josh Horwich]

> - the idea of this (pageScaleFactor) API is to allow the plugin to know the page scale factor in order to map between logical pixels and CSS pixels. This API does not require that anyone calls zoomLevelChanged when the page/device scale change

sorry, meant to say "pageZoomFactor" not "pageScaleFactor".

> 
> Please let me know if that helps clarify things, and/or if this just makes things more confusing than it already is ... and/or if I should just leave the pageScaleFactor part out of this patch.

Same here - meant to say "pageZoomFactor" not "pageScaleFactor"

[James Robinson]

OK. Is there a particular reason you are routing pageZoomFactor through and not pageScaleFactor?  They both change the mapping of CSS to logical pixels.

[Josh Horwich]

Created attachment 149089 [details]
Patch to address feedback, add pageScaleFactor

[Josh Horwich]

(In reply to comment #26)
> OK. Is there a particular reason you are routing pageZoomFactor through and not pageScaleFactor?  They both change the mapping of CSS to logical pixels.

I am now also routing pageScaleFactor as well.

(Updated latest patch, addressing earlier review feedback as well)

[James Robinson]

Comment on attachment 149089 [details]
Patch to address feedback, add pageScaleFactor

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

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:269
> +    return frame->pageZoomFactor();

do you know why the pageScale is on the Page, but the pageZoom is on a Frame?  What is this value when inside an iframe?

[Josh Horwich]

Comment on attachment 149089 [details]
Patch to address feedback, add pageScaleFactor

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

>> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:269
>> +    return frame->pageZoomFactor();
> 
> do you know why the pageScale is on the Page, but the pageZoom is on a Frame?  What is this value when inside an iframe?

I'm not familiar with the history as to why the pageZoom isn't available from the Page (just the Frame). I did choose to pull pageScaleFactor from the page rather than frame->frameScaleFactor since frameScaleFactor appears to return the scale factor between 

I ran a test where a plugin was inside an iframe, and when I invoked pageZoom, the value provided for pageZoomFactor (using the frame) accurately reflected the overall page zoom, which I believe is the desired behavior. I do not have a convenient method of testing pageScale however.

[Josh Horwich]

(In reply to comment #30)
> (From update of attachment 149089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149089&action=review
> 
> >> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:269
> >> +    return frame->pageZoomFactor();
> > 
> > do you know why the pageScale is on the Page, but the pageZoom is on a Frame?  What is this value when inside an iframe?
> 
> I'm not familiar with the history as to why the pageZoom isn't available from the Page (just the Frame). I did choose to pull pageScaleFactor from the page rather than frame->frameScaleFactor since frameScaleFactor appears to return the scale factor between 

(sorry for the copy-and-paste error)

... between logical and CSS only for the main frame, otherwise it merely returns 1. See Source/WebCore/page/Frame.cpp)

[Josh Horwich]

Ping :)

I'd like to get this patch (or equivalent) landed soon so I can land the dependent chromium change. I'm also new to webkit so I believe someone else will need to land it for me. Is there anything else that needs addressing on this? Thank you!

[Adam Barth]

Comment on attachment 149089 [details]
Patch to address feedback, add pageScaleFactor

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

This looks fine.  One silly nit below.  Please feel encouraged to upload a new patch with the nit fixed an to set the commit-queue flag to ?.  That requests a committer to mark your patch for commit via the bot.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:268
> +      return 1.0;

Bad indent, should be four spaces.

[Dana Jansens]

(In reply to comment #33)
> (From update of attachment 149089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149089&action=review
> 
> This looks fine.  One silly nit below.  Please feel encouraged to upload a new patch with the nit fixed an to set the commit-queue flag to ?.  That requests a committer to mark your patch for commit via the bot.

Also include Adam's name in the Changelog as reviewer, or you'll need a reviewer to land it. :)

[Josh Horwich]

Created attachment 149777 [details]
Patch for landing

Fixed style nit (indentation), added reviewer to ChangeLog

[WebKit Review Bot]

Comment on attachment 149777 [details]
Patch for landing

Clearing flags on attachment: 149777

Committed r121364: <http://trac.webkit.org/changeset/121364>

[Dana Jansens]

This is landed no? Should be marked fixed then?