Restructuring WebContents functions from ContentViewCore to WebContents.

Restructuring WebContents functions from ContentViewCore to WebContents.

In this patch we are trying to position the WebContents functionalities to web_contents_android
file from ContentViewCoreImpl to make it functionally readable. Ensuring there is a one-to-one
mapping between WebContents Java and native counterparts.

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

[AKVT, 2014-08-13 16:15:48]

@Yaron PTAL

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:171:
@Override
Corrected the Override issues from previous patch.

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:180:
@Override
ditto

[Yaron, 2014-08-13 20:49:25]

lgtm

Looking forward to when we can get these functions split off from the java CVC.

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:152:
* Used in automation tests.
Can you remove this line? I think it's historical: we use it in other places

[boliu, 2014-08-13 21:04:28]

android_webview lgtm

[AKVT, 2014-08-14 01:14:58]

On 2014/08/13 20:49:25, Yaron wrote:
> lgtm
> 
> Looking forward to when we can get these functions split off from the java
CVC.
> 
>
https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
> (right):
> 
>
https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:152:
> * Used in automation tests.
> Can you remove this line? I think it's historical: we use it in other places

@Yaron could you pls explain bit more on the point "split off from the java CVC"
? Thanks

[Yaron, 2014-08-14 01:19:47]

On 2014/08/14 01:14:58, AKV wrote:
> On 2014/08/13 20:49:25, Yaron wrote:
> > lgtm
> > 
> > Looking forward to when we can get these functions split off from the java
> CVC.
> > 
> >
>
https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
> > File
> >
>
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
> >
>
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:152:
> > * Used in automation tests.
> > Can you remove this line? I think it's historical: we use it in other places
> 
> @Yaron could you pls explain bit more on the point "split off from the java
CVC"
> ? Thanks

The next logical step is to not have ContentViewCore.java always delegate to
WebContents. Instead, you'd have the callers of various ContentViewCore
functions just access WebContents directly and remove the function from
ContentViewCore

[AKVT, 2014-08-14 01:27:09]

On 2014/08/14 01:19:47, Yaron wrote:
> On 2014/08/14 01:14:58, AKV wrote:
> > On 2014/08/13 20:49:25, Yaron wrote:
> > > lgtm
> > > 
> > > Looking forward to when we can get these functions split off from the java
> > CVC.
> > > 
> > >
> >
>
https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
> > > File
> > >
> >
>
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
> > >
> >
>
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:152:
> > > * Used in automation tests.
> > > Can you remove this line? I think it's historical: we use it in other
places
> > 
> > @Yaron could you pls explain bit more on the point "split off from the java
> CVC"
> > ? Thanks
> 
> The next logical step is to not have ContentViewCore.java always delegate to
> WebContents. Instead, you'd have the callers of various ContentViewCore
> functions just access WebContents directly and remove the function from
> ContentViewCore

Thanks Yaron for quick clarification :) I will take care of it in future
patches.

[chromium-reviews, 2014-08-14 01:33:11]

No problem. Note that some of these functions have A LOT of users so we
don't want to do it all in one go. Also, certian pieces of code will have
access to a CVC but only need the WebContents, so it'll be good to look at
usages and plan the cleanup accordingly. For example, if there are places
where we can remove the dependency on CVC in favour of WebContents by
moving a few functions at the same time, those might be good to do in one
CL. I honestly haven't looked at all the clients but it will require some
thought.


On Wed, Aug 13, 2014 at 6:27 PM, <ajith.v@samsung.com> wrote:

