Optimizing DateTimeAndroid and ColorChooserAndroid initializations.

Optimizing DateTimeAndroid and ColorChooserAndroid initializations.

Some of the functions are direct wrapper to WebContents methods. In this
patch, taken care of removing those dependencies and added extra validation to prevent run time crashes.

BUG=398263
Committed: https://crrev.com/9e47ab574004f9b9f32b3b9e9b7cc3cb0e59d0c7
Cr-Commit-Position: refs/heads/master@{#302146}

[AKVT, 2014-10-13 18:08:31]

Patchset #2 (id:100001) has been deleted

[AKVT, 2014-10-13 18:10:14]

ajith.v@samsung.com changed reviewers:
+ tedchoc@chromium.org, yfriedman@chromium.org

[AKVT, 2014-10-13 18:10:15]

@Yaron & Ted PTAL

[AKVT, 2014-10-15 17:22:30]

@Yaron & Ted PTAL

[Yaron, 2014-10-15 20:32:38]

https://codereview.chromium.org/639703004/diff/130001/components/web_contents...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/639703004/diff/130001/components/web_contents...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:55:
webContents.getContext(), initialColor, suggestions);
From a preliminary look, it appears that ApplicationContext is sufficient for
this usage. If that's the case, then I'd prefer you use
base::android::GetApplciationContext in native, and pass that up before we
starting adding Context members to WebContents.

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
content/browser/android/date_time_chooser_android.h:18: class ContentViewCore;
remove

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
content/browser/android/date_time_chooser_android.h:31: void
ShowDialog(WebContents* webcontents,
web_contents

[Yaron, 2014-10-15 20:34:46]

On 2014/10/15 20:32:38, Yaron wrote:
>
https://codereview.chromium.org/639703004/diff/130001/components/web_contents...
> File
>
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
> (right):
> 
>
https://codereview.chromium.org/639703004/diff/130001/components/web_contents...
>
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:55:
> webContents.getContext(), initialColor, suggestions);
> From a preliminary look, it appears that ApplicationContext is sufficient for
> this usage. If that's the case, then I'd prefer you use
> base::android::GetApplciationContext in native, and pass that up before we
> starting adding Context members to WebContents.
> 
Actually, I'm not sure about the call to super (i.e. AlertDialog's ctor)

>
https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
> File content/browser/android/date_time_chooser_android.h (right):
> 
>
https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
> content/browser/android/date_time_chooser_android.h:18: class ContentViewCore;
> remove
> 
>
https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
> content/browser/android/date_time_chooser_android.h:31: void
> ShowDialog(WebContents* webcontents,
> web_contents

[Ted C, 2014-10-15 21:32:42]

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
content/browser/android/date_time_chooser_android.cc:106:
webcontents->GetJavaWebContents().obj(),
I want to agree with Yaron that context definitely shouldn't be added to
WebContents.

We could though look into using GetTopLevelNativeWindow from webcontents to get
access to the WindowAndroid.  Then that would give us access to the activity if
set (or the application context at least).

[AKVT, 2014-10-21 19:28:21]

On 2014/10/15 21:32:42, Ted C wrote:
>
https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
> File content/browser/android/date_time_chooser_android.cc (right):
> 
>
https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
> content/browser/android/date_time_chooser_android.cc:106:
> webcontents->GetJavaWebContents().obj(),
> I want to agree with Yaron that context definitely shouldn't be added to
> WebContents.
> 
> We could though look into using GetTopLevelNativeWindow from webcontents to
get
> access to the WindowAndroid.  Then that would give us access to the activity
if
> set (or the application context at least).

@Yaron & Ted, I am seeing some crash when I use
GetTopLevelNativeWindow->WindowAndroid->ApplicationContext as well as by using
base::android::GetApplciationContext(). I am correcting it, once it's done I
will upload new patch set.

[AKVT, 2014-10-29 18:13:29]

@Yaron & Ted PTAL

https://codereview.chromium.org/639703004/diff/130001/components/web_contents...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/639703004/diff/130001/components/web_contents...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:55:
webContents.getContext(), initialColor, suggestions);
On 2014/10/15 20:32:38, Yaron wrote:
> From a preliminary look, it appears that ApplicationContext is sufficient for
> this usage. If that's the case, then I'd prefer you use
> base::android::GetApplciationContext in native, and pass that up before we
> starting adding Context members to WebContents.

