Bug 31423 – [Android] The Android-specific files in platform/android are out of date

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andrei Popescu]

The files in WebCore/platform/android are out of date wrt to Android 2.0. There are also code style problems.  Patch following soon.

[Andrei Popescu]

Created attachment 43083 [details]
Bring existing files in platform/android in line with Android 2.0

[Dmitry Titov]

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.

[Andrei Popescu]

Created attachment 43171 [details]
Bring existing files in platform/android in line with Android 2.0 (v2)

[Andrei Popescu]

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.

[Andrei Popescu]

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

[Dmitry Titov]

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.

[Andrei Popescu]

Created attachment 43189 [details]
Bring existing files in platform/android in line with Android 2.0 (v3)

[WebKit Commit Bot]

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

[Dmitry Titov]

Removed tab character and landed manually: http://trac.webkit.org/changeset/50971