Summary: | Refactoring: use PlatformBridge to set scroll position in Android scroll view. | ||
---|---|---|---|
Product: | WebKit | Reporter: | sw <swang> |
Component: | History | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | andreip, benm, commit-queue, steveblock, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | OS X 10.5 | ||
Attachments: |
Description
sw
2010-11-15 10:47:37 PST
Created attachment 73917 [details]
Fix android specific history bug.
Make sure only main frame view's position is restored.
who should review my change? > 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. 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.
hi, Andrei/Ben/Steve, Could you review this again? Thanks Simon 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. 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.
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. Created attachment 74680 [details]
Use platform bridge to set scroll position. this fixes layering violation.
Refactoring to prevent layering violation.
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. Created attachment 74683 [details]
Use platform bridge to set scroll position. this fixes layering violation.
Refactoring only.
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 Created attachment 74684 [details]
Use platform bridge to set scroll position. this fixes layering violation.
Refactoring only. Thanks Steve for reviewing.
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.
Created attachment 74685 [details]
Use platform bridge to set scroll position. this fixes layering violation.
Refactoring only.
Comment on attachment 74685 [details]
Use platform bridge to set scroll position. this fixes layering violation.
r=me
thanks a lot Steve for all the review and help. 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> All reviewed patches have been landed. Closing bug. |