Done. Thank you

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
content/browser/android/date_time_chooser_android.cc:106:
webcontents->GetJavaWebContents().obj(),
On 2014/10/15 21:32:42, Ted C wrote:
> I want to agree with Yaron that context definitely shouldn't be added to
> WebContents.
> 
> We could though look into using GetTopLevelNativeWindow from webcontents to
get
> access to the WindowAndroid.  Then that would give us access to the activity
if
> set (or the application context at least).

Done. Thank you

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
content/browser/android/date_time_chooser_android.h:18: class ContentViewCore;
On 2014/10/15 20:32:38, Yaron wrote:
> remove

Done.

https://codereview.chromium.org/639703004/diff/130001/content/browser/android...
content/browser/android/date_time_chooser_android.h:31: void
ShowDialog(WebContents* webcontents,
On 2014/10/15 20:32:38, Yaron wrote:
> web_contents

Done.

[Yaron, 2014-10-29 23:37:22]

I think this lg (and thanks for the cleanup!) but I'll leave final approval to
Ted

[Ted C, 2014-10-30 00:01:01]

https://codereview.chromium.org/639703004/diff/200001/components/web_contents...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/639703004/diff/200001/components/web_contents...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:55:
windowAndroid.getActivity().get(), initialColor, suggestions);
.getActivity().get() can be null in the case of WebView which was not created
with the activity context or if this is called as the activity is garbage
collected, neither of which are very likely cases, but it might be worth
handling it.

Maybe in this case, you could return null from this method if .get() returns
null.  Then in native, you could just call OnColorChosen with the initial color.

I don't know if this will work, but it would be good to try (even just forcing
it to happen to make sure it doesn't crash) to handle this odd cases.

The same will apply in the date time picker.

[AKVT, 2014-10-30 18:26:02]

@Ted & Yaron PTAL

https://codereview.chromium.org/639703004/diff/200001/components/web_contents...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/639703004/diff/200001/components/web_contents...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:55:
windowAndroid.getActivity().get(), initialColor, suggestions);
On 2014/10/30 00:01:01, Ted C wrote:
> .getActivity().get() can be null in the case of WebView which was not created
> with the activity context or if this is called as the activity is garbage
> collected, neither of which are very likely cases, but it might be worth
> handling it.
> 
> Maybe in this case, you could return null from this method if .get() returns
> null.  Then in native, you could just call OnColorChosen with the initial
color.
> 
> I don't know if this will work, but it would be good to try (even just forcing
> it to happen to make sure it doesn't crash) to handle this odd cases.
> 
> The same will apply in the date time picker.

Done. Thank you

[Ted C, 2014-10-30 18:27:51]

lgtm

[AKVT, 2014-10-30 18:57:33]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-10-30 18:59:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/639703004/220001

[commit-bot: I haz the power, 2014-10-30 19:05:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 19:05:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[AKVT, 2014-10-30 19:12:05]

ajith.v@samsung.com changed reviewers:
+ avi@chromium.org

[AKVT, 2014-10-30 19:12:06]

@Ted Thanks.
@avi PTAL. This patch is missing web_contents_impl.cc OWNER LGTM

[Avi (use Gerrit), 2014-10-30 19:22:43]

On 2014/10/30 19:12:06, AKV wrote:
> @Ted Thanks.
> @avi PTAL. This patch is missing web_contents_impl.cc OWNER LGTM

That one file LGTM.

[Avi (use Gerrit), 2014-10-30 19:22:59]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-10-30 19:24:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/639703004/220001

[commit-bot: I haz the power, 2014-10-30 20:24:25]

Committed patchset #5 (id:220001)

[commit-bot: I haz the power, 2014-10-30 20:25:08]

Patchset 5 (id:??) landed as
https://crrev.com/9e47ab574004f9b9f32b3b9e9b7cc3cb0e59d0c7
Cr-Commit-Position: refs/heads/master@{#302146}