Restructuring NavigationController functionalities from ContentViewCore to NavigationController

In the current code of Android Chromium we have scattered all the NavigationController functionalities inside ContentViewCore.
I have started to migrate the ownership to respective class for handling specific functionalities.

In this patch I have taken ownership of NavigationController functionalities to drop in NavaigationController classes.

BUG=398263
R=avi@chromium.org, boliu@chromium.org, nasko@chromium.org, yfriedman@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/4502cc6303f4aabd05e7fef6fbfaf4dfd1600bb6

[AKVT, 2014-07-21 18:07:01]

PTAL this change

[AKVT, 2014-07-21 18:21:09]

@Yaron and Ted C PTAL

[AKVT, 2014-07-22 06:03:38]

@nasko PTAL NavigationController changes.
Some of the function scope I changed to public due to accessing from outside
original package.

[AKVT, 2014-07-22 15:05:46]

On 2014/07/22 06:03:38, AKV wrote:
> @nasko PTAL NavigationController changes.
> Some of the function scope I changed to public due to accessing from outside
> original package.

@Yaron PTAL this continuation change.

[AKVT, 2014-07-22 18:34:24]

On 2014/07/22 15:05:46, AKV wrote:
> On 2014/07/22 06:03:38, AKV wrote:
> > @nasko PTAL NavigationController changes.
> > Some of the function scope I changed to public due to accessing from outside
> > original package.
> 
> @Yaron PTAL this continuation change.

@Ted C and Yaron PTAL

[Ted C, 2014-07-22 18:39:49]

On 2014/07/22 18:34:24, AKV wrote:
> On 2014/07/22 15:05:46, AKV wrote:
> > On 2014/07/22 06:03:38, AKV wrote:
> > > @nasko PTAL NavigationController changes.
> > > Some of the function scope I changed to public due to accessing from
outside
> > > original package.
> > 
> > @Yaron PTAL this continuation change.
> 
> @Ted C and Yaron PTAL

Please be patient.  We have seen this change, but continually asking people to
take
a look will not make it happen any faster.  Generally, I recommend only pinging
a
change if it has been pending for a couple days without comment.

[AKVT, 2014-07-22 18:47:00]

On 2014/07/22 18:39:49, Ted C wrote:
> On 2014/07/22 18:34:24, AKV wrote:
> > On 2014/07/22 15:05:46, AKV wrote:
> > > On 2014/07/22 06:03:38, AKV wrote:
> > > > @nasko PTAL NavigationController changes.
> > > > Some of the function scope I changed to public due to accessing from
> outside
> > > > original package.
> > > 
> > > @Yaron PTAL this continuation change.
> > 
> > @Ted C and Yaron PTAL
> 
> Please be patient.  We have seen this change, but continually asking people to
> take
> a look will not make it happen any faster.  Generally, I recommend only
pinging
> a
> change if it has been pending for a couple days without comment.

Thanks Ted C for the clarification. I thought this patch is unnoticed, so pinged
in day time. Now onward I will follow above process.

[Yaron, 2014-07-22 20:49:06]