> On 2014/08/14 01:19:47, Yaron wrote:
>
>> On 2014/08/14 01:14:58, AKV wrote:
>> > On 2014/08/13 20:49:25, Yaron wrote:
>> > > lgtm
>> > >
>> > > Looking forward to when we can get these functions split off from the
>> java
>> > CVC.
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/464393002/diff/1/content/
> public/android/java/src/org/chromium/content_public/
> browser/WebContents.java
>
>> > > File
>> > >
>> >
>>
>
> content/public/android/java/src/org/chromium/content_
> public/browser/WebContents.java
>
>> > > (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/464393002/diff/1/content/
> public/android/java/src/org/chromium/content_public/
> browser/WebContents.java#newcode152
>
>> > >
>> >
>>
>
> content/public/android/java/src/org/chromium/content_
> public/browser/WebContents.java:152:
>
>> > > * Used in automation tests.
>> > > Can you remove this line? I think it's historical: we use it in other
>>
> places
>
>> >
>> > @Yaron could you pls explain bit more on the point "split off from the
>> java
>> CVC"
>> > ? Thanks
>>
>
>  The next logical step is to not have ContentViewCore.java always delegate
>> to
>> WebContents. Instead, you'd have the callers of various ContentViewCore
>> functions just access WebContents directly and remove the function from
>> ContentViewCore
>>
>
> Thanks Yaron for quick clarification :) I will take care of it in future
> patches.
>
> https://codereview.chromium.org/464393002/
>

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

[AKVT, 2014-08-14 05:31:45]

Thanks Yaron and boliu. I have corrected the comment. PTAL

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/464393002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:152:
* Used in automation tests.
On 2014/08/13 20:49:25, Yaron wrote:
> Can you remove this line? I think it's historical: we use it in other places

Done.

[boliu, 2014-08-14 05:35:43]

On 2014/08/14 05:31:45, AKV wrote:
> Thanks Yaron and boliu. I have corrected the comment. PTAL

You did what Yaron asked, and there are no additional changes. No need to wait
for reapproval in this case.

[AKVT, 2014-08-14 05:35:48]

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

[AKVT, 2014-08-14 05:37:26]

On 2014/08/14 05:35:43, boliu wrote:
> On 2014/08/14 05:31:45, AKV wrote:
> > Thanks Yaron and boliu. I have corrected the comment. PTAL
> 
> You did what Yaron asked, and there are no additional changes. No need to wait
> for reapproval in this case.

Thanks boliu for quick update :) I am committing.

[commit-bot: I haz the power, 2014-08-14 05:37:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/464393002/20001

[AKVT, 2014-08-14 07:28:56]

@ted This patch is failing due to missing Owner LGTM for     
content/browser/web_contents/web_contents_android.cc
content/browser/web_contents/web_contents_android.h
files. PTAL

[commit-bot: I haz the power, 2014-08-14 11:42:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-14 11:46:02]

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

[commit-bot: I haz the power, 2014-08-14 11:46:04]

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

[Ted C, 2014-08-14 17:28:08]

web_contents_android.h - lgtm w/ some style nits

https://codereview.chromium.org/464393002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/464393002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:337:
->CreateRenderViewForInitialEmptyDocument()) {
This indenting seems off, I think the -> should go at the end of the previous
line and this should be 4 from the start of the if above.

https://codereview.chromium.org/464393002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:358:
ConvertJavaStringToUTF16(env, script), c_callback);
The c_ prefix is not something I've seen before.  I think to make this more
standard, I would call the param jcallback.  The scoped ref above
j_scoped_callback and this should just be callback.  Or if you want to keep it
as is, just call this js_callback to make it more about the javascript result
aspect (the c_ describing c++ looks weird)

[AKVT, 2014-08-14 17:39:59]

@Ted PTAL

https://codereview.chromium.org/464393002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/464393002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:337:
->CreateRenderViewForInitialEmptyDocument()) {
On 2014/08/14 17:28:07, Ted C wrote:
> This indenting seems off, I think the -> should go at the end of the previous
> line and this should be 4 from the start of the if above.

Done.

https://codereview.chromium.org/464393002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:358:
ConvertJavaStringToUTF16(env, script), c_callback);
On 2014/08/14 17:28:07, Ted C wrote:
> The c_ prefix is not something I've seen before.  I think to make this more
> standard, I would call the param jcallback.  The scoped ref above
> j_scoped_callback and this should just be callback.  Or if you want to keep it
> as is, just call this js_callback to make it more about the javascript result
> aspect (the c_ describing c++ looks weird)

