Bug 84738 – [BlackBerry] setUserViewportArguments not always respected.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Mike Lattanzio]

Not all code paths lead to the fallback viewport arguments being used. Ensure that the fallback ViewportArguments for all loads are used if no meta viewport is present.

[Mike Lattanzio]

Created attachment 138592 [details]
Patch

[Konrad Piascik]

This looks much cleaner.  Since this is actually an implementation of the original bug then I'm inclined to re-open the previous bug, revert the old commit and cleanup this change to be correct.  I'm just not sure if its worth it.

That being said, I'd like to see if there's some way to write a unit test for this.

[Antonio Gomes]

Comment on attachment 138592 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:3158
> -        currentViewportArguments.targetDensityDpi = deviceDPI;
> +        m_viewportArguments.targetDensityDpi = deviceDPI;
>      }

we set 'deviceDPI' here...

> Source/WebKit/blackberry/Api/WebPage.cpp:3160
> -    ViewportAttributes result = computeViewportAttributes(currentViewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);
> +    ViewportAttributes result = computeViewportAttributes(m_viewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);

... and pass it as a parameter here. Is it really needed?

[Konrad Piascik]

(In reply to comment #3)
> (From update of attachment 138592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138592&action=review
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:3158
> > -        currentViewportArguments.targetDensityDpi = deviceDPI;
> > +        m_viewportArguments.targetDensityDpi = deviceDPI;
> >      }
> 
> we set 'deviceDPI' here...
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:3160
> > -    ViewportAttributes result = computeViewportAttributes(currentViewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);
> > +    ViewportAttributes result = computeViewportAttributes(m_viewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);
> 
> ... and pass it as a parameter here. Is it really needed?

Yes this is needed.

[Antonio Gomes]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 138592 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138592&action=review
> > 
> > > Source/WebKit/blackberry/Api/WebPage.cpp:3158
> > > -        currentViewportArguments.targetDensityDpi = deviceDPI;
> > > +        m_viewportArguments.targetDensityDpi = deviceDPI;
> > >      }
> > 
> > we set 'deviceDPI' here...
> > 
> > > Source/WebKit/blackberry/Api/WebPage.cpp:3160
> > > -    ViewportAttributes result = computeViewportAttributes(currentViewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);
> > > +    ViewportAttributes result = computeViewportAttributes(m_viewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);
> > 
> > ... and pass it as a parameter here. Is it really needed?
> 
> Yes this is needed.

Could you explain why?

Why not assign it from within ::computeViewportAttributes?

[Konrad Piascik]

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

>> Source/WebKit/blackberry/Api/WebPage.cpp:3160
>> +    ViewportAttributes result = computeViewportAttributes(m_viewportArguments, desktopWidth, deviceWidth, deviceHeight, deviceDPI, m_defaultLayoutSize);
> 
> ... and pass it as a parameter here. Is it really needed?

We're not changing the DeviceDPI, what we're doing is changing the default targetDensityDPI of the meta viewport arguments.  On the PlayBook the targetDensityDPI if not specified should be the DeviceDPI.  The DeviceDPI passed into the computeViewportAttributes needs to be the correct deviceDPI otherwise there will be no scaling.

[Mike Lattanzio]

Created attachment 138601 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138601 [details]
Patch

Clearing flags on attachment: 138601

Committed r115135: <http://trac.webkit.org/changeset/115135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.