Remove isSyncDatabase in WebDatabaseObserver [1/3]

Remove isSyncDatabase in WebDatabaseObserver [1/3]

This patch duplicates some methods without the unneccessary
parameter in order to switch to using them in the following patch.

[1] THIS PATCH
[2] https://codereview.chromium.org/666333002/
[3] https://codereview.chromium.org/669063002/

BUG=428254
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184661

[rtakacs, 2014-10-24 20:52:17]

rtakacs.u-szeged@partner.samsung.com changed reviewers:
+ kinuko@google.com

[kinuko, 2014-10-27 06:31:39]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org
- kinuko@google.com

[kinuko, 2014-10-27 06:31:40]

Meta-comment:
* Please don't send reviews to my google account (@google.com) but chromium one
(@chromium.org)
* Would be nice to have links to corresponding chromium/blink CLs in the
description so that reviewers can see how this multi-sided patch is going to
land

https://codereview.chromium.org/660873004/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/Database.cpp (right):

https://codereview.chromium.org/660873004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/Database.cpp:723: stringIdentifier(), errorSite,
webSqlErrorCode, sqliteErrorCode);
I don't think we should do this.

https://codereview.chromium.org/660873004/diff/1/public/platform/WebDatabaseO...
File public/platform/WebDatabaseObserver.h (right):

https://codereview.chromium.org/660873004/diff/1/public/platform/WebDatabaseO...
public/platform/WebDatabaseObserver.h:54: int errorSite, int webSqlErrorCode,
int sqliteErrorCode) { }
Can't you call the corresponding old method here as its default impl instead of
calling these methods twice in the callsites?  Then later you can delete the
default impl and old methods after you override these in chromium.

[rtakacs, 2014-10-28 10:32:17]

rtakacs.u-szeged@partner.samsung.com changed reviewers:
+ darin@chromium.org, mkwst@chromium.org

[rtakacs, 2014-10-28 10:32:18]

This patch is the first of the three steps to eliminate isSyncDatabase.
I duplicated some methods without the unneccesary parameter.
WebDatabaseObserverImpl [chromium side] inherits these new methods and in the
following patch I can switch to using them:
https://codereview.chromium.org/666333002/

The last patch will be a final clean-up for the old methods with isSyncDatabase:
https://codereview.chromium.org/669063002/

[Mike West, 2014-10-28 10:43:14]

mkwst@chromium.org changed required reviewers:
+ kinuko@chromium.org

[Mike West, 2014-10-28 10:43:14]

Looks reasonable; if Kinuko-san is happy, I'm happy.

Please file a bug for this cleanup, and add a `BUG=XXX` line to the CL
description so that all these CLs are tied together.

Additionally, please list the subsequent patches in the CL description. horo@'s
patches like https://codereview.chromium.org/685503004/ are a good example.

https://codereview.chromium.org/660873004/diff/20001/public/platform/WebDatab...
File public/platform/WebDatabaseObserver.h (right):

https://codereview.chromium.org/660873004/diff/20001/public/platform/WebDatab...
public/platform/WebDatabaseObserver.h:56: int errorSite, int webSqlErrorCode,
int sqliteErrorCode)
Nit: This is strange formatting; is it what `git cl format` gives you?

https://codereview.chromium.org/660873004/diff/20001/public/platform/WebDatab...
public/platform/WebDatabaseObserver.h:96: // FIXME: Remove these functions if we
don't use isSyncDatabase in chromium part.
Nit: "Remove these functions once we no longeruse isSyncDatabase in Chromium."

[rtakacs, 2014-10-29 11:10:01]

On 2014/10/28 10:43:14, Mike West wrote:
> Looks reasonable; if Kinuko-san is happy, I'm happy.
> 
> Please file a bug for this cleanup, and add a `BUG=XXX` line to the CL
> description so that all these CLs are tied together.

Done.

> 
> Additionally, please list the subsequent patches in the CL description.
horo@'s
> patches like https://codereview.chromium.org/685503004/ are a good example.

Thanks for showing me an example of how multi-sided patches should look like.
 
>
https://codereview.chromium.org/660873004/diff/20001/public/platform/WebDatab...
> File public/platform/WebDatabaseObserver.h (right):
> 
>
https://codereview.chromium.org/660873004/diff/20001/public/platform/WebDatab...
> public/platform/WebDatabaseObserver.h:56: int errorSite, int webSqlErrorCode,
> int sqliteErrorCode)
> Nit: This is strange formatting; is it what `git cl format` gives you?

