Move WindowOpenDisposition.template to ui/base/android.

ui: Move the rest of android/ dir into base/android/.

This completes the move of ui/android to ui/base/android.

BUG=299841
TEST=None, no functional changes.
R=bulach@chromium.org,newt@chromium.org

[tfarina, 2013-11-09 03:43:40]

Marcus, can I do this in small steps?

I'm having trouble uploading large chunks of files:

Traceback (most recent call last):
  File "/home/tfarina/depot_tools/git_cl.py", line 2327, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/tfarina/depot_tools/git_cl.py", line 2313, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/home/tfarina/depot_tools/subcommand.py", line 245, in execute
    return command(parser, args[1:])
  File "/home/tfarina/depot_tools/git_cl.py", line 1595, in CMDupload
    ret = RietveldUpload(options, args, cl)
  File "/home/tfarina/depot_tools/git_cl.py", line 1475, in RietveldUpload
    issue, patchset = upload.RealMain(upload_args)
  File "/home/tfarina/depot_tools/third_party/upload.py", line 2680, in RealMain
    vcs.UploadBaseFiles(issue, rpc_server, patches, patchset, options, files)
  File "/home/tfarina/depot_tools/third_party/upload.py", line 1215, in
UploadBaseFiles
    print t.get(timeout=60)
  File "/usr/lib/python2.7/multiprocessing/pool.py", line 528, in get
    raise self._value
urllib2.URLError: <urlopen error [Errno -2] Name or service not known>
Exception in thread Thread-7 (most likely raised during interpreter shutdown):
Traceback (most recent call last):

[tfarina, 2013-11-09 12:16:46]

This is not good, I'll try uploading more files.

Thanks!

[tfarina, 2013-11-10 21:15:13]

So, I moved everything (one (file) by one)! :/

I think this is ready for a rubber stamp.

Thanks!

[bulach, 2013-11-11 11:37:31]

+tedchoc is a better reviewer for ui/

I think the .java files would be better with a "base/" directory in the package
path, say:

from:
ui/base/android/java/src/org/chromium/ui/ActivityWindowAndroid.java

to:
ui/base/android/java/src/org/chromium/ui/

base/

ActivityWindowAndroid.java


this would also need some search and replace in the java files so they include
the right package name...

perhaps we could do as a separate patch though... ted?

[tfarina, 2013-11-11 15:07:51]

Ben,

Would you be fine if I revert https://codereview.chromium.org/51663003/, and
instead provide a ui/android/ui_android.gyp:ui_android_java (or whatever name we
choose) to build the C++ source files in its own specific target (rather than in
'ui' target).

While doing this patch, I think ui/android/ makes more sense than moving it
entirely into ui/base/. This would be more aligned with base/android,
chrome/android (there might be other examples as well that follow this pattern).

Marcus, Ted, what you think? Would this be better?

Thanks,

[Ben Goodger (Google), 2013-11-11 21:22:23]

I'm going to defer to Ted on this one.

-Ben


On Mon, Nov 11, 2013 at 7:07 AM, <tfarina@chromium.org> wrote:

> Ben,
>
> Would you be fine if I revert https://codereview.chromium.org/51663003/,
> and
> instead provide a ui/android/ui_android.gyp:ui_android_java (or whatever
> name we
> choose) to build the C++ source files in its own specific target (rather
> than in
> 'ui' target).
>
> While doing this patch, I think ui/android/ makes more sense than moving it
> entirely into ui/base/. This would be more aligned with base/android,
> chrome/android (there might be other examples as well that follow this
> pattern).
>
> Marcus, Ted, what you think? Would this be better?
>
> Thanks,
>
> https://codereview.chromium.org/62203014/
>

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

[Ted C, 2013-11-12 00:30:45]

On 2013/11/11 21:22:23, Ben Goodger (Google) wrote:
> I'm going to defer to Ted on this one.
> 
> -Ben
> 
> 
> On Mon, Nov 11, 2013 at 7:07 AM, <mailto:tfarina@chromium.org> wrote:
> 
> > Ben,
> >
> > Would you be fine if I revert https://codereview.chromium.org/51663003/,
> > and
> > instead provide a ui/android/ui_android.gyp:ui_android_java (or whatever
> > name we
> > choose) to build the C++ source files in its own specific target (rather
> > than in
> > 'ui' target).
> >
> > While doing this patch, I think ui/android/ makes more sense than moving it
> > entirely into ui/base/. This would be more aligned with base/android,
> > chrome/android (there might be other examples as well that follow this
> > pattern).
> >
> > Marcus, Ted, what you think? Would this be better?

Personally, I prefer ui/android purely from the consistency standpoint.  It
also goes along with the java coding style (though I don't know if that
has become irrelevant over time).

