Bug 49550 – Refactoring: use PlatformBridge to set scroll position in Android scroll view.

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[sw]

Fix android specific history bug

This issue is related to how android implements view position restoration.  The fix is to only send main view's restoration to android specific code.

[sw]

Created attachment 73917 [details]
Fix android specific history bug.

Make sure only main frame view's position is restored.

[sw]

who should review my change?

[Andrei Popescu]

> No new tests. (OOPS!)

Please remove this and explain why you don't add a test.

> // Sometimes the parent scrollView is not set for some iFrame when it's first loaded
>// in case of history going back.

Just curious: can you please provide more info about the circumstances when the parent is not set?

> android::WebViewCore *webViewCore = android::WebViewCore::getWebViewCore(this);

There should not be Android namespace code in WebCore, this is a layering violation. You should use the PlatformBridge instead. I know you haven't introduced this yourself in this patch but it'd be good to think how to address the layering violations in this file.

[sw]

Created attachment 74011 [details]
Fix android specific history bug.

Thanks Andrei for review.  I update the patch; and will think about how to remove the android namespace.

[sw]

hi, Andrei/Ben/Steve,

Could you review this again?

Thanks

Simon

[Steve Block]

Comment on attachment 74011 [details]
Fix android specific history bug.

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

> WebCore/platform/android/ScrollViewAndroid.cpp:75
> +    // engadget.com site, you go to some article then hit back.

I think the right place for these kind of details are in the bug, not in comments, and it would be good to understand the underlying problem, rather than just listing a test case.

> WebCore/platform/android/ScrollViewAndroid.cpp:77
> +    android::WebViewCore *webViewCore = android::WebViewCore::getWebViewCore(this);

Layering violation, as Andrei says.

[sw]

Created attachment 74598 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

Use platform bridge to set scroll position.  this fixes layering violation.  since PlatformBridge.cpp is in webkit and is not upstreamed, so this patch doesn't include that file.  Also it seems the platformBridge.h and scrollviewAndroid.cpp are very outdated.

[Steve Block]

Comment on attachment 74598 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

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

> WebCore/ChangeLog:5
> +        Use platform bridge to set scroll position.

You should update the bug title to match this updated description.

> WebCore/ChangeLog:9
> +        test introduced.

The patch no longer has anything to do with browsing history. In any case, LayoutTests are to test platform-independent functionality. This is a change to the Android implementation, so 'Refactoring only, tested by existing tests' suffices.

> WebCore/platform/android/PlatformBridge.h:109
> +    static void setScrollPosition(ScrollView*, int, int);

you should name the two int params, as it's not clear from their types what they are.

[sw]

Created attachment 74680 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

Refactoring to prevent layering violation.

[Steve Block]

Comment on attachment 74680 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

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

> WebCore/ChangeLog:11
> +        (WebCore::ScrollView::platformSetScrollPosition):

PlatformBridge.h seems to be missing? You should probably re-run prepare-ChangeLog.

> WebCore/platform/android/PlatformBridge.h:109
> +    static void setScrollPosition(ScrollView* scrollView, int x, int y);

Don't need to name 'scrollView' as it's obvious from its type.

[sw]

Created attachment 74683 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

Refactoring only.

[Steve Block]

Comment on attachment 74683 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

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

Do you want cq too? You can set cq? to request this.

> WebCore/ChangeLog:5
> +        Use platform bridge to set scroll position.

Need a link to the bug URL

[sw]

Created attachment 74684 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

Refactoring only.  Thanks Steve for reviewing.

[WebKit Review Bot]

Attachment 74684 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/android/PlatformBridge.h', u'WebCore/platform/android/ScrollViewAndroid.cpp']" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[sw]

Created attachment 74685 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

Refactoring only.

[Steve Block]

Comment on attachment 74685 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

r=me

[sw]

thanks a lot Steve for all the review and help.

[WebKit Commit Bot]

Comment on attachment 74685 [details]
Use platform bridge to set scroll position.  this fixes layering violation.

Clearing flags on attachment: 74685

Committed r72631: <http://trac.webkit.org/changeset/72631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.