Done. Thanks

[Yaron, 2014-08-14 17:42:08]

He said "lgtm w/ some style nits". That means that if you agree and make
the relevant changes he asked for you, can then commit the change


On Thu, Aug 14, 2014 at 10:39 AM, <ajith.v@samsung.com> wrote:

> @Ted PTAL
>
>
>
> https://codereview.chromium.org/464393002/diff/20001/
> content/browser/web_contents/web_contents_android.cc
> File content/browser/web_contents/web_contents_android.cc (right):
>
> https://codereview.chromium.org/464393002/diff/20001/
> content/browser/web_contents/web_contents_android.cc#newcode337
> content/browser/web_contents/web_contents_android.cc:337:
> ->CreateRenderViewForInitialEmptyDocument()) {
> On 2014/08/14 17:28:07, Ted C wrote:
>
>> This indenting seems off, I think the -> should go at the end of the
>>
> previous
>
>> line and this should be 4 from the start of the if above.
>>
>
> Done.
>
>
> https://codereview.chromium.org/464393002/diff/20001/
> content/browser/web_contents/web_contents_android.cc#newcode358
> content/browser/web_contents/web_contents_android.cc:358:
> ConvertJavaStringToUTF16(env, script), c_callback);
> On 2014/08/14 17:28:07, Ted C wrote:
>
>> The c_ prefix is not something I've seen before.  I think to make this
>>
> more
>
>> standard, I would call the param jcallback.  The scoped ref above
>> j_scoped_callback and this should just be callback.  Or if you want to
>>
> keep it
>
>> as is, just call this js_callback to make it more about the javascript
>>
> result
>
>> aspect (the c_ describing c++ looks weird)
>>
>
> Done. Thanks
>
> https://codereview.chromium.org/464393002/
>

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

[AKVT, 2014-08-14 17:44:30]

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

[commit-bot: I haz the power, 2014-08-14 17:46:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/464393002/40001

[commit-bot: I haz the power, 2014-08-14 22:22:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Yaron, 2014-08-14 22:26:19]

The CQ bit was unchecked by yfriedman@chromium.org

[Yaron, 2014-08-14 22:26:30]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2014-08-14 22:27:08]

The CQ bit was unchecked by yfriedman@chromium.org

[Yaron, 2014-08-14 22:28:51]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-08-14 22:29:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/464393002/40001

[Yaron, 2014-08-14 22:30:20]

On 2014/08/14 22:27:08, Yaron wrote:
> The CQ bit was unchecked by mailto:yfriedman@chromium.org

I was going to try to land as NOTRY=true since aosp bot has been unreliable the
last two days, but then I realized that it could actually break for similar
reasons as https://codereview.chromium.org/406023002/ so I went back to regular
commit. Sorry for the spam, and hopefully it goes through.

FYI for sheriff, https://chrome-internal-review.googlesource.com/#/c/172516/
fixes the internal tree

[commit-bot: I haz the power, 2014-08-15 05:10:31]

Committed patchset #3 (40001) as 289796

[AKVT, 2014-08-15 05:58:39]

On 2014/08/14 22:30:20, Yaron wrote:
> On 2014/08/14 22:27:08, Yaron wrote:
> > The CQ bit was unchecked by mailto:yfriedman@chromium.org
> 
> I was going to try to land as NOTRY=true since aosp bot has been unreliable
the
> last two days, but then I realized that it could actually break for similar
> reasons as https://codereview.chromium.org/406023002/ so I went back to
regular
> commit. Sorry for the spam, and hopefully it goes through.
> 
> FYI for sheriff, https://chrome-internal-review.googlesource.com/#/c/172516/
> fixes the internal tree

Thanks a lot Yaron :)