Bug 31423 - [Android] The Android-specific files in platform/android are out of date
Summary: [Android] The Android-specific files in platform/android are out of date
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-12 11:00 PST by Andrei Popescu
Modified: 2009-11-13 15:39 PST (History)
4 users (show)

See Also:


Attachments
Bring existing files in platform/android in line with Android 2.0 (21.66 KB, patch)
2009-11-12 11:21 PST, Andrei Popescu
dimich: review-
Details | Formatted Diff | Diff
Bring existing files in platform/android in line with Android 2.0 (v2) (22.55 KB, patch)
2009-11-13 10:56 PST, Andrei Popescu
no flags Details | Formatted Diff | Diff
Bring existing files in platform/android in line with Android 2.0 (v2) (22.55 KB, patch)
2009-11-13 11:01 PST, Andrei Popescu
dimich: review-
Details | Formatted Diff | Diff
Bring existing files in platform/android in line with Android 2.0 (v3) (22.54 KB, patch)
2009-11-13 12:21 PST, Andrei Popescu
dimich: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Popescu 2009-11-12 11:00:14 PST
The files in WebCore/platform/android are out of date wrt to Android 2.0. There are also code style problems.  Patch following soon.
Comment 1 Andrei Popescu 2009-11-12 11:21:33 PST
Created attachment 43083 [details]
Bring existing files in platform/android in line with Android 2.0
Comment 2 Dmitry Titov 2009-11-12 13:00:54 PST
Comment on attachment 43083 [details]
Bring existing files in platform/android in line with Android 2.0

Looks good. Couple of style-related comments:


> +PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
> +{
> +    RefPtr<RenderThemeAndroid> androidTheme = new RenderThemeAndroid();
> +    return androidTheme.release();

return adoptRef(new RenderThemeAndroid()) ?
The regular pattern is to have a RenderThemeAndroid::create() - see RenderThemeChromiumWin for example.

> +    return RenderSkinCombo::Draw(getCanvasFromInfo(info), obj->node(), rect.x(),
> +            rect.y(), rect.width(), rect.height());

Usually the lines longer then 80 characters are not wrapped in WebKit sources.

> Index: WebCore/platform/android/ScreenAndroid.cpp
>  
> +#if PLATFORM(ANDROID)

Is this needed? It seems this file is Android-only.

> Index: WebCore/platform/android/ScrollViewAndroid.cpp
> -void ScrollView::platformOffscreenContentRectangle(const IntRect& rect)
> +/*
> +    Compute the offscreen parts of the drawn rectangle by subtracting
> +    vis from rect. This can compute up to four rectangular slices.
> +*/

Usually the comments like this use '//' style.
Comment 3 Andrei Popescu 2009-11-13 10:56:28 PST
Created attachment 43171 [details]
Bring existing files in platform/android in line with Android 2.0 (v2)
Comment 4 Andrei Popescu 2009-11-13 11:01:50 PST
Created attachment 43172 [details]
Bring existing files in platform/android in line with Android 2.0 (v2)

Removed a tab that I added accidentally in the previous patch.
Comment 5 Andrei Popescu 2009-11-13 11:03:35 PST
Thanks Dimitry!

(In reply to comment #2)
> (From update of attachment 43083 [details])
> Looks good. Couple of style-related comments:
> 
> 
> > +PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
> > +{
> > +    RefPtr<RenderThemeAndroid> androidTheme = new RenderThemeAndroid();
> > +    return androidTheme.release();
> 
> return adoptRef(new RenderThemeAndroid()) ?
> The regular pattern is to have a RenderThemeAndroid::create() - see
> RenderThemeChromiumWin for example.
>

Fixed.
 
> > +    return RenderSkinCombo::Draw(getCanvasFromInfo(info), obj->node(), rect.x(),
> > +            rect.y(), rect.width(), rect.height());
> 
> Usually the lines longer then 80 characters are not wrapped in WebKit sources.
>

Fixed.

 
> > Index: WebCore/platform/android/ScreenAndroid.cpp
> >  
> > +#if PLATFORM(ANDROID)
> 
> Is this needed? It seems this file is Android-only.
> 

Good catch, removed.

> > Index: WebCore/platform/android/ScrollViewAndroid.cpp
> > -void ScrollView::platformOffscreenContentRectangle(const IntRect& rect)
> > +/*
> > +    Compute the offscreen parts of the drawn rectangle by subtracting
> > +    vis from rect. This can compute up to four rectangular slices.
> > +*/
> 
> Usually the comments like this use '//' style.

Fixed.

Thanks,
Andrei
Comment 6 Dmitry Titov 2009-11-13 11:54:25 PST
Comment on attachment 43172 [details]
Bring existing files in platform/android in line with Android 2.0 (v2)

Just one tiny thing before we can send the patch to commit-queue... I would do r+ with "fix on landing" but I think you are not yet a committer.

> +PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
> +{
> +    RefPtr<RenderTheme> androidTheme = RenderThemeAndroid::create();
> +    return androidTheme.release();
> +}

This will cause a new theme to be created for every request of themeForPage(). The other themes use static pointer initialized via releaseRef(), essentially pinning the refcount of the created theme at 1, so the theme is only created once, like here in RenderThemeChromiumMac: 

PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page*)
{
    static RenderTheme* rt = RenderThemeChromiumMac::create().releaseRef();
    return rt;
}

If there is no other considerations, it's better to keep the usage pattern.
Comment 7 Andrei Popescu 2009-11-13 12:21:15 PST
Created attachment 43189 [details]
Bring existing files in platform/android in line with Android 2.0 (v3)
Comment 8 WebKit Commit Bot 2009-11-13 13:23:57 PST
Comment on attachment 43189 [details]
Bring existing files in platform/android in line with Android 2.0 (v3)

Rejecting patch 43189 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
mporaryLinkStubs.cpp
	M	WebCore/platform/android/WidgetAndroid.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/WebCore/platform/android/RenderThemeAndroid.cpp

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 9 Dmitry Titov 2009-11-13 15:39:53 PST
Removed tab character and landed manually: http://trac.webkit.org/changeset/50971