860940 - Replace AndroidGeckoEvent constructors with static factory methods

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Convert constructors to static factory methods

This is the equivalent of bug 725538 but for the JNI wrapper. We have a bunch of AndroidGeckoEvent constructors (and I'll be adding one more in a future bug) and I think it makes more sense to have factory-style methods instead because they can be given meaningful names.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

(Note: this patch depends on the patches in bug 859951 to apply cleanly)

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 736510 [details] [diff] [review]
Convert constructors to static factory methods

Review of attachment 736510 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: widget/android/AndroidJavaWrappers.h
@@ +621,3 @@
>      }
> +
> +    static AndroidGeckoEvent* MakeFromJava(JNIEnv *jenv, jobject jobj) {

I think the name `MakeFromJava()` is unclear. How about something like `MakeFromJavaObject()`?

@@ +627,4 @@
>      }
>  
> +    static AndroidGeckoEvent* CopyResizeEvent(AndroidGeckoEvent *aResizeEvent) {
> +        AndroidGeckoEvent* event = new AndroidGeckoEvent();

Your patch is inconsistent in its use of whitespace before or after * in pointer declarations. I don't really care which style you use, but new code should be consistent. I think Gecko style is whitespace after the *.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> I think the name `MakeFromJava()` is unclear. How about something like
> `MakeFromJavaObject()`?

Done, thanks.

> Your patch is inconsistent in its use of whitespace before or after * in
> pointer declarations. I don't really care which style you use, but new code
> should be consistent. I think Gecko style is whitespace after the *.

I usually also default to space after the * but it looks like most of our widget code does the opposite, hence the inconsistency. I modified my new code to use the existing style (space before *).

Will land once bug 859951 is in to avoid having to rebase.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63203f42c4b8

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63203f42c4b8