http://www.chromium.org/developers/coding-style/java

I believe the plan is to an android directory as a child of each root directory
and we start to mimic the chromium source directory structure like:
<root_dir>/android/java/src/org/chromium/<root_dir>/

Also, what target are you referring to for ui_android_java?  Is that the .jar
file compilation of the .java files?  Or is it the JNI header file generation
based on the annotations in the UI code?  If the latter, 'ui_jni_headers' is
the existing target and is the consistent naming style used.

> >
> > Thanks,
> >
> > https://codereview.chromium.org/62203014/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2013-11-12 01:00:35]

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode57
ui/ui.gyp:57: 'base/android/ui_jni_registrar.cc',
I'm talking about these C++ source files.

If I revert https://codereview.chromium.org/51663003/, then we need to create a
separate target to build them.

That is why I suggested ui_android_java. Perphaps ui_android_cpp makes more
sense?

WYT?

[Ted C, 2013-11-12 18:08:42]

+yfriedman in case he has any strong thoughts.

Looking at the files in ui/base/android, it looks to me like that is actually an
appropriate place for most of the files (or at least is on par with gtk and
cocoa).

The one file that sticks out is the ui_jni_registrar that requires potentially
dependencies on all of the ui sub folders.  To me, that is the only file that
should really get moved to ui/android and the rest should remain where they are.
 I would just keep this target as ui_android.

Granted if we do keep the classes in their current position, we should move the
corresponding .java classes to a base subpackage of the root ui java package.

The larger issues raised here are what is the intended purpose of
<root_folder>/android.  To me, I'd like to see that used as the root of the all
java files and a place for C++ files that have no corollary on any other
platform.  This is where other folks might want to chime in to point out how
silly these thoughts are.

[tfarina, 2013-11-12 19:26:34]

I was not familiar with java code and its structure, now I know it better.

See my replies below, they apply to Ted's comments.

Looks like we have an agreement to keep ui/android/ directory, right?

My understanding is that the purpose of ui/android/ is to be the place for all
java files and to mirror the underlining structure of ui/ for C++ files. That is
what ui/android/java/src/org/chromium/ui should be used for.

The c++ glue, should follow:

ui/app_list/android/ <- C++ files for Java/Android glue
ui/base/android/ <- and so on

The Java side:
ui/android/java/src/org/chromium/ui/app_list/
ui/android/java/src/org/chromium/ui/base/
ui/android/java/src/org/chromium/ui/gfx/

Is ui/android/java/src/org/chromium/ui/autofill/ used by anyone? It clearly
shouldn't be here in ui/ if it is a mirror for components/autofill/

https://codereview.chromium.org/62203014/diff/1650001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/62203014/diff/1650001/ui/gl/gl.gyp#newcode348
ui/gl/gl.gyp:348:
'../base/android/java/src/org/chromium/ui/gfx/SurfaceTexturePlatformWrapper.java',
looks like these two java files should be in
ui/android/java/src/org/chromium/ui/gl/ directory instead.

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode57
ui/ui.gyp:57: 'base/android/ui_jni_registrar.cc',
ui/android/cpp/src/ui_jni_registrar.* would be good place for these two files?

Maybe just ui/android/ui_jni_registar.* is OK?

Could be in ui/android/cpp/ui_jni_registrar.* also.

Which one do you prefer? I'm fine to take any suggestion/preference you have.

I like to use 'cpp', because it pairs with 'java'.

I saw the use of 'cpp' in third_party/libaddressinput/

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode57
ui/ui.gyp:57: 'base/android/ui_jni_registrar.cc',
One second though, looks like it is OK to keep ui_jni_registrar.cc here.

See my comments in
https://codereview.chromium.org/51663003/diff/1/ui/android/ui_jni_registrar.cc
and
https://codereview.chromium.org/51663003/diff/1/ui/base/android/ui_jni_regist...

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode275
ui/ui.gyp:275: 'base/ui_export.h',
we (I) should rename this to ui_base_export.h.

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode632
ui/ui.gyp:632: '../ui/base/android/java/src/org/chromium/ui/Clipboard.java',
I'll move these files into:

ui/android/java/src/org/chromium/ui/base/

[Ted C, 2013-11-12 19:44:56]

+aurimas for the autofill question, but it certainly looks like that no longer
belongs in UI anymore.

Overall, I'm happy with the approach in this latest round of comments.

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode57
ui/ui.gyp:57: 'base/android/ui_jni_registrar.cc',
On 2013/11/12 19:26:36, tfarina wrote:
> One second though, looks like it is OK to keep ui_jni_registrar.cc here.
> 
> See my comments in
> https://codereview.chromium.org/51663003/diff/1/ui/android/ui_jni_registrar.cc
> and
>
https://codereview.chromium.org/51663003/diff/1/ui/base/android/ui_jni_regist...

