Bug 94123 - [BlackBerry] Reload valid page from Error Page keeps history ViewState and zoom.
Summary: [BlackBerry] Reload valid page from Error Page keeps history ViewState and zoom.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Konrad Piascik
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-15 10:43 PDT by Konrad Piascik
Modified: 2012-08-16 12:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.93 KB, patch)
2012-08-15 12:10 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2012-08-15 12:36 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2012-08-16 09:47 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2012-08-16 11:56 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konrad Piascik 2012-08-15 10:43:13 PDT
[BlackBerry] Reload valid page from Error Page keeps history ViewState and zoom.
Comment 1 Konrad Piascik 2012-08-15 12:10:10 PDT
Created attachment 158613 [details]
Patch
Comment 2 Rob Buis 2012-08-15 12:15:08 PDT
Comment on attachment 158613 [details]
Patch

Looks good.
Comment 3 Konrad Piascik 2012-08-15 12:36:19 PDT
Created attachment 158617 [details]
Patch
Comment 4 Antonio Gomes 2012-08-15 12:55:37 PDT
Comment on attachment 158617 [details]
Patch

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

It looks ok. I have some questions.

Also , patch does not seem to apply against ToT

> Source/WebKit/blackberry/Api/WebPage.cpp:-1101
> -                m_userScalable = m_webSettings->isUserScalable();
> -                resetScales();

could you explain this part in the changelog?

> Source/WebKit/blackberry/Api/WebPage.cpp:1781
> +    if (m_mainFrame && m_mainFrame->loader() && m_mainFrame->loader()->shouldRestoreScrollPositionAndViewState()
> +            && m_mainFrame->loader()->client() && m_mainFrame->loader()->client()->shouldRestoreViewState())

I think you have extra 4-spaces on the bottom line.

> Source/WebKit/blackberry/Api/WebPage.cpp:1905
> +    // This checks to make sure we're not calling updateViewportSize
> +    // during WebPagePrivate::init().
> +    if (m_backingStore)
> +        updateViewportSize();

I would have done this chunk on it own patch.

> Source/WebKit/blackberry/Api/WebPage.cpp:3593
> +    m_userScalable = m_webSettings->isUserScalable();
> +    resetScales();

could you explain this part in the changelog?
Comment 5 Konrad Piascik 2012-08-15 13:02:32 PDT
Comment on attachment 158617 [details]
Patch

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

>> Source/WebKit/blackberry/Api/WebPage.cpp:-1101
>> -                resetScales();
> 
> could you explain this part in the changelog?

This is explained in the ChangeLog.
"Have the minimum, maximum and initial scale reset whenever the
 viewport properties are changed.  We also reset user scalable at this
 time.  We need to do this in the case of the error page since it's
 scales and user scalable would persist if you refresh and get a valid
 page."

>> Source/WebKit/blackberry/Api/WebPage.cpp:1905
>> +        updateViewportSize();
> 
> I would have done this chunk on it own patch.

This was never an issue until I moved the call site of resetScales()

>> Source/WebKit/blackberry/Api/WebPage.cpp:3593
>> +    resetScales();
> 
> could you explain this part in the changelog?

This is explained in the ChangeLog. see above
Comment 6 Konrad Piascik 2012-08-16 09:47:01 PDT
Created attachment 158843 [details]
Patch
Comment 7 Antonio Gomes 2012-08-16 11:19:45 PDT
Comment on attachment 158843 [details]
Patch

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

> Source/WebKit/blackberry/ChangeLog:13
> +        the user scalalbe flag if we're reloading from an error page.

scalable*

> Source/WebKit/blackberry/ChangeLog:24
> +        new memeber variable m_shouldRestoreViewState based on the ViewState's

member

> Source/WebKit/blackberry/Api/WebPage.cpp:1908
> -    updateViewportSize();
> +    // This checks to make sure we're not calling updateViewportSize
> +    // during WebPagePrivate::init().
> +    if (m_backingStore)
> +        updateViewportSize();

lets move this check to within ::updateViewportSize?
Comment 8 Konrad Piascik 2012-08-16 11:56:49 PDT
Created attachment 158869 [details]
Patch

Fixed requested changes
Comment 9 WebKit Review Bot 2012-08-16 12:56:08 PDT
Comment on attachment 158869 [details]
Patch

Clearing flags on attachment: 158869

Committed r125803: <http://trac.webkit.org/changeset/125803>
Comment 10 WebKit Review Bot 2012-08-16 12:56:11 PDT
All reviewed patches have been landed.  Closing bug.