https://codereview.chromium.org/406023002/diff/20001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:26: #include
"content/browser/frame_host/navigation_controller_impl.h"
can you remove these 2 inclues?

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:10: #include
"content/browser/frame_host/navigation_entry_impl.h"
can this just use content/public/browser/frame_host/navigation_entry.h?

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:171:
LoadUrl(params);
just call LoadUrlWithParams directly and get rid of the wrapper.

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:180: namespace {
please move this namespace up above all the member functions.

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.h (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.h:50: void
LoadUrl(NavigationController::LoadURLParams& params);
remove

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2249:
assert mWebContents != null;
Sadly, I think this one can happen, which would be an inconsistency... Ted, I
forget, does native NTP have a WebContents?

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:15:
* Holds parameters for ContentViewCore.LoadUrl. Parameters should match
NavigationController

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:85:
* Load url without fixing up the url string. Consumers of ContentView are
responsible for
s/Consumers of ContentView/Clients of NavigationController/

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:94:
* Clears the ContentViewCore's page history in both the backwards and
s/ContentViewCore/NavigationController/

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:100:
* Get a copy of the navigation history of the view.
Ok, I'll stop commenting on view-related changes. Please take a look at the rest
of the diff (and comments) and make sure they make sense. There's no notion of a
"view" here.

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:122:
* Clears SSL preferences for this WebContent Navigation Controller.
s/WebContent//

[Ted C, 2014-07-23 00:04:22]

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2249:
assert mWebContents != null;
On 2014/07/22 20:49:06, Yaron wrote:
> Sadly, I think this one can happen, which would be an inconsistency... Ted, I
> forget, does native NTP have a WebContents?

A CVC should always have a WebContents (unless destroy has been called), so I
think this assert is fine in this case.

[AKVT, 2014-07-23 11:49:23]

PTAL updated changes.

https://codereview.chromium.org/406023002/diff/20001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:26: #include
"content/browser/frame_host/navigation_controller_impl.h"
On 2014/07/22 20:49:05, Yaron wrote:
> can you remove these 2 inclues?

Done.

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:10: #include
"content/browser/frame_host/navigation_entry_impl.h"
On 2014/07/22 20:49:05, Yaron wrote:
> can this just use content/public/browser/frame_host/navigation_entry.h?
"favicon_status.h" header is missing in navigation_entry.h, so throwing
compilation issues. Hence keeping navigation_entry_impl.h

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:171:
LoadUrl(params);
On 2014/07/22 20:49:05, Yaron wrote:
> just call LoadUrlWithParams directly and get rid of the wrapper.

Done.

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:180: namespace {
On 2014/07/22 20:49:05, Yaron wrote:
> please move this namespace up above all the member functions.

Done.

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.h (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.h:50: void
LoadUrl(NavigationController::LoadURLParams& params);
On 2014/07/22 20:49:06, Yaron wrote:
> remove

Done.

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:15:
* Holds parameters for ContentViewCore.LoadUrl. Parameters should match
On 2014/07/22 20:49:06, Yaron wrote:
> NavigationController

Done.

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:85:
* Load url without fixing up the url string. Consumers of ContentView are
responsible for
On 2014/07/22 20:49:06, Yaron wrote:
> s/Consumers of ContentView/Clients of NavigationController/

Done.

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:94:
* Clears the ContentViewCore's page history in both the backwards and
On 2014/07/22 20:49:06, Yaron wrote:
> s/ContentViewCore/NavigationController/

Done.

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:100:
* Get a copy of the navigation history of the view.
On 2014/07/22 20:49:06, Yaron wrote:
> Ok, I'll stop commenting on view-related changes. Please take a look at the
rest
> of the diff (and comments) and make sure they make sense. There's no notion of
a
> "view" here.

Done.

https://codereview.chromium.org/406023002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:122:
* Clears SSL preferences for this WebContent Navigation Controller.
On 2014/07/22 20:49:06, Yaron wrote:
> s/WebContent//

Done.

[Yaron, 2014-07-23 18:58:38]

It would help future reviews if you can separate rebasing and responding to
comments into two separate patchsets. That way it's easier to see what's changed
in response to comments.

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:10: #include
"content/browser/frame_host/navigation_entry_impl.h"
On 2014/07/23 11:49:23, AKV wrote:
> On 2014/07/22 20:49:05, Yaron wrote:
> > can this just use content/public/browser/frame_host/navigation_entry.h?
> "favicon_status.h" header is missing in navigation_entry.h, so throwing
> compilation issues. Hence keeping navigation_entry_impl.h
> 

I think you should include it too then. You aren't actually using
NavigationEntryImpl.

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:24: static void
AddNavigationEntryToHistory(
this should still be in an anonymous namespace, just before the "content"
namespace. Sorry if I was unclear

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.h (right):

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.h:14: #include
"content/public/browser/navigation_controller.h"
This is unnecessary. Please remove.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1067:
if (mWebContents != null)
mWebContents.getNavigationController().continuePendingReload();
make this an assert to be consistent with others.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2920:
@Override
Interesting. I think with the new NavigationController, we'd be able to get rid
of the NaivgationClient interface (but can be done in a follow-up).

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2923:
NavigationHistory history = new NavigationHistory();
remove this.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2924:
mWebContents.getNavigationController().getDirectedNavigationHistory(
missing "return"

https://codereview.chromium.org/406023002/diff/40001/content/public/browser/a...
File content/public/browser/android/content_view_core.h (left):

https://codereview.chromium.org/406023002/diff/40001/content/public/browser/a...
content/public/browser/android/content_view_core.h:50: virtual void
LoadUrl(NavigationController::LoadURLParams& params) = 0;
I tried patching this in locally and it doesn't compile. There's a reference to
this from tab_android.cc (which is easy enough to fix). Please make sure to
build locally before requesting a review.

[AKVT, 2014-07-24 07:47:25]

PTAL

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:24: static void
AddNavigationEntryToHistory(
On 2014/07/23 18:58:38, Yaron wrote:
> this should still be in an anonymous namespace, just before the "content"
> namespace. Sorry if I was unclear

Done.

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.h (right):

https://codereview.chromium.org/406023002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.h:14: #include
"content/public/browser/navigation_controller.h"
On 2014/07/23 18:58:38, Yaron wrote:
> This is unnecessary. Please remove.

Done.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1067:
if (mWebContents != null)
mWebContents.getNavigationController().continuePendingReload();
On 2014/07/23 18:58:38, Yaron wrote:
> make this an assert to be consistent with others.

All NavigationController related functions has been made uniform with
mWebContents assert.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2920:
@Override
On 2014/07/23 18:58:38, Yaron wrote:
> Interesting. I think with the new NavigationController, we'd be able to get
rid
> of the NaivgationClient interface (but can be done in a follow-up).
As suggested will take care in follow up patch.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2923:
NavigationHistory history = new NavigationHistory();
On 2014/07/23 18:58:38, Yaron wrote:
> remove this.

Done.

https://codereview.chromium.org/406023002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2924:
mWebContents.getNavigationController().getDirectedNavigationHistory(
On 2014/07/23 18:58:38, Yaron wrote:
> missing "return"

Thanks. Removed unnecessary local variable.

https://codereview.chromium.org/406023002/diff/40001/content/public/browser/a...
File content/public/browser/android/content_view_core.h (left):

https://codereview.chromium.org/406023002/diff/40001/content/public/browser/a...
content/public/browser/android/content_view_core.h:50: virtual void
LoadUrl(NavigationController::LoadURLParams& params) = 0;
On 2014/07/23 18:58:38, Yaron wrote:
> I tried patching this in locally and it doesn't compile. There's a reference
to
> this from tab_android.cc (which is easy enough to fix). Please make sure to
> build locally before requesting a review.
I had done only content_shell_apk build for verifying changes. I will execute
full build in future changes for catching compilation issues.

[Yaron, 2014-07-24 17:34:35]

lgtm

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:22: namespace
content {
This is still in "content". Please make this "namespace {"

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:51: }
}  // namespace

[AKVT, 2014-07-24 17:57:48]

PTAL

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:22: namespace
content {
On 2014/07/24 17:34:34, Yaron wrote:
> This is still in "content". Please make this "namespace {"

Without content namespace, it throws compilation issues for NavigationEntry,
FavIconStatus class access inside this method.

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:51: }
On 2014/07/24 17:34:34, Yaron wrote:
> }  // namespace

Done.

[Yaron, 2014-07-24 18:05:19]

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:22: namespace
content {
On 2014/07/24 17:57:47, AKV wrote:
> On 2014/07/24 17:34:34, Yaron wrote:
> > This is still in "content". Please make this "namespace {"
> 
> Without content namespace, it throws compilation issues for NavigationEntry,
> FavIconStatus class access inside this method.

You need to specify the fully-qualified type (e.g. content::NavigationEntry)

[AKVT, 2014-07-24 18:11:42]

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_android.cc:22: namespace
content {
On 2014/07/24 18:05:18, Yaron wrote:
> On 2014/07/24 17:57:47, AKV wrote:
> > On 2014/07/24 17:34:34, Yaron wrote:
> > > This is still in "content". Please make this "namespace {"
> > 
> > Without content namespace, it throws compilation issues for NavigationEntry,
> > FavIconStatus class access inside this method.
> 
> You need to specify the fully-qualified type (e.g. content::NavigationEntry)

Thanks. I got the problem :)

[AKVT, 2014-07-24 18:15:27]

@nasko Please NavigationController changes.

[nasko, 2014-07-25 07:29:06]

I'm not familiar with what navigation_controller_android.cc is supposed to do.
Can you include some context?

Adding creis@ as he is likely to be interested in those changes.

[AKVT, 2014-07-25 09:06:52]

On 2014/07/25 07:29:06, nasko (in Munich) wrote:
> I'm not familiar with what navigation_controller_android.cc is supposed to do.
> Can you include some context?
> 
> Adding creis@ as he is likely to be interested in those changes.

@nasko, We have been removing the ContentViewCore dependencies from Navigation
Controller related functionalities. So to deprecate calls from
ContentViewCoreImpl, I have moved the logic to Navigation_Controller_android as
per the suggestion from Yaron. Earlier I had kept this functions inside
Web_Contents_android, which is not correct as per the review comment from Yaron.
@Yaron please add your thoughts on this.

[AKVT, 2014-07-25 09:08:40]

On 2014/07/25 09:06:52, AKV wrote:
> On 2014/07/25 07:29:06, nasko (in Munich) wrote:
> > I'm not familiar with what navigation_controller_android.cc is supposed to
do.
> > Can you include some context?
> > 
> > Adding creis@ as he is likely to be interested in those changes.
> 
> @nasko, We have been removing the ContentViewCore dependencies from Navigation
> Controller related functionalities. So to deprecate calls from
> ContentViewCoreImpl, I have moved the logic to Navigation_Controller_android
as
> per the suggestion from Yaron. Earlier I had kept this functions inside
> Web_Contents_android, which is not correct as per the review comment from
Yaron.
> @Yaron please add your thoughts on this.


@nasko
I just moved ownership of navigation_controller functions to
Navigation_Controller_android from ContentViewCoreImpl. PTAL

[nasko, 2014-07-25 09:38:04]

Some nits to address, but overall moving functions from
content_view_core_impl.cc to content_view_core_impl.cc LGTM.

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:145: JNIEnv* env,
jobject obj,
style: each parameter is on new line.

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:189:
params.can_load_local_resources = can_load_local_resources;
nit: move this above, where you do all the unconditional initialization

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:196:
params.is_renderer_initiated = is_renderer_initiated;
nit: same as above comment

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:244:
NavigationControllerAndroid::GetOriginalUrlForActiveNavigationEntry(
nit: This method doesn't use the ActiveEntry anymore, maybe good to rename for
clarity.

[AKVT, 2014-07-25 10:26:59]

PTAL

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:145: JNIEnv* env,
jobject obj,
On 2014/07/25 09:38:04, nasko (in Munich) wrote:
> style: each parameter is on new line.

Done.

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:189:
params.can_load_local_resources = can_load_local_resources;
On 2014/07/25 09:38:03, nasko (in Munich) wrote:
> nit: move this above, where you do all the unconditional initialization

Done.

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:196:
params.is_renderer_initiated = is_renderer_initiated;
On 2014/07/25 09:38:03, nasko (in Munich) wrote:
> nit: same as above comment

Done.

https://codereview.chromium.org/406023002/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_android.cc:244:
NavigationControllerAndroid::GetOriginalUrlForActiveNavigationEntry(
On 2014/07/25 09:38:03, nasko (in Munich) wrote:
> nit: This method doesn't use the ActiveEntry anymore, maybe good to rename for
> clarity.

Done.

[AKVT, 2014-07-25 10:34:18]

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

[commit-bot: I haz the power, 2014-07-25 10:35:32]

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

[commit-bot: I haz the power, 2014-07-25 12:15:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-25 12:22:11]

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

[commit-bot: I haz the power, 2014-07-25 12:22:12]

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

[AKVT, 2014-07-25 13:29:36]

@Yaron and Ted C, Unit test build break I have fixed. But other builds are still
failing due to package path issue. PTAL

[AKVT, 2014-07-25 13:32:06]

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

[commit-bot: I haz the power, 2014-07-25 13:32:52]

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

[Yaron, 2014-07-25 16:05:56]

https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:5:
package org.chromium.content_public.browser;
Note the package. This is part of the public interface of the content layer.
It's a bit confusing because currently most java code in content is in
content/public/android but we're trying to separate the public interface from
the one which is internal to content (e.g. NavigationController is public but
NavigationControllerImpl is internal).

LoadUrlParams needs to be part of the public interface. Please move it to
content/public/android/java/src/org/chromium/content_public/browser/
The same is true for NavigationHistory. That will appease the checkdeps
failures.

[Yaron, 2014-07-25 16:07:07]

On 2014/07/25 16:05:56, Yaron wrote:
>
https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
> (right):
> 
>
https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
>
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:5:
> package org.chromium.content_public.browser;
> Note the package. This is part of the public interface of the content layer.
> It's a bit confusing because currently most java code in content is in
> content/public/android but we're trying to separate the public interface from
> the one which is internal to content (e.g. NavigationController is public but
> NavigationControllerImpl is internal).
> 
> LoadUrlParams needs to be part of the public interface. Please move it to
> content/public/android/java/src/org/chromium/content_public/browser/
> The same is true for NavigationHistory. That will appease the checkdeps
> failures.

You'll need to update the package accordingly.

[commit-bot: I haz the power, 2014-07-25 16:46:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-25 16:50:44]

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

[commit-bot: I haz the power, 2014-07-25 16:50:45]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[AKVT, 2014-07-26 14:25:09]

@Yaron NavigationHistory has dependency on NavigationEntry and LoadUrlParams has
dependency on PageTransitionTypes. Hence I moved those classes to public folder.
PTAL

@benm PTAL android_webview changes.

https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:5:
package org.chromium.content_public.browser;
On 2014/07/25 16:05:56, Yaron wrote:
> Note the package. This is part of the public interface of the content layer.
> It's a bit confusing because currently most java code in content is in
> content/public/android but we're trying to separate the public interface from
> the one which is internal to content (e.g. NavigationController is public but
> NavigationControllerImpl is internal).
> 
> LoadUrlParams needs to be part of the public interface. Please move it to
> content/public/android/java/src/org/chromium/content_public/browser/
> The same is true for NavigationHistory. That will appease the checkdeps
> failures. 

Thank you. I have made changes accordingly, but I can see dependency with
NavigationEntry from NavigationHistory and PageTrnasitionTypes from
LoadUrlParams as well. So I am moving NavigationEntry and PageTrnasitionTypes to
content_public folder for resolving compilation issues. Please suggest your
opinion.

[AKVT, 2014-07-28 05:55:36]

@avi PTAL content content_jni.gypi and content.gyp changes

[Avi (use Gerrit), 2014-07-28 15:20:06]

gyp changes LGTM

[AKVT, 2014-07-28 16:28:14]

@benm and boliu PTAL android_webView changes.

[Yaron, 2014-07-28 22:43:39]

lgtm

https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/406023002/diff/180001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:5:
package org.chromium.content_public.browser;
On 2014/07/26 14:25:08, AKV wrote:
> On 2014/07/25 16:05:56, Yaron wrote:
> > Note the package. This is part of the public interface of the content layer.
> > It's a bit confusing because currently most java code in content is in
> > content/public/android but we're trying to separate the public interface
from
> > the one which is internal to content (e.g. NavigationController is public
but
> > NavigationControllerImpl is internal).
> > 
> > LoadUrlParams needs to be part of the public interface. Please move it to
> > content/public/android/java/src/org/chromium/content_public/browser/
> > The same is true for NavigationHistory. That will appease the checkdeps
> > failures. 
> 
> Thank you. I have made changes accordingly, but I can see dependency with
> NavigationEntry from NavigationHistory and PageTrnasitionTypes from
> LoadUrlParams as well. So I am moving NavigationEntry and PageTrnasitionTypes
to
> content_public folder for resolving compilation issues. Please suggest your
> opinion.

I think what you have is good.

https://codereview.chromium.org/406023002/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/406023002/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java:10:
//import org.chromium.content_public.browser.PageTransitionTypes;
uncomment.

[boliu, 2014-07-28 23:27:16]

You'll have to make android_aosp bot happy. I suspect with all the public java
changes, it's going to require rolling aosp in addition to fixing mk files under
android_webview.

[Yaron, 2014-07-29 00:35:06]

On 2014/07/28 23:27:16, boliu wrote:
> You'll have to make android_aosp bot happy. I suspect with all the public java
> changes, it's going to require rolling aosp in addition to fixing mk files
under
> android_webview.

I filed crbug.com/398263 to track this refactoring. I couldn't assign to you
cause you're not a project member but please use that bug as a tracking bug.

Note to sheriff: https://chrome-internal-review.googlesource.com/#/c/170593/
will fix the internal repo.

[Yaron, 2014-07-29 00:38:50]

On 2014/07/29 00:35:06, Yaron wrote:
> On 2014/07/28 23:27:16, boliu wrote:
> > You'll have to make android_aosp bot happy. I suspect with all the public
java
> > changes, it's going to require rolling aosp in addition to fixing mk files
> under
> > android_webview.
> 
> I filed crbug.com/398263 to track this refactoring. I couldn't assign to you
> cause you're not a project member but please use that bug as a tracking bug.
> 
> Note to sheriff: https://chrome-internal-review.googlesource.com/#/c/170593/
> will fix the internal repo.

Bo: can you point to a similar change?

[boliu, 2014-07-29 00:48:16]

On 2014/07/29 00:38:50, Yaron wrote:
> Bo: can you point to a similar change?

Not yet.

AOSP is public, so external contributors can do that part. Instructions here:
http://www.chromium.org/developers/how-tos/build-instructions-android-webview,
but let me know if you run into problems.

Also update the CL description please. Right now it's pretty confusing and
doesn't say anything.

[AKVT, 2014-07-30 14:47:53]

@Yaron and boliu Thanks for the clarifications.
@boliu - I couldn't figure out the problem from by following the build log.
Could you please tell which file I have to modify to make android_aosp pass.

https://codereview.chromium.org/406023002/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/406023002/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java:10:
//import org.chromium.content_public.browser.PageTransitionTypes;
On 2014/07/28 22:43:39, Yaron wrote:
> uncomment.

Done.

[boliu, 2014-07-31 00:04:53]

Looking at the error from the bot, maybe this file?
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

[AKVT, 2014-08-04 17:21:15]

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

[commit-bot: I haz the power, 2014-08-04 17:21:38]

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

[AKVT, 2014-08-04 17:29:47]

@benm and boliu PTAL android_webView changes.

[boliu, 2014-08-04 17:31:43]

On 2014/08/04 17:29:47, AKV wrote:
> @benm and boliu PTAL android_webView changes.

I don't see any changes you made in android_webview since the last time you
pinged?

[AKVT, 2014-08-04 17:36:14]

On 2014/08/04 17:31:43, boliu wrote:
> On 2014/08/04 17:29:47, AKV wrote:
> > @benm and boliu PTAL android_webView changes.
> 
> I don't see any changes you made in android_webview since the last time you
> pinged?

I was kept
intermediates-dir-for,GYP,shared)/templates/org/chromium/content_public/browser/PageTransitionTypes.java
Correct path inside java_library_common.mk earlier, but still it was failing.
Now I have reverted the changes due to android_aosp failure. Also this
android_aosp is not showing in default list of Build bots. Please suggest
anything extra I have to add in the .mk file.

[boliu, 2014-08-04 17:56:37]

On 2014/08/04 17:36:14, AKV wrote:
> On 2014/08/04 17:31:43, boliu wrote:
> > On 2014/08/04 17:29:47, AKV wrote:
> > > @benm and boliu PTAL android_webView changes.
> > 
> > I don't see any changes you made in android_webview since the last time you
> > pinged?
> 
> I was kept
>
intermediates-dir-for,GYP,shared)/templates/org/chromium/content_public/browser/PageTransitionTypes.java
> Correct path inside java_library_common.mk earlier, but still it was failing.
> Now I have reverted the changes due to android_aosp failure. Also this
> android_aosp is not showing in default list of Build bots. Please suggest
> anything extra I have to add in the .mk file.

I think the change to java_library_common.mk in PS12 was good but not complete.
You also need to add content_public to LOCAL_SRC_FILES in that file.

android_aosp is running on PS13 as part of cq, so not sure what default list you
mean.

And like I said, you can reproduce everything the android_aosp bot does by
following instructions here:
http://www.chromium.org/developers/how-tos/build-instructions-android-webview I
expect there will be other build errors afer this one is fixed. Spamming the
bots for each one isn't a scalable way to fix them.

[commit-bot: I haz the power, 2014-08-04 20:35:55]

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...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-04 20:39:51]

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

[commit-bot: I haz the power, 2014-08-04 20:39:53]

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-08-05 13:42:06]

On 2014/08/04 17:56:37, boliu wrote:
> On 2014/08/04 17:36:14, AKV wrote:
> > On 2014/08/04 17:31:43, boliu wrote:
> > > On 2014/08/04 17:29:47, AKV wrote:
> > > > @benm and boliu PTAL android_webView changes.
> > > 
> > > I don't see any changes you made in android_webview since the last time
you
> > > pinged?
> > 
> > I was kept
> >
>
intermediates-dir-for,GYP,shared)/templates/org/chromium/content_public/browser/PageTransitionTypes.java
> > Correct path inside java_library_common.mk earlier, but still it was
failing.
> > Now I have reverted the changes due to android_aosp failure. Also this
> > android_aosp is not showing in default list of Build bots. Please suggest
> > anything extra I have to add in the .mk file.
> 
> I think the change to java_library_common.mk in PS12 was good but not
complete.
> You also need to add content_public to LOCAL_SRC_FILES in that file.
> 
> android_aosp is running on PS13 as part of cq, so not sure what default list
you
> mean.
> 
> And like I said, you can reproduce everything the android_aosp bot does by
> following instructions here:
> http://www.chromium.org/developers/how-tos/build-instructions-android-webview
I
> expect there will be other build errors afer this one is fixed. Spamming the
> bots for each one isn't a scalable way to fix them.

@boliu - Thanks for reviewing. As i understand from these build instructions
from
http://www.chromium.org/developers/how-tos/build-instructions-android-webview, i
need to make aosp patch also along with current patch and first land aosp patch
and then update that commited CL id in 
android_webview/buildbot/aosp_manifest.xml as part of this CL and check in.
Please let me know if this is fine.

[boliu, 2014-08-05 16:02:48]

On 2014/08/05 13:42:06, AKV wrote:
> @boliu - Thanks for reviewing. As i understand from these build instructions
> from
> http://www.chromium.org/developers/how-tos/build-instructions-android-webview,
i
> need to make aosp patch also along with current patch and first land aosp
patch
> and then update that commited CL id in 
> android_webview/buildbot/aosp_manifest.xml as part of this CL and check in.
> Please let me know if this is fine.

Yes that's fine.

[AKVT, 2014-08-08 07:18:25]

On 2014/08/05 16:02:48, boliu wrote:
> On 2014/08/05 13:42:06, AKV wrote:
> > @boliu - Thanks for reviewing. As i understand from these build instructions
> > from
> >
http://www.chromium.org/developers/how-tos/build-instructions-android-webview,
> i
> > need to make aosp patch also along with current patch and first land aosp
> patch
> > and then update that commited CL id in 
> > android_webview/buildbot/aosp_manifest.xml as part of this CL and check in.
> > Please let me know if this is fine.
> 
> Yes that's fine.

Thanks boliu. I am setting up aosp build locally to clear of all build issues.
Will upload once I clear of all issues.

[AKVT, 2014-08-12 15:06:12]

On 2014/08/08 07:18:25, AKV wrote:
> On 2014/08/05 16:02:48, boliu wrote:
> > On 2014/08/05 13:42:06, AKV wrote:
> > > @boliu - Thanks for reviewing. As i understand from these build
instructions
> > > from
> > >
> http://www.chromium.org/developers/how-tos/build-instructions-android-webview,
> > i
> > > need to make aosp patch also along with current patch and first land aosp
> > patch
> > > and then update that commited CL id in 
> > > android_webview/buildbot/aosp_manifest.xml as part of this CL and check
in.
> > > Please let me know if this is fine.
> > 
> > Yes that's fine.
> 
> Thanks boliu. I am setting up aosp build locally to clear of all build issues.
> Will upload once I clear of all issues.

Just to update you all, I had set up aosp build and building is in progress as
per the new restructure, solving compilation issues is in progress.

[AKVT, 2014-08-20 17:59:00]

On 2014/08/12 15:06:12, AKV wrote:
> On 2014/08/08 07:18:25, AKV wrote:
> > On 2014/08/05 16:02:48, boliu wrote:
> > > On 2014/08/05 13:42:06, AKV wrote:
> > > > @boliu - Thanks for reviewing. As i understand from these build
> instructions
> > > > from
> > > >
> >
http://www.chromium.org/developers/how-tos/build-instructions-android-webview,
> > > i
> > > > need to make aosp patch also along with current patch and first land
aosp
> > > patch
> > > > and then update that commited CL id in 
> > > > android_webview/buildbot/aosp_manifest.xml as part of this CL and check
> in.
> > > > Please let me know if this is fine.
> > > 
> > > Yes that's fine.
> > 
> > Thanks boliu. I am setting up aosp build locally to clear of all build
issues.
> > Will upload once I clear of all issues.
> 
> Just to update you all, I had set up aosp build and building is in progress as
> per the new restructure, solving compilation issues is in progress.

@Yaron & boliu - Just to update you the progress on this issue as follows;
aosp build is failed after applying the patch. While resolving the errors, my
original Chrome Android got corrupted. I am taking fresh chrome code for
integrating with aosp once again.

[AKVT, 2014-08-25 16:21:53]

On 2014/08/20 17:59:00, AKV wrote:
> On 2014/08/12 15:06:12, AKV wrote:
> > On 2014/08/08 07:18:25, AKV wrote:
> > > On 2014/08/05 16:02:48, boliu wrote:
> > > > On 2014/08/05 13:42:06, AKV wrote:
> > > > > @boliu - Thanks for reviewing. As i understand from these build
> > instructions
> > > > > from
> > > > >
> > >
> http://www.chromium.org/developers/how-tos/build-instructions-android-webview,
> > > > i
> > > > > need to make aosp patch also along with current patch and first land
> aosp
> > > > patch
> > > > > and then update that commited CL id in 
> > > > > android_webview/buildbot/aosp_manifest.xml as part of this CL and
check
> > in.
> > > > > Please let me know if this is fine.
> > > > 
> > > > Yes that's fine.
> > > 
> > > Thanks boliu. I am setting up aosp build locally to clear of all build
> issues.
> > > Will upload once I clear of all issues.
> > 
> > Just to update you all, I had set up aosp build and building is in progress
as
> > per the new restructure, solving compilation issues is in progress.
> 
> @Yaron & boliu - Just to update you the progress on this issue as follows;
> aosp build is failed after applying the patch. While resolving the errors, my
> original Chrome Android got corrupted. I am taking fresh chrome code for
> integrating with aosp once again.

@Yaron & boliu - I had successfully made aosp build locally and extracted
android changes. But I do not have a CLA for android. Could you help me to
proceed further.

[boliu, 2014-08-25 16:32:04]

On 2014/08/25 16:21:53, AKV wrote:
> @Yaron & boliu - I had successfully made aosp build locally and extracted
> android changes. But I do not have a CLA for android. Could you help me to
> proceed further.

Does that mean you don't know how to sign the CLA or that you don't want to?

If it's the first, I think the link is
https://android-review.googlesource.com/#/settings/new-agreement

[AKVT, 2014-08-25 16:49:04]

On 2014/08/25 16:32:04, boliu wrote:
> On 2014/08/25 16:21:53, AKV wrote:
> > @Yaron & boliu - I had successfully made aosp build locally and extracted
> > android changes. But I do not have a CLA for android. Could you help me to
> > proceed further.
> 
> Does that mean you don't know how to sign the CLA or that you don't want to?
> 
> If it's the first, I think the link is
> https://android-review.googlesource.com/#/settings/new-agreement

@boliu, I came to know from my colleagues, Android CLA process will consume more
time. I am initiating it for future patches. So for time being it would be good
someone can land the current changes so that I can land the Chromium changes
easily. Please share your thoughts.

[boliu, 2014-08-25 21:08:49]

On 2014/08/25 16:49:04, AKV wrote:
> On 2014/08/25 16:32:04, boliu wrote:
> > On 2014/08/25 16:21:53, AKV wrote:
> > > @Yaron & boliu - I had successfully made aosp build locally and extracted
> > > android changes. But I do not have a CLA for android. Could you help me to
> > > proceed further.
> > 
> > Does that mean you don't know how to sign the CLA or that you don't want to?
> > 
> > If it's the first, I think the link is
> > https://android-review.googlesource.com/#/settings/new-agreement
> 
> @boliu, I came to know from my colleagues, Android CLA process will consume
more
> time.

How long does it take?

> I am initiating it for future patches. So for time being it would be good
> someone can land the current changes so that I can land the Chromium changes
> easily. Please share your thoughts.

I guess you can send the change to me somehow, and I can patch it in. Make sure
compiles with this CL.

[AKVT, 2014-08-26 14:53:01]

On 2014/08/25 21:08:49, boliu wrote:
> On 2014/08/25 16:49:04, AKV wrote:
> > On 2014/08/25 16:32:04, boliu wrote:
> > > On 2014/08/25 16:21:53, AKV wrote:
> > > > @Yaron & boliu - I had successfully made aosp build locally and
extracted
> > > > android changes. But I do not have a CLA for android. Could you help me
to
> > > > proceed further.
> > > 
> > > Does that mean you don't know how to sign the CLA or that you don't want
to?
> > > 
> > > If it's the first, I think the link is
> > > https://android-review.googlesource.com/#/settings/new-agreement
> > 
> > @boliu, I came to know from my colleagues, Android CLA process will consume
> more
> > time.
> 
> How long does it take?
> 
> > I am initiating it for future patches. So for time being it would be good
> > someone can land the current changes so that I can land the Chromium changes
> > easily. Please share your thoughts.
> 
> I guess you can send the change to me somehow, and I can patch it in. Make
sure
> compiles with this CL.

@Boliu & Yaron Thanks for your supports, I will upload new patch set by adopting
latest changes. Later I will share the AOSP with package name changes in
Bugzilla.

[AKVT, 2014-08-26 19:19:53]

@boliu & Yaron PTAL this new patch set, which is build in AOSP environment.
Please take AOSP file changes from
https://code.google.com/p/android/issues/detail?id=75323 (Only import package
name changes for 3 files), and patch it for AOSP submission.

[Yaron, 2014-08-27 00:42:18]

On 2014/08/26 19:19:53, AKV wrote:
> @boliu & Yaron PTAL this new patch set, which is build in AOSP environment.
> Please take AOSP file changes from
> https://code.google.com/p/android/issues/detail?id=75323 (Only import package
> name changes for 3 files), and patch it for AOSP submission.

I think we could simplify this by having a 3-way patch. For the files that were
moved to content_public, keep the old copy and just have a simple subclass. So
something like

// Don't use this, only for temporary backwards compatability with aosp. 
class LoadUrlParams extends org.chromium.content_public.LoadUrlParams {
}

Then AOSP would work for the roll. Then you land a patch to aosp to switch to
the new patch. Once that lands, then you remove the old version. You still need
to sign the CLA for aosp for the middle one but it makes all the various rolls
cleaner.

[boliu, 2014-08-27 03:04:16]

On 2014/08/27 00:42:18, Yaron wrote:
> On 2014/08/26 19:19:53, AKV wrote:
> > @boliu & Yaron PTAL this new patch set, which is build in AOSP environment.
> > Please take AOSP file changes from
> > https://code.google.com/p/android/issues/detail?id=75323 (Only import
package
> > name changes for 3 files), and patch it for AOSP submission.

I didn't get around to doing this.

> 
> I think we could simplify this by having a 3-way patch. For the files that
were
> moved to content_public, keep the old copy and just have a simple subclass. So
> something like
> 
> // Don't use this, only for temporary backwards compatability with aosp. 
> class LoadUrlParams extends org.chromium.content_public.LoadUrlParams {
> }
> 
> Then AOSP would work for the roll. Then you land a patch to aosp to switch to
> the new patch. Once that lands, then you remove the old version. You still
need
> to sign the CLA for aosp for the middle one but it makes all the various rolls
> cleaner.

We can roll aosp atomically in the same chromium commit, so this isn't
technically needed.

[AKVT, 2014-08-27 03:15:31]

On 2014/08/27 03:04:16, boliu wrote:
> On 2014/08/27 00:42:18, Yaron wrote:
> > On 2014/08/26 19:19:53, AKV wrote:
> > > @boliu & Yaron PTAL this new patch set, which is build in AOSP
environment.
> > > Please take AOSP file changes from
> > > https://code.google.com/p/android/issues/detail?id=75323 (Only import
> package
> > > name changes for 3 files), and patch it for AOSP submission.
> 
> I didn't get around to doing this.
> 
> > 
> > I think we could simplify this by having a 3-way patch. For the files that
> were
> > moved to content_public, keep the old copy and just have a simple subclass.
So
> > something like
> > 
> > // Don't use this, only for temporary backwards compatability with aosp. 
> > class LoadUrlParams extends org.chromium.content_public.LoadUrlParams {
> > }
> > 
> > Then AOSP would work for the roll. Then you land a patch to aosp to switch
to
> > the new patch. Once that lands, then you remove the old version. You still
> need
> > to sign the CLA for aosp for the middle one but it makes all the various
rolls
> > cleaner.
> 
> We can roll aosp atomically in the same chromium commit, so this isn't
> technically needed.