agreed.  I think ui_base_jni_registrar is good.

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode57
ui/ui.gyp:57: 'base/android/ui_jni_registrar.cc',
On 2013/11/12 19:26:36, tfarina wrote:
> ui/android/cpp/src/ui_jni_registrar.* would be good place for these two files?
> 
> Maybe just ui/android/ui_jni_registar.* is OK?
> 
> Could be in ui/android/cpp/ui_jni_registrar.* also.
> 
> Which one do you prefer? I'm fine to take any suggestion/preference you have.
> 
> I like to use 'cpp', because it pairs with 'java'.
> 
> I saw the use of 'cpp' in third_party/libaddressinput/

Not that I think this matters anymore, but if this were to come up again I would
vote against cpp purely based on consistency.  We don't use that in any
non-third_party directories, so we'd want a larger decision before taking this
path just in ui/

https://codereview.chromium.org/62203014/diff/1650001/ui/ui.gyp#newcode632
ui/ui.gyp:632: '../ui/base/android/java/src/org/chromium/ui/Clipboard.java',
On 2013/11/12 19:26:36, tfarina wrote:
> I'll move these files into:
> 
> ui/android/java/src/org/chromium/ui/base/

sgtm

[aurimas (slooooooooow), 2013-11-12 22:06:14]

ui/android/java/src/org/chromium/ui/autofill/ is used by:
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java

Where do you suggest we move it so that both android_webview/ and chrome/ can
use?

[tfarina, 2013-11-12 22:23:23]

On 2013/11/12 22:06:14, aurimas wrote:
> ui/android/java/src/org/chromium/ui/autofill/ is used by:
>
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> 
> Where do you suggest we move it so that both android_webview/ and chrome/ can
> use?

I'd suggest moving it to
components/android/java/src/org/chromium/components/autofill/. Looks like it is
in src/ui, because it is related to 'ui' (Popup, Suggestion) and android_webview
can't 'include' from chrome/?
Otherwise it could also be in
chrome/android/java/src/org/chromium/chrome/browser/ui/autofill/ (but may be
android_webview doesn't allow that?).

[aurimas (slooooooooow), 2013-11-12 22:33:00]

On 2013/11/12 22:23:23, tfarina wrote:
> On 2013/11/12 22:06:14, aurimas wrote:
> > ui/android/java/src/org/chromium/ui/autofill/ is used by:
> >
>
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
> >
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> > 
> > Where do you suggest we move it so that both android_webview/ and chrome/
can
> > use?
> 
> I'd suggest moving it to
> components/android/java/src/org/chromium/components/autofill/. Looks like it
is
> in src/ui, because it is related to 'ui' (Popup, Suggestion) and
android_webview
> can't 'include' from chrome/?
> Otherwise it could also be in
> chrome/android/java/src/org/chromium/chrome/browser/ui/autofill/ (but may be
> android_webview doesn't allow that?).

android_webview cannot use chrome/ so then we can move it to
components/android/java/src/org/chromium/components/autofill/.

Will you do it or do you want me to do that?

[tfarina, 2013-11-12 23:15:19]

On 2013/11/12 22:33:00, aurimas wrote:
> On 2013/11/12 22:23:23, tfarina wrote:
> > On 2013/11/12 22:06:14, aurimas wrote:
> > > ui/android/java/src/org/chromium/ui/autofill/ is used by:
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupGlue.java
> > > 
> > > Where do you suggest we move it so that both android_webview/ and chrome/
> can
> > > use?
> > 
> > I'd suggest moving it to
> > components/android/java/src/org/chromium/components/autofill/. Looks like it
> is
> > in src/ui, because it is related to 'ui' (Popup, Suggestion) and
> android_webview
> > can't 'include' from chrome/?
> > Otherwise it could also be in
> > chrome/android/java/src/org/chromium/chrome/browser/ui/autofill/ (but may be
> > android_webview doesn't allow that?).
> 
> android_webview cannot use chrome/ so then we can move it to
> components/android/java/src/org/chromium/components/autofill/.
> 
> Will you do it or do you want me to do that?
If you can take it I'd be happy :)

Otherwise leave it to me, so I can learn more about how java is compiled in
chromium.

[tfarina, 2013-11-12 23:16:33]

Ted, I made several changes that we discussed here in another CL that I uploaded
here https://codereview.chromium.org/70843003/

[tfarina, 2013-11-13 22:36:16]

This has been replaced by https://codereview.chromium.org/70843003/. Closing...