I runned 'git cl format --full' but there was no difference.
The only difference is that whitespaces between empty blocks are removed ({ } ->
{}). But in this case the check-webkit-style gives me errors. So I left this.

>
https://codereview.chromium.org/660873004/diff/20001/public/platform/WebDatab...
> public/platform/WebDatabaseObserver.h:96: // FIXME: Remove these functions if
we
> don't use isSyncDatabase in chromium part.
> Nit: "Remove these functions once we no longeruse isSyncDatabase in Chromium."

Done.

[kinuko, 2014-10-29 16:25:32]

Thanks, the CL description looks much nicer / more review-friendly now :)
(Thanks Mike for giving better review comments)

lgtm

https://codereview.chromium.org/660873004/diff/40001/public/platform/WebDatab...
File public/platform/WebDatabaseObserver.h (right):

https://codereview.chromium.org/660873004/diff/40001/public/platform/WebDatab...
public/platform/WebDatabaseObserver.h:96: // FIXME: Remove these functions once
we no longeruse isSyncDatabase in Chromium.
nit: longeruse -> longer use

[kinuko, 2014-10-29 16:25:47]

On 2014/10/29 16:25:32, kinuko (slow to respond) wrote:
> Thanks, the CL description looks much nicer / more review-friendly now :)
> (Thanks Mike for giving better review comments)
> 
> lgtm
> 
>
https://codereview.chromium.org/660873004/diff/40001/public/platform/WebDatab...
> File public/platform/WebDatabaseObserver.h (right):
> 
>
https://codereview.chromium.org/660873004/diff/40001/public/platform/WebDatab...
> public/platform/WebDatabaseObserver.h:96: // FIXME: Remove these functions
once
> we no longeruse isSyncDatabase in Chromium.
> nit: longeruse -> longer use

Could you run try bots?

[rtakacs, 2014-10-29 17:27:05]

On 2014/10/29 16:25:47, kinuko (slow to respond) wrote:
> On 2014/10/29 16:25:32, kinuko (slow to respond) wrote:
> > Thanks, the CL description looks much nicer / more review-friendly now :)
> > (Thanks Mike for giving better review comments)
> > 
> > lgtm
> >
Thanks! 
> >
>
https://codereview.chromium.org/660873004/diff/40001/public/platform/WebDatab...
> > File public/platform/WebDatabaseObserver.h (right):
> > 
> >
>
https://codereview.chromium.org/660873004/diff/40001/public/platform/WebDatab...
> > public/platform/WebDatabaseObserver.h:96: // FIXME: Remove these functions
> once
> > we no longeruse isSyncDatabase in Chromium.
> > nit: longeruse -> longer use
> 
Ahh, sorry :) Disadvantage of copy-paste.

> Could you run try bots?

Of course! I forgot to run them.

[rtakacs, 2014-10-29 19:58:06]

The CQ bit was checked by rtakacs.u-szeged@partner.samsung.com

[rtakacs, 2014-10-29 19:58:28]

The CQ bit was unchecked by rtakacs.u-szeged@partner.samsung.com

[Mike West, 2014-10-30 08:07:47]

If Kinuko-san is happy, I'm happy. LGTM % nit.

https://codereview.chromium.org/660873004/diff/60001/public/platform/WebDatab...
File public/platform/WebDatabaseObserver.h (right):

https://codereview.chromium.org/660873004/diff/60001/public/platform/WebDatab...
public/platform/WebDatabaseObserver.h:56: int errorSite, int webSqlErrorCode,
int sqliteErrorCode)
Nit: did `git cl format` agree with this structure? It surprises me that each
item isn't on its own line.

[Mike West, 2014-10-30 08:18:24]

On 2014/10/30 08:07:47, Mike West (OOO until 3rd) wrote:
> If Kinuko-san is happy, I'm happy. LGTM % nit.
> 
>
https://codereview.chromium.org/660873004/diff/60001/public/platform/WebDatab...
> File public/platform/WebDatabaseObserver.h (right):
> 
>
https://codereview.chromium.org/660873004/diff/60001/public/platform/WebDatab...
> public/platform/WebDatabaseObserver.h:56: int errorSite, int webSqlErrorCode,
> int sqliteErrorCode)
> Nit: did `git cl format` agree with this structure? It surprises me that each
> item isn't on its own line.

Ignore this comment. I apparently find it _really_ weird, but if `git cl format`
is happy, no worries.

[rtakacs, 2014-10-30 17:09:44]

The CQ bit was checked by rtakacs.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-10-30 17:10:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660873004/60001

[commit-bot: I haz the power, 2014-10-30 18:12:52]

Committed patchset #4 (id:60001) as 184661