@Yaron and Boliu - As we agreed time being Boliu will land the AOSP changes
until I get the CLA for android. So that I can land the Chromium changes without
any problems.
Or as Yaron suggested to make AOSP happy, shall I keep dummy classes in
content/browser folder for LoadUrlParams, NavigationEtry, NavigationHistory and
PageTransitionTemplate and submit this current patch ?
Please share your opinion

[AKVT, 2014-08-27 16:38:09]

On 2014/08/27 03:15:31, AKV wrote:
> On 2014/08/27 03:04:16, boliu wrote:
> > On 2014/08/27 00:42:18, Yaron wrote:
> > > On 2014/08/26 19:19:53, AKV wrote:
> > > > @boliu & Yaron PTAL this new patch set, which is build in AOSP
> environment.
> > > > Please take AOSP file changes from
> > > > https://code.google.com/p/android/issues/detail?id=75323 (Only import
> > package
> > > > name changes for 3 files), and patch it for AOSP submission.
> > 
> > I didn't get around to doing this.
> > 
> > > 
> > > I think we could simplify this by having a 3-way patch. For the files that
> > were
> > > moved to content_public, keep the old copy and just have a simple
subclass.
> So
> > > something like
> > > 
> > > // Don't use this, only for temporary backwards compatability with aosp. 
> > > class LoadUrlParams extends org.chromium.content_public.LoadUrlParams {
> > > }
> > > 
> > > Then AOSP would work for the roll. Then you land a patch to aosp to switch
> to
> > > the new patch. Once that lands, then you remove the old version. You still
> > need
> > > to sign the CLA for aosp for the middle one but it makes all the various
> rolls
> > > cleaner.
> > 
> > We can roll aosp atomically in the same chromium commit, so this isn't
> > technically needed.
> 
> @Yaron and Boliu - As we agreed time being Boliu will land the AOSP changes
> until I get the CLA for android. So that I can land the Chromium changes
without
> any problems.
> Or as Yaron suggested to make AOSP happy, shall I keep dummy classes in
> content/browser folder for LoadUrlParams, NavigationEtry, NavigationHistory
and
> PageTransitionTemplate and submit this current patch ?
> Please share your opinion

