Make WindowAndroid constructor takes context as param.

Make WindowAndroid constructor takes context as param.

BUG=309104
TBR=yfriedman

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233774

[joth__google, 2013-10-20 17:02:58]

Feels this would be clearer if windowAndroid was abstract and we had
different subclasses for on screen and off screen windows.

It seems awkward that showIntent falls in the on-screen only bucket. That
does seem like unfortunate tripwire to set. (OTOH as showIntent can fail
anyway, so it's not opening a whole new error path)



On Friday, 18 October 2013,  <michaelbai@chromium.org> wrote:
> Reviewers: newt, yfriedman, joth,
>
> Description:
> Make WindowAndroid constructor takes context as param.
>
> BUG=309104
>
> Please review this at https://codereview.chromium.org/29303004/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+50, -18 lines):
>   M
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
>   M
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
>   M
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
>   M ui/android/java/src/org/chromium/ui/WindowAndroid.java
>
>
> Index:
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
> diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
b/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
> index
502dc8100713e808e86307ddb098ec987ae5ba77..ad63e7ecd1ec2138e3b0e60e8f576a4ea19344ac
100644
> ---
a/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
> +++
b/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
> @@ -70,7 +70,7 @@ public class JavascriptAppModalDialog implements
DialogInterface.OnClickListener
>      void showJavascriptAppModalDialog(WindowAndroid window, int
nativeDialogPointer) {
>          assert window != null;
>          Context context = window.getContext();
> -
> +        assert context != null;
>          // Cache the native dialog pointer so that we can use it to
return the response.
>          mNativeDialogPointer = nativeDialogPointer;
>
> Index:
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> index
fc8f4edd98adb5d2831b3d3bb9fb66ad893e93c1..6e9e3443abfcefa5abdffb9a70acb1a10b1baacf
100644
> ---
a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> +++
b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> @@ -4,6 +4,8 @@
>
>  package org.chromium.chrome.browser.autofill;
>
> +import android.content.Context;
> +
>  import org.chromium.base.CalledByNative;
>  import org.chromium.base.JNINamespace;
>  import org.chromium.ui.ViewAndroid;
> @@ -24,7 +26,9 @@ public class AutofillPopupGlue implements
AutofillPopupDelegate{
>      public AutofillPopupGlue(int nativeAutofillPopupViewAndroid,
WindowAndroid windowAndroid,
>              ViewAndroidDelegate containerViewDelegate) {
>          mNativeAutofillPopup = nativeAutofillPopupViewAndroid;
> -        mAutofillPopup = new AutofillPopup(windowAndroid.getContext(),
containerViewDelegate, this);
> +        Context context = windowAndroid.getContext();
> +        assert context != null;
> +        mAutofillPopup = new AutofillPopup(context,
containerViewDelegate, this);
>      }
>
>      @CalledByNative
> Index:
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
> diff --git
a/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
b/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
> index
94d2b5314103d1027c1929aecf0aa55a7d6145e1..0f30ef164835cf3751a26637f501d493d80ff22f
100644
> ---
a/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
> +++
b/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
> @@ -4,6 +4,7 @@
>
>  package org.chromium.chrome.browser.autofill;
>
> +import android.content.Context;
>  import android.test.suitebuilder.annotation.SmallTest;
>
>  import org.chromium.base.test.util.Feature;
> @@ -46,7 +47,9 @@ public class AutofillTest extends
ChromiumTestShellTestBase {
>          UiUtils.runOnUiThread(getActivity(), new Runnable() {
>              @Override
>              public void run() {
> -                mAutofillPopup = new
AutofillPopup(mWindowAndroid.getContext(),
> +                Context context = mWindowAndroid.getContext();
> +                assert context != null;
> +                mAutofillPopup = new AutofillPopup(context,
>                          viewDelegate,
>                          mMockAutofillCallback);
>                  mAutofillPopup.setAnchorRect(50, 500, 500, 50);
> Index: ui/android/java/src/org/chromium/ui/WindowAndroid.java
> diff --git a/ui/android/java/src/org/chromium/ui/WindowAndroid.java
b/ui/android/java/src/org/chromium/ui/WindowAndroid.java
> index
0707eafeeb6da70992f5fe1b9f388d3a2faf02df..55206905a527e280d4edaab0ff508d2a613c5bda
100644
> --- a/ui/android/java/src/org/chromium/ui/WindowAndroid.java
> +++ b/ui/android/java/src/org/chromium/ui/WindowAndroid.java
> @@ -28,6 +28,21 @@ import org.chromium.base.JNINamespace;
>   */
>  @JNINamespace("ui")
>  public class WindowAndroid {
> +    // This simple class is used to prevent activity from being directly
accessed
> +    // and output the alert if the activity is null.
> +    private static class ActivityGetter {
> +        private Activity mActivity;
> +        public ActivityGetter(Activity activity) {
> +            mActivity = activity;
> +        }
> +
> +        public Activity get() {
> +            if (mActivity == null) {
> +                Log.w(TAG, "Wrong type of Context, Activity's Context is
required");
> +            }
> +            return mActivity;
> +        }
> +    }
>
>      private static final String TAG = "WindowAndroid";
>
> @@ -41,20 +56,25 @@ public class WindowAndroid {
>      static final String WINDOW_CALLBACK_ERRORS =
"window_callback_errors";
>
>      private int mNextRequestCode = 0;
> -    protected Activity mActivity;
>      protected Context mApplicationContext;
>      protected SparseArray<IntentCallback> mOutstandingIntents;
>      protected HashMap<Integer, String> mIntentErrors;
> +    private ActivityGetter mActivityGetter;
>
>      /**
> -     * @param activity
> +     * @param context, the activity context is required, if the
application context is given,
> +     * some features don't work.
>       */
> -    public WindowAndroid(Activity activity) {
> -        mActivity = activity;
> -        mApplicationContext = mActivity.getApplicationContext();
> +    public WindowAndroid(Context context) {
> +        Activity activity = null;
> +        if (context instanceof Activity) activity = (Activity)context;
> +        mActivityGetter = new ActivityGetter(activity);
> +        if (activity != null)
> +            mApplicationContext = activity.getApplicationContext();
> +        else
> +            mApplicationContext = context;
>          mOutstandingIntents = new SparseArray<IntentCallback>();
>          mIntentErrors = new HashMap<Integer, String>();
> -
>      }
>
>      /**
> @@ -68,15 +88,17 @@ public class WindowAndroid {
>      public boolean showIntent(Intent intent, IntentCallback callback,
int errorId) {
>          int requestCode = REQUEST_CODE_PREFIX + mNextRequestCode;
>          mNextRequestCode = (mNextRequestCode + 1) %
REQUEST_CODE_RANGE_SIZE;
> +        Activity activity = mActivityGetter.get();
> +        if (activity == null) return false;
>
>          try {
> -            mActivity.startActivityForResult(intent, requestCode);
> +            activity.startActivityForResult(intent, requestCode);
>          } catch (ActivityNotFoundException e) {
>              return false;
>          }
>
>          mOutstandingIntents.put(requestCode, callback);
> -        mIntentErrors.put(requestCode, mActivity.getString(errorId));
> +        mIntentErrors.put(requestCode,
mApplicationContext.getString(errorId));
>
>          return true;
>      }
> @@ -87,7 +109,7 @@ public class WindowAndroid {
>       */
>      public void showError(String error) {
>          if (error != null) {
> -            Toast.makeText(mActivity, error, Toast.LENGTH_SHORT).show();
> +            Toast.makeText(mApplicationContext, error,
Toast.LENGTH_SHORT).show();
>          }
>      }
>
> @@ -96,7 +118,7 @@ public class WindowAndroid {
>       * @param resId The error message string's resource id.
>       */
>      public void showError(int resId) {
> -        showError(mActivity.getString(resId));
> +        showError(mApplicationContext.getString(resId));
>      }
>
>      /**
> @@ -111,17 +133,18 @@ public class WindowAndroid {
>       * Broadcasts the given intent to all interested BroadcastReceivers.
>       */
>      public void sendBroadcast(Intent intent) {
> -        mActivity.sendBroadcast(intent);
> +        mApplicationContext.sendBroadcast(intent);
>      }
>
>      /**
>       * TODO(nileshagrawal): Stop returning Activity Context
crbug.com/233440.
> -     * @return Activity context.
> +     * @return Activity context, it could be null. Note, in most cases,
you probably
> +     * just need Application Context returned by getApplicationContext().
>       * @see #getApplicationContext()
>       */
>      @Deprecated
>      public Context getContext() {
> -        return mActivity;
> +        return mActivityGetter.get();
>      }
>
>      /**
> @@ -170,7 +193,7 @@ public class WindowAndroid {
>
>          if (callback != null) {
>              callback.onIntentCompleted(this, resultCode,
> -                    mActivity.getContentResolver(), data);
> +                    mApplicationContext.getContentResolver(), data);
>              return true;
>          } else {
>              if (errorMessage != null) {
> @@ -224,10 +247,12 @@ public class WindowAndroid {
>       */
>      @CalledByNative
>      public byte[] grabSnapshot(int windowX, int windowY, int width, int
height) {
> +        Activity activity = mActivityGetter.get();
> +        if (activity == null) return null;
>          try {
>              // Take a screenshot of the content view. This generally
includes UI
>              // controls such as the URL bar.
> -            View contentView =
mActivity.findViewById(android.R.id.content);
> +            View contentView =
activity.findViewById(android.R.id.content);
>              if (contentView == null) return null;
>              Bitmap bitmap =
>                      UiUtils.generateScaledScreenshot(contentView, 0,
Bitmap.Config.ARGB_8888);
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-10-20 17:02:59]

From correct address

On Sunday, 20 October 2013, Jonathan Dixon <joth@google.com> wrote:
>
> Feels this would be clearer if windowAndroid was abstract and we had
different subclasses for on screen and off screen windows.
>
> It seems awkward that showIntent falls in the on-screen only bucket. That
does seem like unfortunate tripwire to set. (OTOH as showIntent can fail
anyway, so it's not opening a whole new error path)
>
>
>
> On Friday, 18 October 2013,  <michaelbai@chromium.org> wrote:
>> Reviewers: newt, yfriedman, joth,
>>
>> Description:
>> Make WindowAndroid constructor takes context as param.
>>
>> BUG=309104
>>
>> Please review this at https://codereview.chromium.org/29303004/
>>
>> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>>
>> Affected files (+50, -18 lines):
>>   M
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
>>   M
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
>>   M
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
>>   M ui/android/java/src/org/chromium/ui/WindowAndroid.java
>>
>>
>> Index:
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
>> diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
b/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
>> index
502dc8100713e808e86307ddb098ec987ae5ba77..ad63e7ecd1ec2138e3b0e60e8f576a4ea19344ac
100644
>> ---
a/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
>> +++
b/chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
>> @@ -70,7 +70,7 @@ public class JavascriptAppModalDialog implements
DialogInterface.OnClickListener
>>      void showJavascriptAppModalDialog(WindowAndroid window, int
nativeDialogPointer) {
>>          assert window != null;
>>          Context context = window.getContext();
>> -
>> +        assert context != null;
>>          // Cache the native dialog pointer so that we can use it to
return the response.
>>          mNativeDialogPointer = nativeDialogPointer;
>>
>> Index:
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
>> diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
>> index
fc8f4edd98adb5d2831b3d3bb9fb66ad893e93c1..6e9e3443abfcefa5abdffb9a70acb1a10b1baacf
100644
>> ---
a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
>> +++
b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
>> @@ -4,6 +4,8 @@
>>
>>  package org.chromium.chrome.browser.autofill;
>>
>> +import android.content.Context;
>> +
>>  import org.chromium.base.CalledByNative;
>>  import org.chromium.base.JNINamespace;
>>  import org.chromium.ui.ViewAndroid;
>> @@ -24,7 +26,9 @@ public class AutofillPopupGlue implements
AutofillPopupDelegate{
>>      public AutofillPopupGlue(int nativeAutofillPopupViewAndroid,
WindowAndroid windowAndroid,
>>              ViewAndroidDelegate containerViewDelegate) {
>>          mNativeAutofillPopup = nativeAutofillPopupViewAndroid;
>> -        mAutofillPopup = new AutofillPopup(windowAndroid.getContext(),
containerViewDelegate, this);
>> +        Context context = windowAndroid.getContext();
>> +        assert context != null;
>> +        mAutofillPopup = new AutofillPopup(context,
containerViewDelegate, this);
>>      }
>>
>>      @CalledByNative
>> Index:
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
>> diff --git
a/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
b/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
>> index
94d2b5314103d1027c1929aecf0aa55a7d6145e1..0f30ef164835cf3751a26637f501d493d80ff22f
100644
>> ---
a/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
>> +++
b/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
>> @@ -4,6 +4,7 @@
>>
>>  package org.chromium.chrome.browser.autofill;
>>
>> +import android.content.Context;
>>  import android.test.suitebuilder.annotation.SmallTest;
>>
>>  import org.chromium.base.test.util.Feature;
>> @@ -46,7 +47,9 @@ public class AutofillTest extends
ChromiumTestShellTestBase {
>>          UiUtils.runOnUiThread(getActivity(), new Run

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelbai, 2013-10-25 22:23:21]

As joth's suggestion, splited WindowAndroid into WindowAndroid and
ActivityWindowAndroid.

[Yaron, 2013-10-25 23:24:12]

Newton can you review instead?

[newt (away), 2013-10-27 19:57:50]

I'm a bit confused. You've separated WindowAndroid into WindowAndroid and
ActivityWindowAndroid, which makes the internals of those two classes much
clearer (e.g. WindowAndroid can never accidentally reference the Activity). But
WindowAndroid has default no-op implementations for all the
ActivityWindowAndroid methods, and all the clients still refer directly to
WindowAndroid... so they have no way of knowing if they're calling methods that
are allowed or not.

Could we pass around ActivityWindowAndroid in cases where the activity is needed
and used, and remove the no-op default method implementations from
WindowAndroid?

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
don't need "(c)"

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:23: public class
ActivityWindowAndroid extends WindowAndroid{
space before {

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:74: @Deprecated
this isn't deprecated at all, is it? we are guaranteed to have an activity
context and we don't have any plans to change that.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:44: * @param context, the
application context..
"@param context The application context."

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:46: public
WindowAndroid(Context context) {
might be good to add:

    assert context == context.getApplicationContext();

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:99: *
TODO(nileshagrawal): Stop returning Activity Context crbug.com/233440.
remove TODO now

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:105: public Context
getContext() {
we should call this getActivityContext() to be clear

[michaelbai, 2013-10-28 17:40:38]

Because of webview, we can't pass ActivityWindowAndroid as parameter in some
cases even the Activity Context is needed, for example ContentViewCore,

WebView can't always get Activity context, and most of features of
ContentViewCore just need application context;
the developers should test their feature work or not, once they get the warning
message, they will know which kind of WindowAndroid needed.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/10/27 19:57:51, newt wrote:
> don't need "(c)"

Done.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:23: public class
ActivityWindowAndroid extends WindowAndroid{
On 2013/10/27 19:57:51, newt wrote:
> space before {

Done.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:74: @Deprecated
We shouldn't pass the Activity Context outside of this class, otherwise, the
caller could keep the reference of it, which will stop the activity be gc-ed.


On 2013/10/27 19:57:51, newt wrote:
> this isn't deprecated at all, is it? we are guaranteed to have an activity
> context and we don't have any plans to change that.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:44: * @param context, the
application context..
On 2013/10/27 19:57:51, newt wrote:
> "@param context The application context."

Done.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:46: public
WindowAndroid(Context context) {
On 2013/10/27 19:57:51, newt wrote:
> might be good to add:
> 
>     assert context == context.getApplicationContext();

Done.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:99: *
TODO(nileshagrawal): Stop returning Activity Context crbug.com/233440.
On 2013/10/27 19:57:51, newt wrote:
> remove TODO now

Done.

https://codereview.chromium.org/29303004/diff/50001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:105: public Context
getContext() {
As I plan to remove this method, not worthy of changing the name now.

On 2013/10/27 19:57:51, newt wrote:
> we should call this getActivityContext() to be clear

[michaelbai, 2013-11-01 18:04:08]

ping

[joth, 2013-11-01 18:45:00]

mostly looks good now, just a quibble on how we handle this in AutofillPopupGlue

https://codereview.chromium.org/29303004/diff/130001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
(right):

https://codereview.chromium.org/29303004/diff/130001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java:31:
mAutofillPopup = new AutofillPopup(context, containerViewDelegate, this);
Is the edit in this file really needed? You're going to get a NPE inside
AutofillPopup c'tor anyway, which will be just as obvious what went wrong

Although technically I think the correct thing is to return null from create()
and wind that all the way back so AutofillPopupView::Create() returns NULL on
the native side (which it's allowed to, and is handled already).
This will handle the case of an offscreen webview having key-events
programmatically injected and causing us to hit this autofill path.

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:21: * Only
instance this class when you need the implemented features.
instance => instantiate

Although this statement is kind of vague, it seems it should apply to every
class in the system

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:27: private static void
showActivityRequiredWarning() {
nit: show => log

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:28: Log.w(TAG, "Wrong
type of Context, Activity's Context is required");
"A Content of type Activity is required."

seems this could be spammy or lost in the log though, especially as it doesn't
give any clue what functionality is being dropped

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:106: public Context
getContext() {
can we remove this from the base class? (it's deprecated, looks like only one
place uses it now?)

[michaelbai, 2013-11-01 21:27:04]

PTAL

https://codereview.chromium.org/29303004/diff/130001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
(right):

https://codereview.chromium.org/29303004/diff/130001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java:31:
mAutofillPopup = new AutofillPopup(context, containerViewDelegate, this);
I am planing to remove this deprecated method completely.  I reset all the
change and recorded you comments at  http://crbug.com/233440

On 2013/11/01 18:45:01, joth wrote:
> Is the edit in this file really needed? You're going to get a NPE inside
> AutofillPopup c'tor anyway, which will be just as obvious what went wrong
> 
> Although technically I think the correct thing is to return null from create()
> and wind that all the way back so AutofillPopupView::Create() returns NULL on
> the native side (which it's allowed to, and is handled already).
> This will handle the case of an offscreen webview having key-events
> programmatically injected and causing us to hit this autofill path.
>

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ActivityWindowAndroid.java:21: * Only
instance this class when you need the implemented features.
On 2013/11/01 18:45:01, joth wrote:
> instance => instantiate
> 
> Although this statement is kind of vague, it seems it should apply to every
> class in the system

Done

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:27: private static void
showActivityRequiredWarning() {
On 2013/11/01 18:45:01, joth wrote:
> nit: show => log

Done.

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:28: Log.w(TAG, "Wrong
type of Context, Activity's Context is required");
On 2013/11/01 18:45:01, joth wrote:
> "A Content of type Activity is required."
> 
> seems this could be spammy or lost in the log though, especially as it doesn't
> give any clue what functionality is being dropped

Done.

This message is especially for the webview developers, it is not make sense to
show the local methods here. I hope this give some clue for them.

https://codereview.chromium.org/29303004/diff/130001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:106: public Context
getContext() {
There are some usages in clank. I would remove them completely when fix the
crbug.com/233440

On 2013/11/01 18:45:01, joth wrote:
> can we remove this from the base class? (it's deprecated, looks like only one
> place uses it now?)

[joth, 2013-11-01 23:12:17]

lgtm

https://codereview.chromium.org/29303004/diff/210001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/210001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:60:
logActivityRequiredWarning();
I think you'd be better having a clearer log here:
Log.d(TAG, "Can't show intent as context is not an Activity: " + intent);

And then dropping the other logActivityRequiredWarning() calls -- they're not
really going to help anyone.

[michaelbai, 2013-11-07 21:10:03]

@newt, Do you want to take another look?
@yfriedman, Needs your owner approval for all files.

https://codereview.chromium.org/29303004/diff/210001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/210001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:60:
logActivityRequiredWarning();
On 2013/11/01 23:12:18, joth wrote:
> I think you'd be better having a clearer log here:
> Log.d(TAG, "Can't show intent as context is not an Activity: " + intent);
> 
> And then dropping the other logActivityRequiredWarning() calls -- they're not
> really going to help anyone.

Done.

[newt (away), 2013-11-07 21:55:12]

I'd prefer not having the stub implementations in WindowAndroid. If callers need
to use methods only available in ActivityWindowAndroid, they should either
already have a reference to an AcitityWindowAndroid (ideal case) or cast
WindowAndroid to ActivityWindowAndroid (only when it's unavoidable). Would that
work?

If not, I think this is reasonable.

https://codereview.chromium.org/29303004/diff/270001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/270001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:56: Log.d(TAG, "Can't
show intent as context is not an Activity: " + intent);;
double ;;

[michaelbai, 2013-11-07 22:18:23]

The WindowAndroid or ActivityWindowAndroid is not a WebView API, what we got
from app is context, we will use it like this

WindowAndroid windowAndroid;

if (context instanceof Activity)
  windowAndroid = ActivityWindowAndroid((Activity)context);
else
  windowAndroid = new WindowAndroid(context);

Sounds reasonable?

https://codereview.chromium.org/29303004/diff/270001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/270001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:56: Log.d(TAG, "Can't
show intent as context is not an Activity: " + intent);;
On 2013/11/07 21:55:13, newt wrote:
> double ;;

Done.

[newt (away), 2013-11-07 22:30:42]

On 2013/11/07 22:18:23, michaelbai wrote:
> The WindowAndroid or ActivityWindowAndroid is not a WebView API, what we got
> from app is context, we will use it like this
> 
> WindowAndroid windowAndroid;
> 
> if (context instanceof Activity)
>   windowAndroid = ActivityWindowAndroid((Activity)context);
> else
>   windowAndroid = new WindowAndroid(context);
> 
> Sounds reasonable?

Ok, understood. Sounds good.

lgtm

[commit-bot: I haz the power, 2013-11-07 22:55:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/29303004/320001

[commit-bot: I haz the power, 2013-11-08 05:31:23]

Change committed as 233774

[frankf, 2013-11-09 00:33:57]

On 2013/11/08 05:31:23, I haz the power (commit-bot) wrote:
> Change committed as 233774

This broke content_browsertests:

http://build.chromium.org/p/chromium.fyi/builders/Android%20Tests%20%28ICS%20...
Logcat:
http://build.chromium.org/p/chromium.fyi/builders/Android%20Tests%20%28ICS%20...
1801F: 11-08 07:49:35.953 22046 22046 E AndroidRuntime: java.lang.AssertionError
1801F: 11-08 07:49:35.953 22046 22046 E AndroidRuntime: at
org.chromium.ui.WindowAndroid.<init>(WindowAndroid.java:41) 1801F: 11-08
07:49:35.953 22046 22046 E AndroidRuntime: at
org.chromium.content_browsertests_apk.ContentBrowserTestsActivity.onCreate(ContentBrowserTestsActivity.java:43)

[joth, 2013-11-09 02:22:10]

https://codereview.chromium.org/29303004/diff/320001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/320001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:41: assert context ==
context.getApplicationContext();
Can this just be removed?

[michaelbai, 2013-11-09 06:05:53]

https://codereview.chromium.org/29303004/diff/320001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):

https://codereview.chromium.org/29303004/diff/320001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/WindowAndroid.java:41: assert context ==
context.getApplicationContext();
I think this make the people use the right class, I found there are 2 places
need to change, and will have patch soon.

On 2013/11/09 02:22:11, joth wrote:
> Can this just be removed?

[joth, 2013-11-09 06:27:07]

Ah gotcha. Thanks.

On Friday, 8 November 2013, wrote:

>
> https://codereview.chromium.org/29303004/diff/320001/ui/
> android/java/src/org/chromium/ui/WindowAndroid.java
> File ui/android/java/src/org/chromium/ui/WindowAndroid.java (right):
>
> https://codereview.chromium.org/29303004/diff/320001/ui/
> android/java/src/org/chromium/ui/WindowAndroid.java#newcode41
> ui/android/java/src/org/chromium/ui/WindowAndroid.java:41: assert
> context == context.getApplicationContext();
> I think this make the people use the right class, I found there are 2
> places need to change, and will have patch soon.
>
> On 2013/11/09 02:22:11, joth wrote:
>
>> Can this just be removed?
>>
>
> https://codereview.chromium.org/29303004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.