Bug 84738 - [BlackBerry] setUserViewportArguments not always respected.
Summary: [BlackBerry] setUserViewportArguments not always respected.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-24 10:18 PDT by Mike Lattanzio
Modified: 2012-04-24 16:46 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2012-04-24 10:26 PDT, Mike Lattanzio
no flags Details | Formatted Diff | Diff
Patch (4.61 KB, patch)
2012-04-24 11:06 PDT, Mike Lattanzio
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lattanzio 2012-04-24 10:18:55 PDT
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.
Comment 1 Mike Lattanzio 2012-04-24 10:26:11 PDT
Created attachment 138592 [details]
Patch
Comment 2 Konrad Piascik 2012-04-24 10:36:47 PDT
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.
Comment 3 Antonio Gomes 2012-04-24 10:37:26 PDT
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?
Comment 4 Konrad Piascik 2012-04-24 10:41:10 PDT
(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.
Comment 5 Antonio Gomes 2012-04-24 10:51:12 PDT
(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?
Comment 6 Konrad Piascik 2012-04-24 10:57:49 PDT
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.
Comment 7 Mike Lattanzio 2012-04-24 11:06:25 PDT
Created attachment 138601 [details]
Patch
Comment 8 WebKit Review Bot 2012-04-24 16:45:53 PDT
Comment on attachment 138601 [details]
Patch

Clearing flags on attachment: 138601

Committed r115135: <http://trac.webkit.org/changeset/115135>
Comment 9 WebKit Review Bot 2012-04-24 16:46:02 PDT
All reviewed patches have been landed.  Closing bug.