@Yaron Boliu Could you please share your opinion on my comments above ?

[boliu, 2014-08-27 17:11:08]

Do what Yaron said if you want this to go in today.

We (the android webview team) messed up. It's currently impossible to roll aosp.
Might take a day or two to get that sorted out. Sorry.

Side note: in general, give reviewers at least 24 hours to respond, especially
when review is across time zones.

[AKVT, 2014-08-27 18:59:01]

On 2014/08/27 17:11:08, boliu wrote:
> Do what Yaron said if you want this to go in today.
> 
> We (the android webview team) messed up. It's currently impossible to roll
aosp.
> Might take a day or two to get that sorted out. Sorry.
> 
> Side note: in general, give reviewers at least 24 hours to respond, especially
> when review is across time zones.

Thanks boliu. I will try to compile AOSP once again by keeping duplicate copies
as suggested. And will upload a new patch set. Sorry for pinging without
considering Time Zone. I will restrict it by checking time lines.

[AKVT, 2014-09-01 10:41:13]

On 2014/08/27 18:59:01, AKV wrote:
> On 2014/08/27 17:11:08, boliu wrote:
> > Do what Yaron said if you want this to go in today.
> > 
> > We (the android webview team) messed up. It's currently impossible to roll
> aosp.
> > Might take a day or two to get that sorted out. Sorry.
> > 
> > Side note: in general, give reviewers at least 24 hours to respond,
especially
> > when review is across time zones.
> 
> Thanks boliu. I will try to compile AOSP once again by keeping duplicate
copies
> as suggested. And will upload a new patch set. Sorry for pinging without
> considering Time Zone. I will restrict it by checking time lines.

