Bug 68098 – ValueAuto and ValueMediumDPI are the same for all platforms.

• Status: UNCONFIRMED
• Component: Layout and Rendering

[Konrad Piascik]

Created attachment 107365 [details]
Add default parameter to override the ValueAuto DPI.

Allow platforms to specify their own default targetDensityDPI by adding a default parameter to the computeViewportAttributes method.

[Kenneth Rohde Christiansen]

Comment on attachment 107365 [details]
Add default parameter to override the ValueAuto DPI.

No changelog, no explanation why this is a good chance etc, thus r-

[Konrad Piascik]

Created attachment 107521 [details]
Updated patch with ChangeLog, explanation

[Antonio Gomes]

Comment on attachment 107521 [details]
Updated patch with ChangeLog, explanation

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

> Source/WebCore/dom/ViewportArguments.cpp:63
>          args.targetDensityDpi = 120;
>          break;
>      case ViewportArguments::ValueAuto:
> +        args.targetDensityDpi = autoDPI;
> +        break;
>      case ViewportArguments::ValueMediumDPI:

Should we really default-value it to 160, unlike the other parameters? Is it only to not update the call sites?

[Konrad Piascik]

(In reply to comment #3)
> (From update of attachment 107521 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107521&action=review
> 
> > Source/WebCore/dom/ViewportArguments.cpp:63
> >          args.targetDensityDpi = 120;
> >          break;
> >      case ViewportArguments::ValueAuto:
> > +        args.targetDensityDpi = autoDPI;
> > +        break;
> >      case ViewportArguments::ValueMediumDPI:
> 
> Should we really default-value it to 160, unlike the other parameters? Is it only to not update the call sites?

That's my motivation for the default value.  If it's preferred I can update the call sites.

[Kenneth Rohde Christiansen]

Comment on attachment 107521 [details]
Updated patch with ChangeLog, explanation

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

> Source/WebCore/ChangeLog:8
> +        Not all platforms should have the same default value for the
> +        targetDensityDPI. Added an optional parameter to allow
> +        platforms to override the default auto value.

The default value is set to 160 for a reason. The reason being that that is what the original iOS used. Android even went as far as specifying what they call -density independent pixels- DIPS, which are specified as pixels at 160 DPI.

Our devices (N9, N950) have a default DPI of 240 and we do not need this modification.

Changing to another default is just going to break all pages designed with 160 in mind (most mobile pages), so I do not see how this is going to help with anything. 

So so far I am not convinced this is the right thing to do.

[Konrad Piascik]

(In reply to comment #5)
> (From update of attachment 107521 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107521&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Not all platforms should have the same default value for the
> > +        targetDensityDPI. Added an optional parameter to allow
> > +        platforms to override the default auto value.
> 
> The default value is set to 160 for a reason. The reason being that that is what the original iOS used. Android even went as far as specifying what they call -density independent pixels- DIPS, which are specified as pixels at 160 DPI.
> 
> Our devices (N9, N950) have a default DPI of 240 and we do not need this modification.
> 
> Changing to another default is just going to break all pages designed with 160 in mind (most mobile pages), so I do not see how this is going to help with anything. 
> 
> So so far I am not convinced this is the right thing to do.

Most mobile pages don't know or care about target density.  They code to a layout width, which is 320px for most sites.

In order for a device to display 320px at a density of 160DPI then that device must be at least 2 inches wide(screen width).  computeViewportAttributes will calculate a layout width smaller than 320px if a device's screen width is less than 2 inches.

This change allows each platform/device to specify its own default if needed.

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 107521 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107521&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Not all platforms should have the same default value for the
> > > +        targetDensityDPI. Added an optional parameter to allow
> > > +        platforms to override the default auto value.
> > 
> > The default value is set to 160 for a reason. The reason being that that is what the original iOS used. Android even went as far as specifying what they call -density independent pixels- DIPS, which are specified as pixels at 160 DPI.
> > 
> > Our devices (N9, N950) have a default DPI of 240 and we do not need this modification.
> > 
> > Changing to another default is just going to break all pages designed with 160 in mind (most mobile pages), so I do not see how this is going to help with anything. 
> > 
> > So so far I am not convinced this is the right thing to do.
> 
> Most mobile pages don't know or care about target density.  They code to a layout width, which is 320px for most sites.
> 
> In order for a device to display 320px at a density of 160DPI then that device must be at least 2 inches wide(screen width).  computeViewportAttributes will calculate a layout width smaller than 320px if a device's screen width is less than 2 inches.
> 
> This change allows each platform/device to specify its own default if needed.

This would be good to add as a comment in the code and in the ChangeLog. This explains a lot.

I don't like the argument name much, what about something like maximumDPI instead? Would something like that be possible?

[Kenneth Rohde Christiansen]

> I don't like the argument name much, what about something like maximumDPI instead? Would something like that be possible?

Or change the algorithm to bound the DPI by the deviceDPI? Wouldn't that make more sense?

[Konrad Piascik]

(In reply to comment #8)
> > I don't like the argument name much, what about something like maximumDPI instead? Would something like that be possible?
> 
> Or change the algorithm to bound the DPI by the deviceDPI? Wouldn't that make more sense?

The change that I'm proposing is to override the default value of 160 to be device/platform configurable.  We already have bounds on the web page specified DPI.

If a web developer wants to specify a target DPI different than the default then they should be able to do so, within the already defined bounds.  This would allow the web developer to have more control, assuming they know what they're doing, and still give a device/platform the ability to better control how things DPI correction works in the default case.

[Konrad Piascik]

Created attachment 107846 [details]
Updated ChangeLog and variable name

[Kenneth Rohde Christiansen]

Comment on attachment 107846 [details]
Updated ChangeLog and variable name

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

> Source/WebCore/ChangeLog:6
> +        320px is the layout widht which most mobile sites use as a

*width

> Source/WebCore/ChangeLog:13
> +        mobile web pages.  Allowing a platform specific default
> +        value to be set for ValueAuto of targetDensityDPI helps deal
> +        with this case.

Would be great if you made it clear how it helps. Maybe adding some comments to computeViewportAttributes in the header file. This is done elsewhere in WebKit

> Source/WebCore/ChangeLog:17
> +        No new tests needed, since it causes no behavioral change.

It would be good for tests emulating a screen less than 2 inches so we can all get the same behavior.

[Kenneth Rohde Christiansen]

Antonio can you help Konrad with this? The problem is said, but it is a bit vage how it is dealt with. I also believe that it should be possible to do some tests and add a bit of api documentation. Also knowing how this is dealt with on most current devices; blackberry, android with screens less than 2 inches would be good info.

[Eric Seidel (no email)]

Comment on attachment 107846 [details]
Updated ChangeLog and variable name

Can we test this?

[Konrad Piascik]

(In reply to comment #13)
> (From update of attachment 107846 [details])
> Can we test this?

We currently test all output from the computeViewportAttributes method except the devicePixelRatio.  Adjusting the AutoDPI value would affect the devicePixelRatio.  This property should be accessible through javascript via window.devicePixelRatio, however I'm not sure if all ports set this value to reflect the output of computeViewporAttributes.

[Anders Carlsson]

Comment on attachment 107846 [details]
Updated ChangeLog and variable name

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.