Implement WebViewDatabase's hasFormData API for chromium based webview.

Implement WebViewDatabase's hasFormData API for chromium based webview.

BUG=b/6234236

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

[sgurun-gerrit only, 2013-04-26 16:32:51]

Hey Ben,

this is a very rough patch for implementing hasFormData() call. Not ready for a
real review - will implement unit tests, fix issues here and there, etc, but if
you have 5 minutes to take a quick look at give some feedback about overall
structure, I appreciate it.

[sgurun-gerrit only, 2013-04-26 16:34:38]

this also implements some missing pieces that I left out in previous
https://chromiumcodereview.appspot.com/13902021/

[Ilya Sherman, 2013-04-27 00:05:55]

webdata/ changes LGTM with one nit.

Is there any particular reason that the corresponding bug is not on crbug.com?

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
File components/autofill/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
components/autofill/browser/webdata/autofill_table.cc:472: } else {
nit: No else stmt after a return stmt, please.

[joth, 2013-04-27 01:02:53]

On 2013/04/27 00:05:55, Ilya Sherman wrote:
> webdata/ changes LGTM with one nit.
> 
> Is there any particular reason that the corresponding bug is not on crbug.com?
> 
the "implementation API X" tracking for this project was started many months
(years) ago when the android port was not yet public, for progress tracking
reasons we've kept them all in one place.
Going forward, new issues raised are generally appearing on crbug.

[sgurun-gerrit only, 2013-04-29 17:55:15]

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
File components/autofill/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
components/autofill/browser/webdata/autofill_table.cc:472: } else {
On 2013/04/27 00:05:55, Ilya Sherman wrote:
> nit: No else stmt after a return stmt, please.

Done.

[benm (inactive), 2013-04-29 18:00:03]

Thanks Selim! Looks good, I think it would be quite nice to refactor the
AwFormDatabase class into AwFormDatabaseService, wdyt?

https://codereview.chromium.org/14503010/diff/8001/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/DEPS#newco...
android_webview/DEPS:12: "+components/webdata/common",
do these need to be at the top level?

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
android_webview/browser/aw_form_database_service.cc:27: : completion_(false,
false) {
missing some members.

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
File android_webview/browser/aw_form_database_service.h (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
android_webview/browser/aw_form_database_service.h:21: class
AwFormDatabaseService : public WebDataServiceConsumer {
Can we refactor the AwFormDatabase class from the last CL for ClearData into
this class?

https://codereview.chromium.org/14503010/diff/8001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwFormDatabase.java
(right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwFormDatabase.java:16:
public static boolean hasFormData() {
Rather than having these as statics, can they call through to the
AwFormDatabaseService directly (i.e. they have a ptr to the native instance,
like most of our JNI)

Then this class would be AwForDatabaseService rather than AwFormDatabase.

https://codereview.chromium.org/14503010/diff/8001/android_webview/native/aw_...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/native/aw_...
android_webview/native/aw_form_database.cc:16: // static
anonymous namespace?

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
File components/autofill/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
components/autofill/browser/webdata/autofill_table.cc:472: } else {
return s.Step() ? s.ColumnInt(0) > 0 : false;

looks quite neat. You lose the NOTREACHED though I guess.

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
File components/autofill/browser/webdata/autofill_table_unittest.cc (right):

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
components/autofill/browser/webdata/autofill_table_unittest.cc:143:
EXPECT_FALSE(table_->HasFormElements());
+ a test where we EXPECT_TRUE ? :)

[sgurun-gerrit only, 2013-04-29 19:32:38]

https://codereview.chromium.org/14503010/diff/8001/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/DEPS#newco...
android_webview/DEPS:12: "+components/webdata/common",
I think now they do since there are includes from
android_webview/browser/aw_form_database_service to both targets.

On 2013/04/29 18:00:03, benm wrote:
> do these need to be at the top level?

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
android_webview/browser/aw_form_database_service.cc:27: : completion_(false,
false) {
On 2013/04/29 18:00:03, benm wrote:
> missing some members.

Done.

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
File android_webview/browser/aw_form_database_service.h (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/browser/aw...
android_webview/browser/aw_form_database_service.h:21: class
AwFormDatabaseService : public WebDataServiceConsumer {
makes sense. Will try to move most of the logic here.

On 2013/04/29 18:00:03, benm wrote:
> Can we refactor the AwFormDatabase class from the last CL for ClearData into
> this class?

https://codereview.chromium.org/14503010/diff/8001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwFormDatabase.java
(right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwFormDatabase.java:16:
public static boolean hasFormData() {
We could. I think AwBrowserContext is the proper way to get this pointer since
these objects are owned by the AwBrowserContext, however at the moment
awbrowsercontext java and native sides are disconnected. I think Joth has a TODO
for this. 

My feeling is to leave this as static, with a TODO explaining how we are
thinking of solving it, and then handling this after that. Or I can address
AwBrowserContext right away. I am fine with both. Let me know if you have a
preference.

On 2013/04/29 18:00:03, benm wrote:
> Rather than having these as statics, can they call through to the
> AwFormDatabaseService directly (i.e. they have a ptr to the native instance,
> like most of our JNI)
> 
> Then this class would be AwForDatabaseService rather than AwFormDatabase.

https://codereview.chromium.org/14503010/diff/8001/android_webview/native/aw_...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/8001/android_webview/native/aw_...
android_webview/native/aw_form_database.cc:16: // static
this def is for autofill namespace.

On 2013/04/29 18:00:03, benm wrote:
> anonymous namespace?

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
File components/autofill/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
components/autofill/browser/webdata/autofill_table.cc:472: } else {
himm, notreached is used all over this file in similar circumstances. I am not
sure if we want to lose it.


On 2013/04/29 18:00:03, benm wrote:
> return s.Step() ? s.ColumnInt(0) > 0 : false;
> 
> looks quite neat. You lose the NOTREACHED though I guess.

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
File components/autofill/browser/webdata/autofill_table_unittest.cc (right):

https://codereview.chromium.org/14503010/diff/8001/components/autofill/browse...
components/autofill/browser/webdata/autofill_table_unittest.cc:143:
EXPECT_FALSE(table_->HasFormElements());
will do.

On 2013/04/29 18:00:03, benm wrote:
> + a test where we EXPECT_TRUE ? :)

[sgurun-gerrit only, 2013-05-03 22:06:51]

ready for review, please take a look. thanks,

[benm (inactive), 2013-05-07 11:16:11]

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:19: LOG(WARNING) <<
"initializing autocomplete database failed";
Does the caller of this function then disable autofill functionality? Just
wondering if we need to do anything else here. Maybe we could drop the DB and
try again or something?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:28: has_form_elements_(0),
0 -> false

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:32: web_database_->AddTable(
Do we need to do this every time that we instantiate this object? We need it
first time to set up the database, but after that it should be taken care of by
LoadDatabase() I think?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:43: CancelPendingQuery();
Shutdown() too?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:47:
autofill_data_->ShutdownOnUIThread();
DCHECK that we're on the UI thread.

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:65: 
nit: remove \n (and throughout)

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.h (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:30: // Returns whether the
form has any elements stored. May do
do you mean whether the database has any elements stored?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:32: bool HasFormElements();
Would HasFormData be a better name for symmetry with the Android API?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:35: void ClearFormElements();
ClearFormData?

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:17:
scoped_refptr<autofill::AutofillWebDataService>
Can this be removed now?

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:36: DCHECK(context);
Don't think we need these DCHECKs, we will crash on the next line if it's null

[sgurun-gerrit only, 2013-05-07 18:39:01]

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:19: LOG(WARNING) <<
"initializing autocomplete database failed";
On 2013/05/07 11:16:11, benm wrote:
> Does the caller of this function then disable autofill functionality? Just
> wondering if we need to do anything else here. Maybe we could drop the DB and
> try again or something?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:19: LOG(WARNING) <<
"initializing autocomplete database failed";
the webdata service backend does not issue any queries if the initialization
fails:

  if (db_ && init_status_ == sql::INIT_OK && !request->IsCancelled()) {
    request->SetResult(task.Run(db_.get()).Pass());
  }

as far as I can see, chrome is only showing a UI when init fails. I don't see it
taking any other action:
 // Users of this class may provide a callback to handle errors
  // (e.g. by showing a UI). The callback is called only on error, and
  // takes a single parameter, the sql::InitStatus value from trying
  // to open the database.
  // TODO(joi): Should we combine this with WebDatabaseService::InitCallback?
  typedef base::Callback<void(sql::InitStatus)> ProfileErrorCallback;

// Callback to show error dialog on profile load error.
void ProfileErrorCallback(sql::InitStatus status) {
  ShowProfileErrorDialog(
      (status == sql::INIT_FAILURE) ?
      IDS_COULDNT_OPEN_PROFILE_ERROR : IDS_PROFILE_TOO_NEW_ERROR);
}

     <message name="IDS_COULDNT_OPEN_PROFILE_ERROR" desc="Error displayed on
startup when the profile can not be opened correctly due to problems reading or
writing files in it">
Your profile could not be opened correctly.

Some features may be unavailable.  Please check that the profile exists and you
have permission to read and write its contents."

On 2013/05/07 11:16:11, benm wrote:
> Does the caller of this function then disable autofill functionality? Just
> wondering if we need to do anything else here. Maybe we could drop the DB and
> try again or something?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:28: has_form_elements_(0),
On 2013/05/07 11:16:11, benm wrote:
> 0 -> false

Done.

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:32: web_database_->AddTable(
I think the issue is that there are lots of abstraction layers hiding what is
going on with the real database. Also the naming does not help.

What we are adding is a WebDatabaseTable, which wraps a database table, to the
backend. A WebDatabaseTable knows how to create a db table if it does not exist
and how to migrate itself to newer versions. It also knows how to run queries to
retrieve data (i.e. such as HasFormElements()). At the initialization time, the
backend iterates over all the WebDatabaseTables and inits them,
creating/migrating tables as necessary. the init routine is called by
LoadDatabase() so we need to add the table before calling it.

If on the other hand what you mean is whether we need to do it more than once
during the lifetime of a process, no we do not. but it is already owned by the
AwBrowserContext, and created once.

On 2013/05/07 11:16:11, benm wrote:
> Do we need to do this every time that we instantiate this object? We need it
> first time to set up the database, but after that it should be taken care of
by
> LoadDatabase() I think?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:47:
autofill_data_->ShutdownOnUIThread();
On 2013/05/07 11:16:11, benm wrote:
> DCHECK that we're on the UI thread.

Done.

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:65: 
On 2013/05/07 11:16:11, benm wrote:
> nit: remove \n (and throughout)

Done.

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.h (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:30: // Returns whether the
form has any elements stored. May do
On 2013/05/07 11:16:11, benm wrote:
> do you mean whether the database has any elements stored?

Done.

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:32: bool HasFormElements();
I think we can use "data" in android_webview/ but components/ use elements when
referring to saved form data.

On 2013/05/07 11:16:11, benm wrote:
> Would HasFormData be a better name for symmetry with the Android API?

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:35: void ClearFormElements();
On 2013/05/07 11:16:11, benm wrote:
> ClearFormData?

Done.

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:17:
scoped_refptr<autofill::AutofillWebDataService>
we don't use it android_Webview/ but this method is already declared in
components/ (see autofill_webdata_service.h)
  static scoped_refptr<AutofillWebDataService> FromBrowserContext(
      content::BrowserContext* context);

so the definition has to go somewhere.



On 2013/05/07 11:16:11, benm wrote:
> Can this be removed now?

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:36: DCHECK(context);
On 2013/05/07 11:16:11, benm wrote:
> Don't think we need these DCHECKs, we will crash on the next line if it's null

Done.

[sgurun-gerrit only, 2013-05-07 18:41:59]

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:43: CancelPendingQuery();
On 2013/05/07 11:16:11, benm wrote:
> Shutdown() too?

Done.

[benm (inactive), 2013-05-07 19:06:53]

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.h (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.h:32: bool HasFormElements();
That sounds good to me.

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/45001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:17:
scoped_refptr<autofill::AutofillWebDataService>
Ah, I see.

https://codereview.chromium.org/14503010/diff/56001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/56001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:44: CancelPendingQuery();
These should be the other way around I think? (CancelPendingQuery calls methods
on autofill_data_)

https://codereview.chromium.org/14503010/diff/56001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:95: 
maybe for clarity we should set has_form_data_ to false here before we test
result (i.e. move it from line 83)

https://codereview.chromium.org/14503010/diff/56001/android_webview/native/aw...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/56001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:52: return service->ClearFormData();
no need to return here

[sgurun-gerrit only, 2013-05-07 19:56:09]

https://codereview.chromium.org/14503010/diff/56001/android_webview/browser/a...
File android_webview/browser/aw_form_database_service.cc (right):

https://codereview.chromium.org/14503010/diff/56001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:44: CancelPendingQuery();
On 2013/05/07 19:06:55, benm wrote:
> These should be the other way around I think? (CancelPendingQuery calls
methods
> on autofill_data_)

Done.

https://codereview.chromium.org/14503010/diff/56001/android_webview/browser/a...
android_webview/browser/aw_form_database_service.cc:95: 
On 2013/05/07 19:06:55, benm wrote:
> maybe for clarity we should set has_form_data_ to false here before we test
> result (i.e. move it from line 83)

Done.

https://codereview.chromium.org/14503010/diff/56001/android_webview/native/aw...
File android_webview/native/aw_form_database.cc (right):

https://codereview.chromium.org/14503010/diff/56001/android_webview/native/aw...
android_webview/native/aw_form_database.cc:52: return service->ClearFormData();
On 2013/05/07 19:06:55, benm wrote:
> no need to return here

Done.

[benm (inactive), 2013-05-07 20:08:43]

lgtm

lgtm, thanks selim!

[commit-bot: I haz the power, 2013-05-07 20:30:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/14503010/62001

[sgurun-gerrit only, 2013-05-07 20:35:35]

committing, thanks guys for the great review.

[commit-bot: I haz the power, 2013-05-08 15:30:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/14503010/62001

[commit-bot: I haz the power, 2013-05-08 16:16:38]

Change committed as 198919