frameworks/webview/chromium/java/com/android/webview/chromium/WebViewChromium.java:1143:
error: no suitable constructor found for
WebBackForwardListChromium(org.chromium.content_public.browser.NavigationHistory)
        return new WebBackForwardListChromium(
               ^
    constructor
WebBackForwardListChromium.WebBackForwardListChromium(List<WebHistoryItemChromium>,int)
is not applicable
      (actual and formal argument lists differ in length)
    constructor
WebBackForwardListChromium.WebBackForwardListChromium(org.chromium.content.browser.NavigationHistory)
is not applicable
      (actual argument org.chromium.content_public.browser.NavigationHistory
cannot be converted to org.chromium.content.browser.NavigationHistory by method
invocation conversion)


To solve this error, I have to manually type cast
org.chromium.content_public.browser.NavigationHistory  to
org.chromium.content.browser.NavigationHistory  inside this file
WebBackForwardListChromium.java. Any casting outside of this file is not solving
the issue.

So could you please roll AOSP this time for me. Next time I will roll myself, as
my CLA will be ready by that time.
Based on your reply I will rebase current patch set.

[AKVT, 2014-09-02 18:04:58]

On 2014/09/01 10:41:13, AKV wrote:
> On 2014/08/27 18:59:01, AKV wrote:
> > On 2014/08/27 17:11:08, boliu wrote:
> > > Do what Yaron said if you want this to go in today.
> > > 
> > > We (the android webview team) messed up. It's currently impossible to roll
> > aosp.
> > > Might take a day or two to get that sorted out. Sorry.
> > > 
> > > Side note: in general, give reviewers at least 24 hours to respond,
> especially
> > > when review is across time zones.
> > 
> > Thanks boliu. I will try to compile AOSP once again by keeping duplicate
> copies
> > as suggested. And will upload a new patch set. Sorry for pinging without
> > considering Time Zone. I will restrict it by checking time lines.
> 
>
frameworks/webview/chromium/java/com/android/webview/chromium/WebViewChromium.java:1143:
> error: no suitable constructor found for
>
WebBackForwardListChromium(org.chromium.content_public.browser.NavigationHistory)
>         return new WebBackForwardListChromium(
>                ^
>     constructor
>
WebBackForwardListChromium.WebBackForwardListChromium(List<WebHistoryItemChromium>,int)
> is not applicable
>       (actual and formal argument lists differ in length)
>     constructor
>
WebBackForwardListChromium.WebBackForwardListChromium(org.chromium.content.browser.NavigationHistory)
> is not applicable
>       (actual argument org.chromium.content_public.browser.NavigationHistory
> cannot be converted to org.chromium.content.browser.NavigationHistory by
method
> invocation conversion)
> 
> 
> To solve this error, I have to manually type cast
> org.chromium.content_public.browser.NavigationHistory  to
> org.chromium.content.browser.NavigationHistory  inside this file
> WebBackForwardListChromium.java. Any casting outside of this file is not
solving
> the issue.
> 
> So could you please roll AOSP this time for me. Next time I will roll myself,
as
> my CLA will be ready by that time.
> Based on your reply I will rebase current patch set.

@boliu and Yaron - Could you please update the plan for AOSP rolling for landing
this patch.

[boliu, 2014-09-02 19:31:07]

I'm a bit busy at the moment and don't have time to look into aosp issues. I'll
ask around if the issue is resolved.

On 2014/09/01 10:41:13, AKV wrote:
> On 2014/08/27 18:59:01, AKV wrote:
> > On 2014/08/27 17:11:08, boliu wrote:
> > > Do what Yaron said if you want this to go in today.
> > > 
> > > We (the android webview team) messed up. It's currently impossible to roll
> > aosp.
> > > Might take a day or two to get that sorted out. Sorry.
> > > 
> > > Side note: in general, give reviewers at least 24 hours to respond,
> especially
> > > when review is across time zones.
> > 
> > Thanks boliu. I will try to compile AOSP once again by keeping duplicate
> copies
> > as suggested. And will upload a new patch set. Sorry for pinging without
> > considering Time Zone. I will restrict it by checking time lines.
> 
>
frameworks/webview/chromium/java/com/android/webview/chromium/WebViewChromium.java:1143:
> error: no suitable constructor found for
>
WebBackForwardListChromium(org.chromium.content_public.browser.NavigationHistory)
>         return new WebBackForwardListChromium(
>                ^
>     constructor
>
WebBackForwardListChromium.WebBackForwardListChromium(List<WebHistoryItemChromium>,int)
> is not applicable
>       (actual and formal argument lists differ in length)
>     constructor
>
WebBackForwardListChromium.WebBackForwardListChromium(org.chromium.content.browser.NavigationHistory)
> is not applicable
>       (actual argument org.chromium.content_public.browser.NavigationHistory
> cannot be converted to org.chromium.content.browser.NavigationHistory by
method
> invocation conversion)
> 
> 
> To solve this error, I have to manually type cast
> org.chromium.content_public.browser.NavigationHistory  to
> org.chromium.content.browser.NavigationHistory  inside this file
> WebBackForwardListChromium.java. Any casting outside of this file is not
solving
> the issue.

So you are *not* taking Yaron's suggestion then?

ie instead of deleting
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java,
put this in:

"""

package org.chromium.content.browser;

public class LoadUrlParams extends
org.chromium.content_public.browser.LoadUrlParams {
  public static final int LOAD_TYPE_DEFAULT =
org.chromium.content_public.browser.LoadUrlParams.LOAD_TYPE_DEFAULT;
  ...
}
"""

> 
> So could you please roll AOSP this time for me. Next time I will roll myself,
as
> my CLA will be ready by that time.
> Based on your reply I will rebase current patch set.

[boliu, 2014-09-02 19:34:21]

On 2014/09/02 19:31:07, boliu wrote:
> I'm a bit busy at the moment and don't have time to look into aosp issues.
I'll
> ask around if the issue is resolved.
> 
> On 2014/09/01 10:41:13, AKV wrote:
> > On 2014/08/27 18:59:01, AKV wrote:
> > > On 2014/08/27 17:11:08, boliu wrote:
> > > > Do what Yaron said if you want this to go in today.
> > > > 
> > > > We (the android webview team) messed up. It's currently impossible to
roll
> > > aosp.
> > > > Might take a day or two to get that sorted out. Sorry.
> > > > 
> > > > Side note: in general, give reviewers at least 24 hours to respond,
> > especially
> > > > when review is across time zones.
> > > 
> > > Thanks boliu. I will try to compile AOSP once again by keeping duplicate
> > copies
> > > as suggested. And will upload a new patch set. Sorry for pinging without
> > > considering Time Zone. I will restrict it by checking time lines.
> > 
> >
>
frameworks/webview/chromium/java/com/android/webview/chromium/WebViewChromium.java:1143:
> > error: no suitable constructor found for
> >
>
WebBackForwardListChromium(org.chromium.content_public.browser.NavigationHistory)
> >         return new WebBackForwardListChromium(
> >                ^
> >     constructor
> >
>
WebBackForwardListChromium.WebBackForwardListChromium(List<WebHistoryItemChromium>,int)
> > is not applicable
> >       (actual and formal argument lists differ in length)
> >     constructor
> >
>
WebBackForwardListChromium.WebBackForwardListChromium(org.chromium.content.browser.NavigationHistory)
> > is not applicable
> >       (actual argument org.chromium.content_public.browser.NavigationHistory
> > cannot be converted to org.chromium.content.browser.NavigationHistory by
> method
> > invocation conversion)

Oh wait, I misunderstood. You'll need a dummy NavigationHistory too.

Basically you can't remove anything public if you want to take Yaron's
suggestion and do 3-sided patchs.

> > 
> > 
> > To solve this error, I have to manually type cast
> > org.chromium.content_public.browser.NavigationHistory  to
> > org.chromium.content.browser.NavigationHistory  inside this file
> > WebBackForwardListChromium.java. Any casting outside of this file is not
> solving
> > the issue.
> 
> So you are *not* taking Yaron's suggestion then?
> 
> ie instead of deleting
>
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java,
> put this in:
> 
> """
> 
> package org.chromium.content.browser;
> 
> public class LoadUrlParams extends
> org.chromium.content_public.browser.LoadUrlParams {
>   public static final int LOAD_TYPE_DEFAULT =
> org.chromium.content_public.browser.LoadUrlParams.LOAD_TYPE_DEFAULT;
>   ...
> }
> """
> 
> > 
> > So could you please roll AOSP this time for me. Next time I will roll
myself,
> as
> > my CLA will be ready by that time.
> > Based on your reply I will rebase current patch set.

[boliu, 2014-09-03 20:34:14]

AOSP patch here: https://android-review.googlesource.com/106456

You can wait until that's submitted, get the new sha1, and update
android_webview/buildbot/aosp_manifest.xml

And finally android_webview lgtm once you've done all that.

[boliu, 2014-09-04 01:26:13]

On 2014/09/03 20:34:14, boliu wrote:
> AOSP patch here: https://android-review.googlesource.com/106456
> 
> You can wait until that's submitted, get the new sha1, and update
> android_webview/buildbot/aosp_manifest.xml
> 
> And finally android_webview lgtm once you've done all that.

AOSP side is in. Just update the frameworks/webview sha1 to
8dc9a9d1217f84a7f9b0554f22a826394eabc856 and android_aosp bot should be green.
Then this is good to go.

[AKVT, 2014-09-04 06:55:00]

On 2014/09/04 01:26:13, boliu wrote:
> On 2014/09/03 20:34:14, boliu wrote:
> > AOSP patch here: https://android-review.googlesource.com/106456
> > 
> > You can wait until that's submitted, get the new sha1, and update
> > android_webview/buildbot/aosp_manifest.xml
> > 
> > And finally android_webview lgtm once you've done all that.
> 
> AOSP side is in. Just update the frameworks/webview sha1 to
> 8dc9a9d1217f84a7f9b0554f22a826394eabc856 and android_aosp bot should be green.
> Then this is good to go.

Thank you very much boliu and Yaron. I am rebasing the patch with AOSP SHA1 for
committing.

[AKVT, 2014-09-04 09:01:04]

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

[commit-bot: I haz the power, 2014-09-04 09:02:12]

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

[commit-bot: I haz the power, 2014-09-04 09:45:23]

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

[commit-bot: I haz the power, 2014-09-04 09:45:25]

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

[AKVT, 2014-09-04 14:55:25]

On 2014/09/04 09:45:25, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_clang_dbg_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

@Yaron, Boliu and Ted

The following files contain a third-party license but are not in a listed
third-party directory and are not whitelisted. You must add the following files
to the whitelist.
content/browser/renderer_host/web_input_event_aurax11.cc
This failure is not related to this patch I think. Please correct me if I am
wrong.

FAILED: if [ ! -e lib/libbase.so -o ! -e lib/libbase.so.TOC ]; then
/mnt/scratch0/b_used/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -m32
-Wl,--no-as-needed -o lib/libbase.so -Wl,-soname=libbase.so @lib/libbase.so.rsp
&& { readelf -d lib/libbase.so | grep SONAME ; nm -gD -f p lib/libbase.so | cut
-f1-2 -d' '; } > lib/libbase.so.TOC; else /mnt/scratch0/b_used/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -m32
-Wl,--no-as-needed -o lib/libbase.so -Wl,-soname=libbase.so @lib/libbase.so.rsp
&& { readelf -d lib/libbase.so | grep SONAME ; nm -gD -f p lib/libbase.so | cut
-f1-2 -d' '; } > lib/libbase.so.tmp && if ! cmp -s lib/libbase.so.tmp
lib/libbase.so.TOC; then mv lib/libbase.so.tmp lib/libbase.so.TOC ; fi; fi
/usr/bin/ld: cannot find -lgmodule-2.0
/usr/bin/ld: cannot find -lgobject-2.0
/usr/bin/ld: cannot find -lgthread-2.0
/usr/bin/ld: cannot find -lglib-2.0
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

This failure also not sure why happened.

New warnings.
Please fix, or perhaps add to
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
--------------------------------------------------------------------------------
M V EI2: org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
may expose internal representation by storing an externally mutable object into
LoadUrlParams.mPostData  At LoadUrlParams.java
M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData() may
expose int

This is failure is introduced by this patch. Could you please guide how we can
tackle this warnings. Can I keep /* package */ to calm down this warning instead
of public API ?

[boliu, 2014-09-04 15:40:22]

On 2014/09/04 14:55:25, AKV wrote:
> On 2014/09/04 09:45:25, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_clang_dbg_recipe on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
> 
> @Yaron, Boliu and Ted
> 
> The following files contain a third-party license but are not in a listed
> third-party directory and are not whitelisted. You must add the following
files
> to the whitelist.
> content/browser/renderer_host/web_input_event_aurax11.cc
> This failure is not related to this patch I think. Please correct me if I am
> wrong.
> 
> FAILED: if [ ! -e lib/libbase.so -o ! -e lib/libbase.so.TOC ]; then
> /mnt/scratch0/b_used/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
> -Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -m32
> -Wl,--no-as-needed -o lib/libbase.so -Wl,-soname=libbase.so
@lib/libbase.so.rsp
> && { readelf -d lib/libbase.so | grep SONAME ; nm -gD -f p lib/libbase.so |
cut
> -f1-2 -d' '; } > lib/libbase.so.TOC; else
/mnt/scratch0/b_used/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
> -Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -m32
> -Wl,--no-as-needed -o lib/libbase.so -Wl,-soname=libbase.so
@lib/libbase.so.rsp
> && { readelf -d lib/libbase.so | grep SONAME ; nm -gD -f p lib/libbase.so |
cut
> -f1-2 -d' '; } > lib/libbase.so.tmp && if ! cmp -s lib/libbase.so.tmp
> lib/libbase.so.TOC; then mv lib/libbase.so.tmp lib/libbase.so.TOC ; fi; fi
> /usr/bin/ld: cannot find -lgmodule-2.0
> /usr/bin/ld: cannot find -lgobject-2.0
> /usr/bin/ld: cannot find -lgthread-2.0
> /usr/bin/ld: cannot find -lglib-2.0
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> ninja: build stopped: subcommand failed.
> 
> This failure also not sure why happened.

That's unrelated to this. It's been failing for awhile now for most builds:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

> 
> New warnings.
> Please fix, or perhaps add to
>
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
>
--------------------------------------------------------------------------------
> M V EI2: org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
> may expose internal representation by storing an externally mutable object
into
> LoadUrlParams.mPostData  At LoadUrlParams.java
> M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData() may
> expose int
> 
> This is failure is introduced by this patch. Could you please guide how we can
> tackle this warnings. Can I keep /* package */ to calm down this warning
instead
> of public API ?

Update src/build/android/findbugs_filter/findbugs_known_bugs.txt

That make presubmit complain, but let's see first

[AKVT, 2014-09-04 16:58:50]

@Boliu, I had corrected the Warning file. PTAL

[AKVT, 2014-09-04 17:05:50]

On 2014/09/04 15:40:22, boliu wrote:
> On 2014/09/04 14:55:25, AKV wrote:
> > On 2014/09/04 09:45:25, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   android_clang_dbg_recipe on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
> > 
> > @Yaron, Boliu and Ted
> > 
> > The following files contain a third-party license but are not in a listed
> > third-party directory and are not whitelisted. You must add the following
> files
> > to the whitelist.
> > content/browser/renderer_host/web_input_event_aurax11.cc
> > This failure is not related to this patch I think. Please correct me if I am
> > wrong.
> > 
> > FAILED: if [ ! -e lib/libbase.so -o ! -e lib/libbase.so.TOC ]; then
> > /mnt/scratch0/b_used/build/goma/gomacc
> > ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
> > -Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -m32
> > -Wl,--no-as-needed -o lib/libbase.so -Wl,-soname=libbase.so
> @lib/libbase.so.rsp
> > && { readelf -d lib/libbase.so | grep SONAME ; nm -gD -f p lib/libbase.so |
> cut
> > -f1-2 -d' '; } > lib/libbase.so.TOC; else
> /mnt/scratch0/b_used/build/goma/gomacc
> > ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
> > -Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -m32
> > -Wl,--no-as-needed -o lib/libbase.so -Wl,-soname=libbase.so
> @lib/libbase.so.rsp
> > && { readelf -d lib/libbase.so | grep SONAME ; nm -gD -f p lib/libbase.so |
> cut
> > -f1-2 -d' '; } > lib/libbase.so.tmp && if ! cmp -s lib/libbase.so.tmp
> > lib/libbase.so.TOC; then mv lib/libbase.so.tmp lib/libbase.so.TOC ; fi; fi
> > /usr/bin/ld: cannot find -lgmodule-2.0
> > /usr/bin/ld: cannot find -lgobject-2.0
> > /usr/bin/ld: cannot find -lgthread-2.0
> > /usr/bin/ld: cannot find -lglib-2.0
> > clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
> > ninja: build stopped: subcommand failed.
> > 
> > This failure also not sure why happened.
> 
> That's unrelated to this. It's been failing for awhile now for most builds:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> > 
> > New warnings.
> > Please fix, or perhaps add to
> >
>
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
> >
>
--------------------------------------------------------------------------------
> > M V EI2:
org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
> > may expose internal representation by storing an externally mutable object
> into
> > LoadUrlParams.mPostData  At LoadUrlParams.java
> > M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData() may
> > expose int
> > 
> > This is failure is introduced by this patch. Could you please guide how we
can
> > tackle this warnings. Can I keep /* package */ to calm down this warning
> instead
> > of public API ?
> 
> Update src/build/android/findbugs_filter/findbugs_known_bugs.txt
> 
> That make presubmit complain, but let's see first

Thanks boliu. I am trying to commit with mentioned changes above.

[AKVT, 2014-09-04 17:06:48]

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

[commit-bot: I haz the power, 2014-09-04 17:08:47]

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

[boliu, 2014-09-04 17:10:54]

On 2014/09/04 16:58:50, AKV wrote:
> @Boliu, I had corrected the Warning file. PTAL

You've only moved LoadUrlParams, so there are new warnings as well which you
need to add. From the bot:

********************************************************************************
New warnings.
Please fix, or perhaps add to
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
--------------------------------------------------------------------------------
M V EI2: org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
may expose internal representation by storing an externally mutable object into
LoadUrlParams.mPostData  At LoadUrlParams.java
M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData() may
expose internal representation by returning LoadUrlParams.mPostData  At
LoadUrlParams.java
--------------------------------------------------------------------------------

[AKVT, 2014-09-04 17:19:39]

On 2014/09/04 17:10:54, boliu wrote:
> On 2014/09/04 16:58:50, AKV wrote:
> > @Boliu, I had corrected the Warning file. PTAL
> 
> You've only moved LoadUrlParams, so there are new warnings as well which you
> need to add. From the bot:
> 
>
********************************************************************************
> New warnings.
> Please fix, or perhaps add to
>
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
>
--------------------------------------------------------------------------------
> M V EI2: org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
> may expose internal representation by storing an externally mutable object
into
> LoadUrlParams.mPostData  At LoadUrlParams.java
> M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData() may
> expose internal representation by returning LoadUrlParams.mPostData  At
> LoadUrlParams.java
>
--------------------------------------------------------------------------------

@boliu - This warning messages was already present inside the file. I had just
removed. During presubmit time, it was telling, these file entries are just for
removal only not for addition. Could you please correct me if anything else I
have to do

[Yaron, 2014-09-04 17:26:47]

On 2014/09/04 17:19:39, AKV wrote:
> On 2014/09/04 17:10:54, boliu wrote:
> > On 2014/09/04 16:58:50, AKV wrote:
> > > @Boliu, I had corrected the Warning file. PTAL
> > 
> > You've only moved LoadUrlParams, so there are new warnings as well which you
> > need to add. From the bot:
> > 
> >
>
********************************************************************************
> > New warnings.
> > Please fix, or perhaps add to
> >
>
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
> >
>
--------------------------------------------------------------------------------
> > M V EI2:
org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
> > may expose internal representation by storing an externally mutable object
> into
> > LoadUrlParams.mPostData  At LoadUrlParams.java
> > M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData() may
> > expose internal representation by returning LoadUrlParams.mPostData  At
> > LoadUrlParams.java
> >
>
--------------------------------------------------------------------------------
> 
> @boliu - This warning messages was already present inside the file. I had just
> removed. During presubmit time, it was telling, these file entries are just
for
> removal only not for addition. Could you please correct me if anything else I
> have to do

we need to land this for you. We'll need to move the findbugs exclusions to the
new class (you haven't fixed the issue, so it should just move from the old
class to the new class) but the presubmit won't let you so it needs to be
dcommitted. I'll land this for you but I don't think I can do it today.

[AKVT, 2014-09-04 17:38:56]

On 2014/09/04 17:26:47, Yaron wrote:
> On 2014/09/04 17:19:39, AKV wrote:
> > On 2014/09/04 17:10:54, boliu wrote:
> > > On 2014/09/04 16:58:50, AKV wrote:
> > > > @Boliu, I had corrected the Warning file. PTAL
> > > 
> > > You've only moved LoadUrlParams, so there are new warnings as well which
you
> > > need to add. From the bot:
> > > 
> > >
> >
>
********************************************************************************
> > > New warnings.
> > > Please fix, or perhaps add to
> > >
> >
>
/mnt/scratch0/b_used/build/slave/android_clang_dbg_recipe/build/src/build/android/findbugs_filter/findbugs_known_bugs.txt
> > >
> >
>
--------------------------------------------------------------------------------
> > > M V EI2:
> org.chromium.content_public.browser.LoadUrlParams.setPostData(byte[])
> > > may expose internal representation by storing an externally mutable object
> > into
> > > LoadUrlParams.mPostData  At LoadUrlParams.java
> > > M V EI: org.chromium.content_public.browser.LoadUrlParams.getPostData()
may
> > > expose internal representation by returning LoadUrlParams.mPostData  At
> > > LoadUrlParams.java
> > >
> >
>
--------------------------------------------------------------------------------
> > 
> > @boliu - This warning messages was already present inside the file. I had
just
> > removed. During presubmit time, it was telling, these file entries are just
> for
> > removal only not for addition. Could you please correct me if anything else
I
> > have to do
> 
> we need to land this for you. We'll need to move the findbugs exclusions to
the
> new class (you haven't fixed the issue, so it should just move from the old
> class to the new class) but the presubmit won't let you so it needs to be
> dcommitted. I'll land this for you but I don't think I can do it today.

@Yaron & boliu - I missed to check the path correctly. I updated it correctly
now inside findbugs_known_bugs.txt. If presubmit fails, please help me to gain
LGTM once again.

[boliu, 2014-09-04 17:45:18]

The CQ bit was checked by boliu@chromium.org

[boliu, 2014-09-04 17:46:22]

On 2014/09/04 17:38:56, AKV wrote:
> @Yaron & boliu - I missed to check the path correctly. I updated it correctly
> now inside findbugs_known_bugs.txt. If presubmit fails, please help me to gain
> LGTM once again.

The issue is not owner approval. Please learn to read bot output. It says

"""
** Presubmit Warnings **
Following files should only contain deletions.
  build/android/findbugs_filter/findbugs_known_bugs.txt
"""

So cq won't land this because of presubmit warnings, so just wait for Yaron to
do it for you. I +cq anyway to run through other bots.

[commit-bot: I haz the power, 2014-09-04 17:49:36]

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

[commit-bot: I haz the power, 2014-09-04 18:06:22]

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

[commit-bot: I haz the power, 2014-09-04 18:06:24]

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-09-05 10:53:23]

@boliu and Yaron - I have corrected the findbugs_known_bugs.txt. Once Yaron
lands the changes, this will be ready to move. PTAL

[Yaron, 2014-09-05 17:50:06]

Committed patchset #19 (id:360001) manually as 4502cc6 (presubmit successful).

[AKVT, 2014-09-06 07:39:19]

On 2014/09/05 17:50:06, Yaron wrote:
> Committed patchset #19 (id:360001) manually as 4502cc6 (presubmit successful).

#Yaron, boliu, Avi, nasko, Ted - Thank you very much for your overwhelmed and
encouraging support to land this plant. Finally we set the base and got the
first fruit. Looking forward to generate more and more nice fruits from here.
Once again thank you very much :)

[commit-bot: I haz the power, 2014-09-10 03:40:35]

Patchset 19 (id:??) landed as
https://crrev.com/4502cc6303f4aabd05e7fef6fbfaf4dfd1600bb6
Cr-Commit-Position: refs/heads/master@{#293550}