Issue 1834563002: initial add of SQL based storage

bburns

bburns@chromium.org changed reviewers: + dimich@chromium.org, fgorski@chromium.org

4 years, 9 months ago (2016-03-24 01:05:37 UTC) #1

bburns

This is the first step towards a SQLite based store. This doesn't have tests, which ...

4 years, 9 months ago (2016-03-24 01:05:38 UTC) #2

fgorski

Please try to adapt tests from OPMS_impl_unittests. I don't think the callbacks are implemented properly. ...

4 years, 9 months ago (2016-03-24 16:27:56 UTC) #3

Dmitry Titov

Agree with all Filip's comments, couple more: https://codereview.chromium.org/1834563002/diff/1/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/1/components/offline_pages/offline_page_metadata_store_sql.cc#newcode43 components/offline_pages/offline_page_metadata_store_sql.cc:43: #define OFFLINE_ID ...

4 years, 9 months ago (2016-03-24 23:22:20 UTC) #4

bburns

Comments addressed. Tests added. Please take another look. Thanks! --brendan https://codereview.chromium.org/1834563002/diff/1/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/1/components/offline_pages/offline_page_metadata_store_sql.cc#newcode27 ...

4 years, 9 months ago (2016-03-25 23:07:38 UTC) #5

fgorski

Hey, Brendan, this patch looks much better and this is huge amount of work. I ...

4 years, 8 months ago (2016-03-29 06:50:19 UTC) #6

bburns

Comments addressed, please re-check. Will work on determining why tests are broken. https://codereview.chromium.org/1834563002/diff/60001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc ...

4 years, 8 months ago (2016-03-30 16:54:40 UTC) #7

Comments addressed, please re-check.  Will work on determining why tests are
broken.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:28: // this is a
#define instead of a const so that
On 2016/03/29 06:50:18, fgorski wrote:
> nit: sentence case.

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:29: // I can use it
inline in other SQL statements below
On 2016/03/29 06:50:19, fgorski wrote:
> so it can be used.

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:118: base::FilePath
path(statement->ColumnString(OP_FILE_PATH));
On 2016/03/29 06:50:19, fgorski wrote:
> From the level db store impl:
> #if defined(OS_POSIX)
>   path_string = item.file_path.value();
> #elif defined(OS_WIN)
>   path_string = base::WideToUTF8(item.file_path.value());
> #endif

Is this really required?  It seems like something due to weirdness in Proto not
anything inherent.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:131: const char
sql[] =
On 2016/03/29 06:50:19, fgorski wrote:
> since it is a const, kSql

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:172: } else if
(!base::CreateDirectory(db_file_path_.DirName())) {
On 2016/03/29 06:50:18, fgorski wrote:
> since you are logging here, probably makes sense to call
> CreateDirectoryAndGetError and log the error.

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:187:
std::vector<OfflinePageItem>());
On 2016/03/29 06:50:19, fgorski wrote:
> do you intend to stop execution here?
> if so return.
> 
> Also consider logging appropriate message.
> 
> This might also be why the tests are failing, in case you are calling Init
twice
> on meta_table (and hit DCHECK)

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:192:
NotifyLoadResult(callback, runner, STORE_INIT_FAILED,
On 2016/03/29 06:50:19, fgorski wrote:
> this is a single line, but a multiline body, surround with {}.
> 
> Also return if it is appropriate and log

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:198:
std::vector<OfflinePageItem>());
On 2016/03/29 06:50:19, fgorski wrote:
> return missing

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:241: if
(!transaction.Commit()) {
On 2016/03/29 06:50:18, fgorski wrote:
> This posts the result twice upon failure, because you didn't return. Also, I
> don't like the if here. how about:
> 
> bool result = transaction.Commit();  // can be bool success = 
> runner->PostTask(FROM_HERE, base::Bind(callback, result));
> 

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:248: bool success =
DropTable(db_.get(), kOfflinePagesTableName);
On 2016/03/29 06:50:19, fgorski wrote:
> shouldn't DDL be called on the background thread?
> 
> Also I don't see a call to Reset in tests, so perhaps there is a chance to
> improve there.

If you want.  Drop table seemed like it was so fast that there was little reason
to background it.  Your call.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:253: const
LoadCallback& callback,
On 2016/03/29 06:50:19, fgorski wrote:
> I don't mean to argue either order is better:
> callback, runner,
> or
> runner, callback,
> 
> but please use it consistently in the code.

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:289: // Nothing to
do.
On 2016/03/29 06:50:19, fgorski wrote:
> Good shortcut, but for safety you can post it to the thread, so execution
always
> yields here.
> We've had code in the past that looped itself on the call stack retrying. I
> don't expect this problem here, but it also makes things easier to test.

Done.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:37: bool in_memory);
On 2016/03/29 06:50:19, fgorski wrote:
> don't expose it here if it is only for testing.
> 
> Please add a method:
> SetInMemoryForTesting(bool in_memory)
> 
> That is a common pattern.

Done.

fgorski

3 responses to address. Please also update the Issue description, specify bug and loop in ...

4 years, 8 months ago (2016-03-30 17:19:31 UTC) #8

3 responses to address.

Please also update the Issue description, specify bug and loop in Jian Li, who
will be a perfect sub for Dmitry, since he is available and will probably find
things I missed.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:118: base::FilePath
path(statement->ColumnString(OP_FILE_PATH));
On 2016/03/30 16:54:39, bburns wrote:
> On 2016/03/29 06:50:19, fgorski wrote:
> > From the level db store impl:
> > #if defined(OS_POSIX)
> >   path_string = item.file_path.value();
> > #elif defined(OS_WIN)
> >   path_string = base::WideToUTF8(item.file_path.value());
> > #endif
> 
> Is this really required?  It seems like something due to weirdness in Proto
not
> anything inherent.

It's actually file path that is weird.

class BASE_EXPORT FilePath {
 public:
#if defined(OS_POSIX)
  // On most platforms, native pathnames are char arrays, and the encoding
  // may or may not be specified.  On Mac OS X, native pathnames are encoded
  // in UTF-8.
  typedef std::string StringType;
#elif defined(OS_WIN)
  // On Windows, for Unicode-aware applications, native pathnames are wchar_t
  // arrays encoded in UTF-16.
  typedef std::wstring StringType;
#endif  // OS_WIN

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:248: bool success =
DropTable(db_.get(), kOfflinePagesTableName);
On 2016/03/30 16:54:39, bburns wrote:
> On 2016/03/29 06:50:19, fgorski wrote:
> > shouldn't DDL be called on the background thread?
> > 
> > Also I don't see a call to Reset in tests, so perhaps there is a chance to
> > improve there.
> 
> If you want.  Drop table seemed like it was so fast that there was little
reason
> to background it.  Your call.

Let's stay consistent with choice of threads. If you put all activity related to
DB on background thread, you will never try to drop table at the same time your
writing to it, so no additional synchronization problems.

https://codereview.chromium.org/1834563002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:289: // Nothing to
do.
On 2016/03/30 16:54:40, bburns wrote:
> On 2016/03/29 06:50:19, fgorski wrote:
> > Good shortcut, but for safety you can post it to the thread, so execution
> always
> > yields here.
> > We've had code in the past that looped itself on the call stack retrying. I
> > don't expect this problem here, but it also makes things easier to test.
> 
> Done.

I meant something like:
base::ThreadTaskRunnerHandle::Get()->Post()

as you will not have to involve the background thread at all, but will break
complete this call, before the async return starts.

fgorski

lgtm Repeating my earlier request, to get you moving from here: Please also update the ...

4 years, 8 months ago (2016-03-30 20:39:57 UTC) #10

bburns

bburns@chromium.org changed reviewers: + jianli@chromium.org - dimich@chromium.org

4 years, 8 months ago (2016-03-30 21:03:39 UTC) #11

bburns

Jian, Filip LGTM'd this but would like you to take a look too. Please take ...

4 years, 8 months ago (2016-03-30 21:03:40 UTC) #12

bburns

Description was changed from ========== initial add of SQL based storage BUG=NONE ========== to ========== ...

4 years, 8 months ago (2016-03-30 21:03:50 UTC) #13

bburns

Description was changed from ========== initial add of SQL based storage BUG=599235 ========== to ========== ...

4 years, 8 months ago (2016-03-30 21:04:30 UTC) #14

jianli

https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode28 components/offline_pages/offline_page_metadata_store_sql.cc:28: // This is a #define instead of a const ...

4 years, 8 months ago (2016-03-31 23:40:55 UTC) #15

bburns

Comments addressed. Please re-check. Thanks! --brendan https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode28 components/offline_pages/offline_page_metadata_store_sql.cc:28: // This is ...

4 years, 8 months ago (2016-04-01 17:12:57 UTC) #16

jianli

https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode168 components/offline_pages/offline_page_metadata_store_sql.cc:168: OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {} On 2016/04/01 17:12:56, bburns wrote: > On ...

4 years, 8 months ago (2016-04-01 20:57:59 UTC) #18

bburns

Comments addressed. Please re-check. Thanks! https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/140001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode168 components/offline_pages/offline_page_metadata_store_sql.cc:168: OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {} On 2016/04/01 ...

4 years, 8 months ago (2016-04-01 22:51:44 UTC) #19

jianli

lgtm https://codereview.chromium.org/1834563002/diff/240001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/240001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode175 components/offline_pages/offline_page_metadata_store_sql.cc:175: DLOG(WARNING) << "Proto database will not be deleted."; ...

4 years, 8 months ago (2016-04-01 22:59:52 UTC) #20

bburns

On 2016/04/01 22:59:52, jianli wrote: > lgtm > > https://codereview.chromium.org/1834563002/diff/240001/components/offline_pages/offline_page_metadata_store_sql.cc > File components/offline_pages/offline_page_metadata_store_sql.cc (right): > ...

4 years, 8 months ago (2016-04-01 23:23:23 UTC) #21

bburns

bburns@chromium.org changed reviewers: + shess@chromium.org

4 years, 8 months ago (2016-04-01 23:24:12 UTC) #22

bburns

shess: friendly monday afternoon ping for OWNERS Thanks! --brendan

4 years, 8 months ago (2016-04-04 22:38:33 UTC) #24

bburns

bburns@chromium.org changed reviewers: + gbillock@chromium.org - shess@chromium.org

4 years, 8 months ago (2016-04-05 21:00:04 UTC) #25

bburns

-shess: on vacation +gbillock: for SQL OWNERS. Thanks! --brendan

4 years, 8 months ago (2016-04-05 21:00:05 UTC) #26

Greg Billock

On 2016/04/05 21:00:05, bburns wrote: > -shess: on vacation > +gbillock: for SQL OWNERS. > ...

4 years, 8 months ago (2016-04-05 21:35:17 UTC) #27

chromium-reviews

Ok, if you'd prefer I can wait for shess@ he is on vacation until next ...

4 years, 8 months ago (2016-04-05 21:37:08 UTC) #28

chromium-reviews

I realzied actually, I'm on vacation next week, so if it's not a giant problem, ...

4 years, 8 months ago (2016-04-05 21:38:19 UTC) #29

Scott Hess - ex-Googler

shess@chromium.org changed reviewers: + shess@chromium.org

4 years, 8 months ago (2016-04-12 21:49:20 UTC) #30

Scott Hess - ex-Googler

shess@chromium.org changed reviewers: + shess@chromium.org

4 years, 8 months ago (2016-04-12 21:49:20 UTC) #31

Scott Hess - ex-Googler

sorry for the delay, I was oot and neglected to set my status. https://codereview.chromium.org/1834563002/diff/320001/components/offline_pages/offline_page_metadata_store_sql.cc File ...

4 years, 8 months ago (2016-04-12 21:49:21 UTC) #32

Scott Hess - ex-Googler

sorry for the delay, I was oot and neglected to set my status. https://codereview.chromium.org/1834563002/diff/320001/components/offline_pages/offline_page_metadata_store_sql.cc File ...

4 years, 8 months ago (2016-04-12 21:49:22 UTC) #33

Scott Hess - ex-Googler

https://codereview.chromium.org/1834563002/diff/60001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/60001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode248 components/offline_pages/offline_page_metadata_store_sql.cc:248: bool success = DropTable(db_.get(), kOfflinePagesTableName); On 2016/03/30 17:19:31, fgorski ...

4 years, 8 months ago (2016-04-12 21:57:23 UTC) #34

Scott Hess - ex-Googler

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/320001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode47 components/offline_pages/offline_page_metadata_store_sql.cc:47: " user_initiated BOOLEAN," On 2016/04/12 21:49:21, Scott Hess wrote: ...

4 years, 8 months ago (2016-04-14 18:18:53 UTC) #35

bburns

Comments addressed. Please take another look. Sorry for the delay, I was also ooo (last ...

4 years, 8 months ago (2016-04-19 17:59:45 UTC) #36

Comments addressed.  Please take another look.

Sorry for the delay, I was also ooo (last week)

Thanks!
--brendan

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:32: #define
OFFLINE_PAGES_TABLE_NAME "offlinepages"
On 2016/04/12 21:49:21, Scott Hess wrote:
> Are you actually planning to ever use this to somehow redirect table names? 
> When debugging things from the sql/ level, it's generally helpful if clients
> just code statements in searchable form rather than doing a partial ORM layer.

> If it will be useful, go for it, but if you're imagining this is best practice
> or something, reconsider.

Its repeated in several places, so I have a constant to prevent typos, etc. 
It's not intendeded for ORM.

I'm happy to inline the string if you'd rather.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:39: " offline_url
VARCHAR(2048),"
On 2016/04/12 21:49:22, Scott Hess wrote:
> VARCHAR(N) has no meaning to SQLite, it will all be TEXT.  Unless you feel
this
> is self-documenting or something, I'd just go with TEXT or VARCHAR.

Done.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:47: " user_initiated
BOOLEAN,"
On 2016/04/14 18:18:53, Scott Hess wrote:
> On 2016/04/12 21:49:21, Scott Hess wrote:
> > BOOLEAN has no meaning to SQLite.
> 
> SQLite stores most types as a type code plus the type data.  INTEGER 0 and
> INTEGER 1 have an optimized encoding of 1 byte each, whereas INTEGER 2 or
> INTEGER -1 would need 2 bytes, so that's the most sensible way to encode a
> boolean.
> 
> [SQLite added this encoding in 2006, so you might also see suggestions to
encode
> booleans as VARCHAR column with NULL or "".  IMHO that's more contrived, don't
> use that.]

Done.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:47: " user_initiated
BOOLEAN,"
On 2016/04/12 21:49:21, Scott Hess wrote:
> BOOLEAN has no meaning to SQLite.

Acknowledged.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:48: "
UNIQUE(offline_id))";
On 2016/04/12 21:49:21, Scott Hess wrote:
> Why not just "offline_id INTEGER PRIMARY KEY" in the first place?

I had this.  For some reason it didn't work correctly with the INSERT OR REPLACE
that I'm doing for update.  But I'm def. not an expert, I could have had some
other error.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:60: // that hold the
different pieces of offline page.
On 2016/04/12 21:49:21, Scott Hess wrote:
> This feels all indexy and static-compile-checking, but in reality it's a
> hard-coded mapping which only happens to coincidentally match, and has no
actual
> automated checking.  The bind case isn't abstracted in this way, and has the
> same basic problem.  The only solution is thorough tests, but once you have
that
> this abstraction really doesn't buy much.
> 
> That said, your preference.

This is really just to declare constants for the column indices (e.g. Column #0,
etc)

Previous reviewers preferred an enum to a #define.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:151: #endif
On 2016/04/12 21:49:22, Scott Hess wrote:
> Declaring the variable inline in the conditional would have the useful side
> effect of not working accidentally with empty paths for new platforms.

Done.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:183:
meta_table_.reset(new sql::MetaTable);
On 2016/04/12 21:49:21, Scott Hess wrote:
> You never use meta_table_ outside of this method.  Maybe it could be stack
> local?  Note that it maintains a pointer to the db, so it might still make
sense
> as a unique_ptr<> rather than being constructed directly on the stack.

Done.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:189: if
(!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {
On 2016/04/12 21:49:21, Scott Hess wrote:
> Are there lots of these?

Lots of errors?  Probably not, but still worth handling, right?

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:249: //
TODO(bburns): add UMA metrics here (and for levelDB).
On 2016/04/12 21:49:21, Scott Hess wrote:
> What does this mean?  It sounds like you're building dueling implementations? 
> The tracking bug really doesn't explain it.

There's an existing implementation:

https://code.google.com/p/chromium/codesearch#chromium/src/components/offline...

This is part of deprecating that version and migrating to SQLite.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:333: if
(offline_ids.size() == 0) {
On 2016/04/12 21:49:21, Scott Hess wrote:
> .empty() may be more robust.

Done.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:13: #include
"base/memory/scoped_ptr.h"
On 2016/04/12 21:49:22, Scott Hess wrote:
> sql/ has been converted to unique_ptr...

Done.

bburns

bburns@chromium.org changed reviewers: - gbillock@chromium.org

4 years, 8 months ago (2016-04-19 18:31:47 UTC) #37

Scott Hess - ex-Googler

Quick pass reviewing comments-on-comments. I'll follow up with another pass on the code changes. https://codereview.chromium.org/1834563002/diff/320001/components/offline_pages/offline_page_metadata_store_sql.cc ...

4 years, 8 months ago (2016-04-20 18:22:37 UTC) #38

Quick pass reviewing comments-on-comments.  I'll follow up with another pass on
the code changes.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:32: #define
OFFLINE_PAGES_TABLE_NAME "offlinepages"
On 2016/04/19 17:59:45, bburns wrote:
> On 2016/04/12 21:49:21, Scott Hess wrote:
> > Are you actually planning to ever use this to somehow redirect table names? 
> > When debugging things from the sql/ level, it's generally helpful if clients
> > just code statements in searchable form rather than doing a partial ORM
layer.
> 
> > If it will be useful, go for it, but if you're imagining this is best
practice
> > or something, reconsider.
> 
> Its repeated in several places, so I have a constant to prevent typos, etc. 
> It's not intendeded for ORM.
> 
> I'm happy to inline the string if you'd rather.

It's really your call.  I'd be more averse if you were dynamically constructing
statements (a half-implemented ORM is generally materially worse than a complete
ORM).

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:48: "
UNIQUE(offline_id))";
On 2016/04/19 17:59:45, bburns wrote:
> On 2016/04/12 21:49:21, Scott Hess wrote:
> > Why not just "offline_id INTEGER PRIMARY KEY" in the first place?
> 
> I had this.  For some reason it didn't work correctly with the INSERT OR
REPLACE
> that I'm doing for update.  But I'm def. not an expert, I could have had some
> other error.

AFAICT, it should work fine.  I mean when I play around with sqlite3, I can do
INSERT OR REPLACE with NULL or an assigned value for the primary key, and it
works as I'd expect it to.

Actually, you should probably even say "INTEGER PRIMARY KEY NOT NULL".  SQLite
allows NULL in primary keys, but "INTEGER PRIMARY KEY" is an alias for rowid,
which cannot be NULL.  So SQLite will just ignore the NOT NULL, but it provides
a tidbit of documentation.

It's probably worth figuring out.  As-is, this creates an additional btree
versus just using a primary key.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:60: // that hold the
different pieces of offline page.
On 2016/04/19 17:59:45, bburns wrote:
> On 2016/04/12 21:49:21, Scott Hess wrote:
> > This feels all indexy and static-compile-checking, but in reality it's a
> > hard-coded mapping which only happens to coincidentally match, and has no
> actual
> > automated checking.  The bind case isn't abstracted in this way, and has the
> > same basic problem.  The only solution is thorough tests, but once you have
> that
> > this abstraction really doesn't buy much.
> > 
> > That said, your preference.
> 
> This is really just to declare constants for the column indices (e.g. Column
#0,
> etc)
> 
> Previous reviewers preferred an enum to a #define.

I'd probably prefer an enum to a #define, too.  But my point was more that there
is no language support or tooling for keeping this list in sync with SQLite's
list.  So you need to manually fix things up as needed.  [I am not asserting
that SQLite will arbitrarily reorder the columns returned by *.]

In the end this is your call as to whether you think it will gain you anything. 
If you later want to just inline the values, feel free to loop me in to review
it, I would support that.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:189: if
(!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {
On 2016/04/19 17:59:45, bburns wrote:
> On 2016/04/12 21:49:21, Scott Hess wrote:
> > Are there lots of these?
> 
> Lots of errors?  Probably not, but still worth handling, right?

Apologies - I mean are there lots of distinct databases involved?  I'm wondering
why you have a separate directory rather than just having the database stored
wherever the directory is stored.  And if you do have a directory of databases,
I'm wondering why you're intuiting where to put things based on a pathname
rather than explicitly coding "Here is the directory to store things, here is
the name to use within that directory."

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:249: //
TODO(bburns): add UMA metrics here (and for levelDB).
On 2016/04/19 17:59:44, bburns wrote:
> On 2016/04/12 21:49:21, Scott Hess wrote:
> > What does this mean?  It sounds like you're building dueling
implementations? 
> > The tracking bug really doesn't explain it.
> 
> There's an existing implementation:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/offline...
> 
> This is part of deprecating that version and migrating to SQLite.

OK, we had an offline discussion.  Reasoning for changing from leveldb to SQLite
seems legit, and I've expressed my concerns WRT places where such a change won't
magically fix problems.

bburns

Comments addressed. I went through and addressed all your comments in the design doc. too. ...

4 years, 8 months ago (2016-04-20 20:34:24 UTC) #39

Comments addressed.  I went through and addressed all your comments in the
design doc. too.  Many thanks for your thoughtful attention!

My one remaining question is with regard to VACUUM, but I don't think that needs
to block this CL.

Thanks again!
--brendan

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:32: #define
OFFLINE_PAGES_TABLE_NAME "offlinepages"
On 2016/04/20 18:22:37, Scott Hess wrote:
> On 2016/04/19 17:59:45, bburns wrote:
> > On 2016/04/12 21:49:21, Scott Hess wrote:
> > > Are you actually planning to ever use this to somehow redirect table
names? 
> > > When debugging things from the sql/ level, it's generally helpful if
clients
> > > just code statements in searchable form rather than doing a partial ORM
> layer.
> > 
> > > If it will be useful, go for it, but if you're imagining this is best
> practice
> > > or something, reconsider.
> > 
> > Its repeated in several places, so I have a constant to prevent typos, etc. 
> > It's not intendeded for ORM.
> > 
> > I'm happy to inline the string if you'd rather.
> 
> It's really your call.  I'd be more averse if you were dynamically
constructing
> statements (a half-implemented ORM is generally materially worse than a
complete
> ORM).

Thanks, I think I prefer this way of doing things.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:48: "
UNIQUE(offline_id))";
On 2016/04/20 18:22:36, Scott Hess wrote:
> On 2016/04/19 17:59:45, bburns wrote:
> > On 2016/04/12 21:49:21, Scott Hess wrote:
> > > Why not just "offline_id INTEGER PRIMARY KEY" in the first place?
> > 
> > I had this.  For some reason it didn't work correctly with the INSERT OR
> REPLACE
> > that I'm doing for update.  But I'm def. not an expert, I could have had
some
> > other error.
> 
> AFAICT, it should work fine.  I mean when I play around with sqlite3, I can do
> INSERT OR REPLACE with NULL or an assigned value for the primary key, and it
> works as I'd expect it to.
> 
> Actually, you should probably even say "INTEGER PRIMARY KEY NOT NULL".  SQLite
> allows NULL in primary keys, but "INTEGER PRIMARY KEY" is an alias for rowid,
> which cannot be NULL.  So SQLite will just ignore the NOT NULL, but it
provides
> a tidbit of documentation.
> 
> It's probably worth figuring out.  As-is, this creates an additional btree
> versus just using a primary key.

Hrm, it works now.  I blame the cosmic rays...

Done.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:60: // that hold the
different pieces of offline page.
On 2016/04/20 18:22:36, Scott Hess wrote:
> On 2016/04/19 17:59:45, bburns wrote:
> > On 2016/04/12 21:49:21, Scott Hess wrote:
> > > This feels all indexy and static-compile-checking, but in reality it's a
> > > hard-coded mapping which only happens to coincidentally match, and has no
> > actual
> > > automated checking.  The bind case isn't abstracted in this way, and has
the
> > > same basic problem.  The only solution is thorough tests, but once you
have
> > that
> > > this abstraction really doesn't buy much.
> > > 
> > > That said, your preference.
> > 
> > This is really just to declare constants for the column indices (e.g. Column
> #0,
> > etc)
> > 
> > Previous reviewers preferred an enum to a #define.
> 
> I'd probably prefer an enum to a #define, too.  But my point was more that
there
> is no language support or tooling for keeping this list in sync with SQLite's
> list.  So you need to manually fix things up as needed.  [I am not asserting
> that SQLite will arbitrarily reorder the columns returned by *.]
> 
> In the end this is your call as to whether you think it will gain you
anything. 
> If you later want to just inline the values, feel free to loop me in to review
> it, I would support that.

Acknowledged.  I think I prefer the descriptive names to random constants in the
code, but I agree it's a little brittle.

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:189: if
(!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {
On 2016/04/20 18:22:36, Scott Hess wrote:
> On 2016/04/19 17:59:45, bburns wrote:
> > On 2016/04/12 21:49:21, Scott Hess wrote:
> > > Are there lots of these?
> > 
> > Lots of errors?  Probably not, but still worth handling, right?
> 
> Apologies - I mean are there lots of distinct databases involved?  I'm
wondering
> why you have a separate directory rather than just having the database stored
> wherever the directory is stored.  And if you do have a directory of
databases,
> I'm wondering why you're intuiting where to put things based on a pathname
> rather than explicitly coding "Here is the directory to store things, here is
> the name to use within that directory."

Ah, I'm basically just mirroring the existing LevelDB implementation that passes
in a directory name to the constructor.

I'm not entirely sure why it is done that way, but I'd prefer the consistency
for easily switching the code over.

Scott Hess - ex-Googler

Apologies for not circling back as promised to review the current state of the code. ...

4 years, 8 months ago (2016-04-21 03:25:11 UTC) #40

Apologies for not circling back as promised to review the current state of the
code.  I spent the day holding someone's hand in an urgent-care center  :-(. 
Unfortunately, I might spend part of tomorrow doing the same, so maybe find some
filler work for the meanwhile :-).

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:189: if
(!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {
On 2016/04/20 20:34:24, bburns wrote:
> On 2016/04/20 18:22:36, Scott Hess wrote:
> > On 2016/04/19 17:59:45, bburns wrote:
> > > On 2016/04/12 21:49:21, Scott Hess wrote:
> > > > Are there lots of these?
> > > 
> > > Lots of errors?  Probably not, but still worth handling, right?
> > 
> > Apologies - I mean are there lots of distinct databases involved?  I'm
> wondering
> > why you have a separate directory rather than just having the database
stored
> > wherever the directory is stored.  And if you do have a directory of
> databases,
> > I'm wondering why you're intuiting where to put things based on a pathname
> > rather than explicitly coding "Here is the directory to store things, here
is
> > the name to use within that directory."
> 
> Ah, I'm basically just mirroring the existing LevelDB implementation that
passes
> in a directory name to the constructor.
> 
> I'm not entirely sure why it is done that way, but I'd prefer the consistency
> for easily switching the code over.

It totally makes sense with leveldb, since that's a federation of files.

Is it using the _same_ directory?  If so, that maybe presents an eventual
migration issue, since the obvious way to delete the leveldb directory is a
recursive delete, rather than deleting the individual files in the directory.

chromium-reviews

Ugh, sorry to hear that, hope folks feel better soon! There's definitely no rush on ...

4 years, 8 months ago (2016-04-21 03:26:59 UTC) #41

Ugh, sorry to hear that, hope folks feel better soon!

There's definitely no rush on this.

Thanks
Brendan

On Wed, Apr 20, 2016, 8:25 PM <shess@chromium.org> wrote:

> Apologies for not circling back as promised to review the current state of
> the
> code. I spent the day holding someone's hand in an urgent-care center :-(.
> Unfortunately, I might spend part of tomorrow doing the same, so maybe
> find some
> filler work for the meanwhile :-).
>
>
>
>
>
https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
> File components/offline_pages/offline_page_metadata_store_sql.cc
> (right):
>
>
>
https://codereview.chromium.org/1834563002/diff/320001/components/offline_pag...
> components/offline_pages/offline_page_metadata_store_sql.cc:189: if
> (!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {
> On 2016/04/20 20:34:24, bburns wrote:
> > On 2016/04/20 18:22:36, Scott Hess wrote:
> > > On 2016/04/19 17:59:45, bburns wrote:
> > > > On 2016/04/12 21:49:21, Scott Hess wrote:
> > > > > Are there lots of these?
> > > >
> > > > Lots of errors? Probably not, but still worth handling, right?
> > >
> > > Apologies - I mean are there lots of distinct databases involved?
> I'm
> > wondering
> > > why you have a separate directory rather than just having the
> database stored
> > > wherever the directory is stored. And if you do have a directory of
> > databases,
> > > I'm wondering why you're intuiting where to put things based on a
> pathname
> > > rather than explicitly coding "Here is the directory to store
> things, here is
> > > the name to use within that directory."
> >
> > Ah, I'm basically just mirroring the existing LevelDB implementation
> that passes
> > in a directory name to the constructor.
> >
> > I'm not entirely sure why it is done that way, but I'd prefer the
> consistency
> > for easily switching the code over.
>
> It totally makes sense with leveldb, since that's a federation of files.
>
> Is it using the _same_ directory? If so, that maybe presents an
> eventual migration issue, since the obvious way to delete the leveldb
> directory is a recursive delete, rather than deleting the individual
> files in the directory.
>
> https://codereview.chromium.org/1834563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Scott Hess - ex-Googler

OK, mostly getting down to nits, here. https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn File components/offline_pages/BUILD.gn (right): https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn#newcode90 components/offline_pages/BUILD.gn:90: "//sql:sql", Why ...

4 years, 8 months ago (2016-04-25 19:57:55 UTC) #42

bburns

comments addressed, please re-check. thanks! --brendan https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn File components/offline_pages/BUILD.gn (right): https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn#newcode90 components/offline_pages/BUILD.gn:90: "//sql:sql", On 2016/04/25 ...

4 years, 8 months ago (2016-04-26 23:27:02 UTC) #43

comments addressed, please re-check.

thanks!
--brendan

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/BUILD.gn:90: "//sql:sql",
On 2016/04/25 19:57:55, Scott Hess wrote:
> Why is this //sql:sql while the other was //sql?

good question, changed the other to sql:sql (they're equivalent, but the
consistency is good, I suppose)

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl_unittest.cc
(right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:22:
#include "sql/connection.h"
On 2016/04/25 19:57:55, Scott Hess wrote:
> Is this ever used in this CL?

good catch, removed.

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:204: if
(!sql::MetaTable::DoesTableExist(db_.get())) {
On 2016/04/25 19:57:55, Scott Hess wrote:
> The design doc suggestions that you'd use table_name_v1, table_name_v2, etc,
but
> OFFLINE_PAGES_TABLE_NAME has no embedded version info, and you have a meta
table
> here.  So which way were you using to track it?

Ah, this was copy/pasted before I really knew what I was doing.  Removed (and v1
added to the table name, since it was implicit before)

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:308: }
On 2016/04/25 19:57:55, Scott Hess wrote:
> AFAICT, the only reason LoadSync() needs the runner and callback are to pass
> them to here, and the only reason NotifyLoadResult() exists is to be called by
> LoadSync().  I'm wondering if the flow of this would have been easier as
> something like:
> 
> bool InitDatabase() {
>   // Almost everything in LoadSync(), failure returns false;
> }
> 
> void LoadSync(runner, callback) {
>   bool success = InitDatabase();
>   // Everything in NotifyLoadResult().
> }
> 
> Or s/bool/LoadStatus/, of course.
> 
> [I see that offline_page_metadata_store_impl does have a more complicated
> loading sequence which needs the continuation, but AFAICT there's no strong
> reason to retain the complexity here.]

Mostly done.  I retained NotifyLoadResult since there are actually three result
codes, and so re-using the code helps clean things up some.

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:85:
base::WeakPtrFactory<OfflinePageMetadataStoreSQL> weak_ptr_factory_;
On 2016/04/25 19:57:55, Scott Hess wrote:
> I am not entirely able to follow whether this adheres to the thread-related
> comment in weak_ptr.h .  I see that weak_ptr.cc has thread-checker tests.  Do
> the tests you added actually run with multiple threads?  I think they may not,
> it's hard to tell by inspection.
> 
> [Basically, I'm going to delegate this to everyone else, but if you read the
> above and think "Wait, what thread-related comment in weak_ptr.h?", then you
> should go read that comment.]

I believe that this is covered by the fact that in the destructor, we actually
push the destroy onto the background_task_runner_ thread, which means that the
delete
happens after any other access is done.

Scott Hess - ex-Googler

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn File components/offline_pages/BUILD.gn (right): https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn#newcode90 components/offline_pages/BUILD.gn:90: "//sql:sql", On 2016/04/26 23:27:02, bburns wrote: > On 2016/04/25 ...

4 years, 7 months ago (2016-04-27 15:21:00 UTC) #44

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/BUILD.gn:90: "//sql:sql",
On 2016/04/26 23:27:02, bburns wrote:
> On 2016/04/25 19:57:55, Scott Hess wrote:
> > Why is this //sql:sql while the other was //sql?
> 
> good question, changed the other to sql:sql (they're equivalent, but the
> consistency is good, I suppose)

In that case ... why //sql:sql but not //url:url?

[I still can't figure out if this little voice in my head is a defect or not!]

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_impl_unittest.cc
(right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:22:
#include "sql/connection.h"
On 2016/04/26 23:27:02, bburns wrote:
> On 2016/04/25 19:57:55, Scott Hess wrote:
> > Is this ever used in this CL?
> 
> good catch, removed.

Unwinding this another step - since the test does not rely directly on //sql,
can BUILD.gn remove the second dependency?  I _think_ that works IWYU-style,
where if you only use it indirectly through other dependencies, you don't need
it.

To be sure, it feels like you are probably going to eventually need this
dependency because you probably will need sql::Connection code to write tests
for certain edge cases.  I'd be super happy with those tests here, but until
then unnecessary dependencies should be avoided.

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:85:
base::WeakPtrFactory<OfflinePageMetadataStoreSQL> weak_ptr_factory_;
On 2016/04/26 23:27:02, bburns wrote:
> On 2016/04/25 19:57:55, Scott Hess wrote:
> > I am not entirely able to follow whether this adheres to the thread-related
> > comment in weak_ptr.h .  I see that weak_ptr.cc has thread-checker tests. 
Do
> > the tests you added actually run with multiple threads?  I think they may
not,
> > it's hard to tell by inspection.
> > 
> > [Basically, I'm going to delegate this to everyone else, but if you read the
> > above and think "Wait, what thread-related comment in weak_ptr.h?", then you
> > should go read that comment.]
> 
> I believe that this is covered by the fact that in the destructor, we actually
> push the destroy onto the background_task_runner_ thread, which means that the
> delete
> happens after any other access is done.

My understanding is that you can't run is_valid() on a different thread than
you're running GetWeakPtr() on.  You _can_ pass the result of GetWeakPtr() to a
different thread, and have that thread pass it back to the current thread, and
then run is_valid().

The reason is because there is no locking, so the Invalidate() call is
inherently racey WRT dereferencing.  On one thread you could Invalidate()
between the point where get() calls is_valid() and when it returns the pointer. 
The thread calling Invalidate() could immediately assume that it can safely
delete the pointed-to object.

I am not worrying about the db_ instance, here, AFAICT that's fine.  Your weak
pointers are to an instance of OfflienPageMetadataStoreSQL.  AFAICT there is no
code preventing ~OfflinePageMetadataStoreSQL() from running while LoadSync() (or
others) are running in a different thread.

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
Hmm.  Hmm.  When I made my suggestion about removing MetaTable, I was thinking
only in terms of your _v1/_v2 notion.  But looking at this, I'm wondering what
happens if an older _v1 version of the code runs against a newer _v2 database. 
As-is, the older version would create a _v1 database and merrily populate it.

Have you worked out a plausible migration flow for this kind of case?  An
obvious first step would be to check _v2, but ... I've seen migration failures
which involves multiple steps, so even _v2 may not be reliable.  Checking for
the table's name as a prefix also may not work, because you may change to an
entirely different schema.

I'd encourage actually working it out rather than YOLO.  Once you ship it, you
have to deal with the edge cases no matter how poor they turned out to be.  I am
satisified with dealing with them by deleting the entire database and starting
over, but my experience is that usually developers aren't happy with that
solution, so they layer ad-hoc on top of ad-hoc.

I guess the nice thing about MetaTable is that it's a datum everyone agrees on. 
If you need MetaTable, then I'd drop the _v1, as _v2 may never happen.

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:201:
NotifyLoadResult(runner, callback, STORE_INIT_FAILED, result);
This probably needs an early return.

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:203: const char
kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
This spacing is funky.  Personally I'd probably have a blank line, then kSql
then statement then while() as a block.

I'd also move |result| down to where it's really used.  My suggestion about
splitting setup and notification differently was partially about getting rid of
the stylized STORE_LOAD_FAILED call with the empty vector, but if you prefer to
keep the LoadSync()/NotifyLoadResult() breakdown like this, then probably best
to keep the coding for the error cases consistent, since they are more likely to
be copy/pasted around.

bburns

ok, addressed comments again. thanks for your patience. --brendan https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn File components/offline_pages/BUILD.gn (right): https://codereview.chromium.org/1834563002/diff/380001/components/offline_pages/BUILD.gn#newcode90 components/offline_pages/BUILD.gn:90: ...

4 years, 7 months ago (2016-04-27 19:59:21 UTC) #45

ok, addressed comments again.  thanks for your patience.

--brendan

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/BUILD.gn:90: "//sql:sql",
On 2016/04/27 15:21:00, Scott Hess wrote:
> On 2016/04/26 23:27:02, bburns wrote:
> > On 2016/04/25 19:57:55, Scott Hess wrote:
> > > Why is this //sql:sql while the other was //sql?
> > 
> > good question, changed the other to sql:sql (they're equivalent, but the
> > consistency is good, I suppose)
> 
> In that case ... why //sql:sql but not //url:url?
> 
> [I still can't figure out if this little voice in my head is a defect or not!]

if you do "//foo" it implicitly means "//foo:foo"

so I guess it's a style thing, I'm not sure there is a ton of consistency around
this, but I don't super care.

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:85:
base::WeakPtrFactory<OfflinePageMetadataStoreSQL> weak_ptr_factory_;
On 2016/04/27 15:21:00, Scott Hess wrote:
> On 2016/04/26 23:27:02, bburns wrote:
> > On 2016/04/25 19:57:55, Scott Hess wrote:
> > > I am not entirely able to follow whether this adheres to the
thread-related
> > > comment in weak_ptr.h .  I see that weak_ptr.cc has thread-checker tests. 
> Do
> > > the tests you added actually run with multiple threads?  I think they may
> not,
> > > it's hard to tell by inspection.
> > > 
> > > [Basically, I'm going to delegate this to everyone else, but if you read
the
> > > above and think "Wait, what thread-related comment in weak_ptr.h?", then
you
> > > should go read that comment.]
> > 
> > I believe that this is covered by the fact that in the destructor, we
actually
> > push the destroy onto the background_task_runner_ thread, which means that
the
> > delete
> > happens after any other access is done.
> 
> My understanding is that you can't run is_valid() on a different thread than
> you're running GetWeakPtr() on.  You _can_ pass the result of GetWeakPtr() to
a
> different thread, and have that thread pass it back to the current thread, and
> then run is_valid().
> 
> The reason is because there is no locking, so the Invalidate() call is
> inherently racey WRT dereferencing.  On one thread you could Invalidate()
> between the point where get() calls is_valid() and when it returns the
pointer. 
> The thread calling Invalidate() could immediately assume that it can safely
> delete the pointed-to object.
> 
> I am not worrying about the db_ instance, here, AFAICT that's fine.  Your weak
> pointers are to an instance of OfflienPageMetadataStoreSQL.  AFAICT there is
no
> code preventing ~OfflinePageMetadataStoreSQL() from running while LoadSync()
(or
> others) are running in a different thread.

So I think that this is covered because all of the callbacks are handled via
base::Bind,
and base::Bind knows how to handle weak pointers.

(additionally, in this case, this class is created once on startup and never
destroyed,
but of course it should still work properly anyway)

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
On 2016/04/27 15:21:00, Scott Hess wrote:
> Hmm.  Hmm.  When I made my suggestion about removing MetaTable, I was thinking
> only in terms of your _v1/_v2 notion.  But looking at this, I'm wondering what
> happens if an older _v1 version of the code runs against a newer _v2 database.

> As-is, the older version would create a _v1 database and merrily populate it.
> 
> Have you worked out a plausible migration flow for this kind of case?  An
> obvious first step would be to check _v2, but ... I've seen migration failures
> which involves multiple steps, so even _v2 may not be reliable.  Checking for
> the table's name as a prefix also may not work, because you may change to an
> entirely different schema.
> 
> I'd encourage actually working it out rather than YOLO.  Once you ship it, you
> have to deal with the edge cases no matter how poor they turned out to be.  I
am
> satisified with dealing with them by deleting the entire database and starting
> over, but my experience is that usually developers aren't happy with that
> solution, so they layer ad-hoc on top of ad-hoc.
> 
> I guess the nice thing about MetaTable is that it's a datum everyone agrees
on. 
> If you need MetaTable, then I'd drop the _v1, as _v2 may never happen.

So my understanding of the Chrome philosophy is that it is roll-forward only.

So the only way to be in a situation where the code speaks _v1 and the database
is _v2 is if someone uninstalls a new Chrome, and re-installs an old chrome,
while saving application data (fairly hard to do)

The basic plan is to do the forward migration on startup in the background:

if (v1 exists) { 
  for each row
     migrate(row, v1, v2)
  delete table(v1)
}

And likewise in the lookup path while this migration is happening:

if (! row exists in v2) {
  if (v1 exists) {
     lookup row in v1 and return data
  }
}

And similarly for the "SELECT * from V1" union "SELECT * from V2" for
listing/querying.

We don't have (and don't plan to have) consistency requirements between rows in
the database, so I don't think that there will be much in the way of edge cases.

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:201:
NotifyLoadResult(runner, callback, STORE_INIT_FAILED, result);
On 2016/04/27 15:21:00, Scott Hess wrote:
> This probably needs an early return.

Done.

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:203: const char
kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
On 2016/04/27 15:21:00, Scott Hess wrote:
> This spacing is funky.  Personally I'd probably have a blank line, then kSql
> then statement then while() as a block.
> 
> I'd also move |result| down to where it's really used.  My suggestion about
> splitting setup and notification differently was partially about getting rid
of
> the stylized STORE_LOAD_FAILED call with the empty vector, but if you prefer
to
> keep the LoadSync()/NotifyLoadResult() breakdown like this, then probably best
> to keep the coding for the error cases consistent, since they are more likely
to
> be copy/pasted around.

Done.

Scott Hess - ex-Googler

shess@chromium.org changed reviewers: + rsleevi@chromium.org

4 years, 7 months ago (2016-04-27 21:16:44 UTC) #46

Scott Hess - ex-Googler

Ryan, looping you in WRT my WeakPtr commentary. Mostly not aiming for a treatise (you ...

4 years, 7 months ago (2016-04-27 21:16:45 UTC) #47

Ryan, looping you in WRT my WeakPtr commentary.  Mostly not aiming for a
treatise (you can feel free to delegate to me on that), just to make sure I'm
not mis-representing things grossly.

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/BUILD.gn:90: "//sql:sql",
On 2016/04/27 19:59:21, bburns wrote:
> On 2016/04/27 15:21:00, Scott Hess wrote:
> > On 2016/04/26 23:27:02, bburns wrote:
> > > On 2016/04/25 19:57:55, Scott Hess wrote:
> > > > Why is this //sql:sql while the other was //sql?
> > > 
> > > good question, changed the other to sql:sql (they're equivalent, but the
> > > consistency is good, I suppose)
> > 
> > In that case ... why //sql:sql but not //url:url?
> > 
> > [I still can't figure out if this little voice in my head is a defect or
not!]
> 
> if you do "//foo" it implicitly means "//foo:foo"
> 
> so I guess it's a style thing, I'm not sure there is a ton of consistency
around
> this, but I don't super care.

I'm fine with a "don't care", mostly I was pointing it out because the case
you're adding is the only case in this file where it's //x:x.

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/380001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:85:
base::WeakPtrFactory<OfflinePageMetadataStoreSQL> weak_ptr_factory_;
On 2016/04/27 19:59:21, bburns wrote:
> On 2016/04/27 15:21:00, Scott Hess wrote:
> > On 2016/04/26 23:27:02, bburns wrote:
> > > On 2016/04/25 19:57:55, Scott Hess wrote:
> > > > I am not entirely able to follow whether this adheres to the
> thread-related
> > > > comment in weak_ptr.h .  I see that weak_ptr.cc has thread-checker
tests. 
> > Do
> > > > the tests you added actually run with multiple threads?  I think they
may
> > not,
> > > > it's hard to tell by inspection.
> > > > 
> > > > [Basically, I'm going to delegate this to everyone else, but if you read
> the
> > > > above and think "Wait, what thread-related comment in weak_ptr.h?", then
> you
> > > > should go read that comment.]
> > > 
> > > I believe that this is covered by the fact that in the destructor, we
> actually
> > > push the destroy onto the background_task_runner_ thread, which means that
> the
> > > delete
> > > happens after any other access is done.
> > 
> > My understanding is that you can't run is_valid() on a different thread than
> > you're running GetWeakPtr() on.  You _can_ pass the result of GetWeakPtr()
to
> a
> > different thread, and have that thread pass it back to the current thread,
and
> > then run is_valid().
> > 
> > The reason is because there is no locking, so the Invalidate() call is
> > inherently racey WRT dereferencing.  On one thread you could Invalidate()
> > between the point where get() calls is_valid() and when it returns the
> pointer. 
> > The thread calling Invalidate() could immediately assume that it can safely
> > delete the pointed-to object.
> > 
> > I am not worrying about the db_ instance, here, AFAICT that's fine.  Your
weak
> > pointers are to an instance of OfflienPageMetadataStoreSQL.  AFAICT there is
> no
> > code preventing ~OfflinePageMetadataStoreSQL() from running while LoadSync()
> (or
> > others) are running in a different thread.
> 
> So I think that this is covered because all of the callbacks are handled via
> base::Bind,
> and base::Bind knows how to handle weak pointers.
>
> (additionally, in this case, this class is created once on startup and never
> destroyed,
> but of course it should still work properly anyway)

base::Bind knows how to use a weak pointer as a target, but it isn't doing
anything special to allow you to safely access the same underlying instance in
multiple threads.

Here is what I believe to be a valid use of weak pointer across threads:

  callback = base::Bind(&MyClass::MyFunction, weak_ptr_factory_.GetWeakPtr());
  background_task_runner_->PostTask(FROM_HERE,
base::Bind(&OtherClass::OtherFunction, other_object,
base::ThreadTaskRunnerHandle::Get(), callback));

void OtherClass::OtherFunction(scoped_refptr<base::SingleThreadTaskRunner>
runner, base::Closure callback) {
  // ...
  runner->PostTask(FROM_HERE, callback);
}

In this case, the weak pointer is passed along to the background thread as
opaque data, which is then passed back to the foreground thread unchanged, and
the weak pointer is not dereferenced until the foreground task runner calls the
callback.

In your case, you are calling a method against the weak pointer in a background
thread.  The weak pointer implementation provides no locking to allow it to
safely check the validity of the weak pointer when on a different thread than
the factory the weak pointer was created on (WeakPtr::Get is racy against
WeakPtrFactory::Invalidate()).  base::Bind() relies on WeakPtr::get(), so the
race remains.  The task runner relies on the callback, so in the end it will
also rely on WeakPtr::get(), so that race remains.  In addition, there is
nothing that prevents the underlying object from being destructed while the
background task runner is executing a method on that underlying object.

I've double-checked this with a few people who sit nearby, and I'm not finding
the hole in my reading of the implementation details.  I'll loop in rsleevi@ in
case he'd like to apply additional advice.

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/470001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
On 2016/04/27 19:59:21, bburns wrote:
> On 2016/04/27 15:21:00, Scott Hess wrote:
> > Hmm.  Hmm.  When I made my suggestion about removing MetaTable, I was
thinking
> > only in terms of your _v1/_v2 notion.  But looking at this, I'm wondering
what
> > happens if an older _v1 version of the code runs against a newer _v2
database.
> 
> > As-is, the older version would create a _v1 database and merrily populate
it.
> > 
> > Have you worked out a plausible migration flow for this kind of case?  An
> > obvious first step would be to check _v2, but ... I've seen migration
failures
> > which involves multiple steps, so even _v2 may not be reliable.  Checking
for
> > the table's name as a prefix also may not work, because you may change to an
> > entirely different schema.
> > 
> > I'd encourage actually working it out rather than YOLO.  Once you ship it,
you
> > have to deal with the edge cases no matter how poor they turned out to be. 
I
> am
> > satisified with dealing with them by deleting the entire database and
starting
> > over, but my experience is that usually developers aren't happy with that
> > solution, so they layer ad-hoc on top of ad-hoc.
> > 
> > I guess the nice thing about MetaTable is that it's a datum everyone agrees
> on. 
> > If you need MetaTable, then I'd drop the _v1, as _v2 may never happen.
> 
> So my understanding of the Chrome philosophy is that it is roll-forward only.
> 
> So the only way to be in a situation where the code speaks _v1 and the
database
> is _v2 is if someone uninstalls a new Chrome, and re-installs an old chrome,
> while saving application data (fairly hard to do)
>
> The basic plan is to do the forward migration on startup in the background:
> 
> if (v1 exists) { 
>   for each row
>      migrate(row, v1, v2)
>   delete table(v1)
> }
> 
> And likewise in the lookup path while this migration is happening:
> 
> if (! row exists in v2) {
>   if (v1 exists) {
>      lookup row in v1 and return data
>   }
> }
> 
> And similarly for the "SELECT * from V1" union "SELECT * from V2" for
> listing/querying.
> 
> We don't have (and don't plan to have) consistency requirements between rows
in
> the database, so I don't think that there will be much in the way of edge
cases.

I agree that the Chrome philosophy is not to be compatible in both directions,
but in practice stuff happens.  In the end, my greatest concern is that if
someone does manage to get their system into this case, does it eventually
recover?  Broadly from your description, it sounds like it will, so sounds good.

[I guess take this as advice to not assume that motion is 100% forward.  There
is nobody in the system enforcing that invariant, and the people installing
Chrome are sometimes pretty "clever".  The overriding concern is that the
database doesn't end up wedged.]

Scott Hess - ex-Googler

shess@chromium.org changed reviewers: - rsleevi@chromium.org

4 years, 7 months ago (2016-04-27 21:19:40 UTC) #48

Scott Hess - ex-Googler

Actually, moving Ryan to CC instead of Reviewer.

4 years, 7 months ago (2016-04-27 21:19:42 UTC) #49

bburns

Thanks, I'm 100% happy to change the weak_ptr semantics, I'm pretty much a newb when ...

4 years, 7 months ago (2016-04-27 21:26:37 UTC) #50

Scott Hess - ex-Googler

On 2016/04/27 21:26:37, bburns wrote: > Thanks, I'm 100% happy to change the weak_ptr semantics, ...

4 years, 7 months ago (2016-04-27 21:51:57 UTC) #51

Scott Hess - ex-Googler

https://codereview.chromium.org/1834563002/diff/490001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/490001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode173 components/offline_pages/offline_page_metadata_store_sql.cc:173: db_.reset(new sql::Connection); I just realized that I was being ...

4 years, 7 months ago (2016-04-28 02:37:53 UTC) #52

https://codereview.chromium.org/1834563002/diff/490001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/490001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:173: db_.reset(new
sql::Connection);
I just realized that I was being lax about other connection setup!  Somewhere
around here you should have something like:
  db_->set_page_size(4096);
  db_->set_cache_size(500);  // Or -2000
  db_->set_histogram_tag("OfflinePageMetadata");
  db_->set_exclusive_locking();

The default page size is 1024, but SQLite is changing to 4096.  The vast
majority of our systems should use 4096 going forward.  4096 is somewhat more
efficient with read-ahead and page packing, 1024 is probably better for very
constrained systems, but the primary reason for the change is that 1024 on many
systems to day involves a read-modify-write cycle.  For your case, I think your
rows are likely to be large enough to justify this, because it should reduce the
incidence of overflow pages quite a bit.

The default cache size is 2000 pages, so ~2MB for 1024-byte pages.  So
page_size*4 implies cache_size/4.  Negative values tell SQLite to calculate in
terms of KB rather than page units.  [Maybe I should add a set_cache_size_kb()
or something and deprecate the prior usage.]  [[These days by default the file
is mmap'ed which mostly removes the need to worry too much about the cache
size.]]

Chromium profiles _should_ be under a giant lock, so two Chromium's cannot
access the files at the same time.  Exclusive locking tells SQLite to just keep
the lock once the file is locked.  This reduces syscall overhead slightly, and
also allows SQLite to make some assumptions about whether any outside parties
are modifying the file while it isn't looking.  It also means you don't have to
worry about SQLITE_BUSY handling (not that anyone handles that).

The histogram tag is a sql/ addition.  sql::Connection will maintain a set of
histograms for the connection.  See histograms for Sqlite.Error.*,
Sqlite.SizeKB.*, etc.  Also modify the SqliteDatabases histogram_suffixes in
histograms.xml to add your new tag.  I'm not sure what the rules are about those
names, I've stuck to camel-case [A-Za-z]+ just to be safe.

Scott Hess - ex-Googler

On 2016/04/27 21:16:45, Scott Hess wrote: > Ryan, looping you in WRT my WeakPtr commentary. ...

4 years, 7 months ago (2016-04-28 17:53:42 UTC) #53

bburns

On 2016/04/28 02:37:53, Scott Hess wrote: > https://codereview.chromium.org/1834563002/diff/490001/components/offline_pages/offline_page_metadata_store_sql.cc > File components/offline_pages/offline_page_metadata_store_sql.cc (right): > > https://codereview.chromium.org/1834563002/diff/490001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode173 ...

4 years, 7 months ago (2016-04-28 18:54:55 UTC) #54

On 2016/04/28 02:37:53, Scott Hess wrote:
>
https://codereview.chromium.org/1834563002/diff/490001/components/offline_pag...
> File components/offline_pages/offline_page_metadata_store_sql.cc (right):
> 
>
https://codereview.chromium.org/1834563002/diff/490001/components/offline_pag...
> components/offline_pages/offline_page_metadata_store_sql.cc:173: db_.reset(new
> sql::Connection);
> I just realized that I was being lax about other connection setup!  Somewhere
> around here you should have something like:
>   db_->set_page_size(4096);
>   db_->set_cache_size(500);  // Or -2000
>   db_->set_histogram_tag("OfflinePageMetadata");
>   db_->set_exclusive_locking();
> 
> The default page size is 1024, but SQLite is changing to 4096.  The vast
> majority of our systems should use 4096 going forward.  4096 is somewhat more
> efficient with read-ahead and page packing, 1024 is probably better for very
> constrained systems, but the primary reason for the change is that 1024 on
many
> systems to day involves a read-modify-write cycle.  For your case, I think
your
> rows are likely to be large enough to justify this, because it should reduce
the
> incidence of overflow pages quite a bit.
> 
> The default cache size is 2000 pages, so ~2MB for 1024-byte pages.  So
> page_size*4 implies cache_size/4.  Negative values tell SQLite to calculate in
> terms of KB rather than page units.  [Maybe I should add a set_cache_size_kb()
> or something and deprecate the prior usage.]  [[These days by default the file
> is mmap'ed which mostly removes the need to worry too much about the cache
> size.]]
> 
> Chromium profiles _should_ be under a giant lock, so two Chromium's cannot
> access the files at the same time.  Exclusive locking tells SQLite to just
keep
> the lock once the file is locked.  This reduces syscall overhead slightly, and
> also allows SQLite to make some assumptions about whether any outside parties
> are modifying the file while it isn't looking.  It also means you don't have
to
> worry about SQLITE_BUSY handling (not that anyone handles that).
> 
> The histogram tag is a sql/ addition.  sql::Connection will maintain a set of
> histograms for the connection.  See histograms for Sqlite.Error.*,
> Sqlite.SizeKB.*, etc.  Also modify the SqliteDatabases histogram_suffixes in
> histograms.xml to add your new tag.  I'm not sure what the rules are about
those
> names, I've stuck to camel-case [A-Za-z]+ just to be safe.

Ok, I added those tuning parameters, and also move to static functions for the
Bind(*)

please take another look.

thanks!
--brendan

Ryan Sleevi

On 2016/04/27 21:26:37, bburns wrote: > Thanks, I'm 100% happy to change the weak_ptr semantics, ...

4 years, 7 months ago (2016-04-28 21:24:03 UTC) #55

On 2016/04/27 21:26:37, bburns wrote:
> Thanks, I'm 100% happy to change the weak_ptr semantics, I'm pretty much a
newb
> when it comes to Chrome's threading model.

Right, everything Scott said is spot on.


Just to chime in on this part, as far as options:

- Scott highlighted one option, which is to have functions in the anonymous
namespace responsible for doing your work on the background thread. The general
idiom for this is using PostTaskAndReply(FROM_HERE,
base::Bind(&ThingToRunOnWorker, ...), base::Bind(&MyClass::MyResultHandler,
weak_factory_.AsWeakPtr(), ...)). You can also use the base/task_runner_util.h
PostTaskAndReplyWithResult (huh... we should probably move that to base/task
...), which lets you smuggle a result back

- Another option is to use an inner-class for thread-hopping semantics, and
using DeleteSoon to ensure the inner class is nuked on the background thread
when the parent is going away. Mostly, that allows you to keep common state in
between calls - but that common state should *only* be accessed on your worker
thread, and NEVER on the main thread.

Example:
class Inner {
 public:
  // Called on the 'main' thread
  explicit Inner(/* some state to initialize */);

  // Called on the 'background' thread
  void DoSomething(...);

 private:
  friend class base::DeleteHelper<Inner>;

  // Called on the 'background' thread. This is private,
  // with only the DeleteHelper having access, so that you can force the object
  // as always deleted via BackgroundThread->DeleteSoon(inner_) - e.g. that it's
  // always on the background thread.
  ~Inner();
};

Then you use PostTaskAndReply[WithResult] to post tasks, but use
base::Bind(&Inner::DoSomething, inner). When your outer object goes away, you
background_task_runner_->DeleteSoon(inner).

- Finally, you can do tricks with reference counting the inner class, but that
gets even more complex. You can see an example of this idiom in
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc...
, but I mostly mention it so you know it exists, not that you can use it.

bburns

On 2016/04/28 21:24:03, Ryan Sleevi wrote: > On 2016/04/27 21:26:37, bburns wrote: > > Thanks, ...

4 years, 7 months ago (2016-04-28 21:32:01 UTC) #56

On 2016/04/28 21:24:03, Ryan Sleevi wrote:
> On 2016/04/27 21:26:37, bburns wrote:
> > Thanks, I'm 100% happy to change the weak_ptr semantics, I'm pretty much a
> newb
> > when it comes to Chrome's threading model.
> 
> Right, everything Scott said is spot on.
> 
> 
> Just to chime in on this part, as far as options:
> 
> - Scott highlighted one option, which is to have functions in the anonymous
> namespace responsible for doing your work on the background thread. The
general
> idiom for this is using PostTaskAndReply(FROM_HERE,
> base::Bind(&ThingToRunOnWorker, ...), base::Bind(&MyClass::MyResultHandler,
> weak_factory_.AsWeakPtr(), ...)). You can also use the base/task_runner_util.h
> PostTaskAndReplyWithResult (huh... we should probably move that to base/task
> ...), which lets you smuggle a result back
> 
> - Another option is to use an inner-class for thread-hopping semantics, and
> using DeleteSoon to ensure the inner class is nuked on the background thread
> when the parent is going away. Mostly, that allows you to keep common state in
> between calls - but that common state should *only* be accessed on your worker
> thread, and NEVER on the main thread.
> 
> Example:
> class Inner {
>  public:
>   // Called on the 'main' thread
>   explicit Inner(/* some state to initialize */);
> 
>   // Called on the 'background' thread
>   void DoSomething(...);
> 
>  private:
>   friend class base::DeleteHelper<Inner>;
> 
>   // Called on the 'background' thread. This is private,
>   // with only the DeleteHelper having access, so that you can force the
object
>   // as always deleted via BackgroundThread->DeleteSoon(inner_) - e.g. that
it's
>   // always on the background thread.
>   ~Inner();
> };
> 
> Then you use PostTaskAndReply[WithResult] to post tasks, but use
> base::Bind(&Inner::DoSomething, inner). When your outer object goes away, you
> background_task_runner_->DeleteSoon(inner).
> 
> - Finally, you can do tricks with reference counting the inner class, but that
> gets even more complex. You can see an example of this idiom in
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc...
> , but I mostly mention it so you know it exists, not that you can use it.

Cool, thanks.

I think that the static method solution I moved to works in this case, but its
good to know what the options are.

fgorski

small nits after drive by. https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode201 components/offline_pages/offline_page_metadata_store_sql.cc:201: namespace {} // anonymous ...

4 years, 7 months ago (2016-04-29 05:46:25 UTC) #57

bburns

comments addressed, please re-check. thanks --brendan https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode201 components/offline_pages/offline_page_metadata_store_sql.cc:201: namespace {} // ...

4 years, 7 months ago (2016-04-29 16:43:25 UTC) #58

bburns

https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.h File components/offline_pages/offline_page_metadata_store_sql.h (right): https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.h#newcode58 components/offline_pages/offline_page_metadata_store_sql.h:58: base::FilePath path, On 2016/04/29 16:43:25, bburns wrote: > On ...

4 years, 7 months ago (2016-04-29 16:48:04 UTC) #59

bburns

On 2016/04/29 16:48:04, bburns wrote: > https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.h > File components/offline_pages/offline_page_metadata_store_sql.h (right): > > https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.h#newcode58 > ...

4 years, 7 months ago (2016-04-29 17:00:03 UTC) #60

bburns

On 2016/04/29 17:00:03, bburns wrote: > On 2016/04/29 16:48:04, bburns wrote: > > > https://codereview.chromium.org/1834563002/diff/510001/components/offline_pages/offline_page_metadata_store_sql.h ...

4 years, 7 months ago (2016-05-02 17:27:27 UTC) #61

Scott Hess - ex-Googler

I think I'm happy with the weak-ptr-less-ness of things! Now just grinding away the nits. ...

4 years, 7 months ago (2016-05-02 20:36:19 UTC) #62

I think I'm happy with the weak-ptr-less-ness of things!  Now just grinding away
the nits.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:18: #include
"sql/error_delegate_util.h"
Don't think you're using this anywhere.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:38: " user_initiated
INTEGER,"  // this is actually a boolean
Will this be used later?  If the boolean-ness is really considered subtle,
suggest you enumerate the values you're using for true and false (I don't think
it's subtle).  Also, since you aren't setting it right now, it's going to be
NULL in the database.

Your INSERT OR REPLACE will also leave [status] NULL, and [offline_url]. 
ColumnInt() will transform NULL to 0, and ColumnString() will transform NULL to
"", so it's probably safe for this code as-is, but if you ever do SQL queries
against those columns, NULL may not be what you want (for instance, "WHERE
user_initiated<>1" will succeed for values 0 and NULL, but if you say "WHERE
user_initiated=0" it will not succeed for NULL).

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
Either provide optional {}, or don't, but don't be inconsistent within a file.

I prefer to always show the {}, but I've gotten enough pushback in reviews that
I wouldn't recommend it.  AFAICT the safest route is no {} unless either the
body is more than one line or the if() is more than one line.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:144: std::string
path_string = base::WideToUTF8(item.file_path.value());
Man, I hate this pattern.  But I have nothing for you WRT resolving it (maybe it
comes up enough to warrant Path versions in sql::Statement).  But I would put
#else with an #error to protect against typos in your OS_ references.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:160:
db->set_histogram_tag("OfflinePageMetadata");
Missing histograms.xml change.

If you're worried about Yet Another OWNER syndrome, I've found the histograms
owners to be very responsive.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:166: }
Another case to change if you decide to omit optional {}.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:181:
CreateSchema(db);
Why have CreateSchema() return a flag that you're just burying the result?

I'm willing to be relaxed about actually implementing error-handling at this
point, but in that case please log a tracking bug and a TODO referencing the
tracking bug (probably here would be reasonable).

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:190: :
background_task_runner_(background_task_runner),
Maybe std::move().  For most structures I wouldn't care, but scoped_refptr
implies a lock somewhere, or some other sort of delicate code.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:235: DCHECK(db);
This DCHECK is probably redundant.  If you feel that it is important for
purposes of providing a sensible heads-up, then I'd put it in
AddOrUpdateOfflinePage() before posting the task in the first place.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:246: DCHECK(db);
Likewise here, I think.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:268:
sql::Connection* db,
This is taking ownership.  If the delete fails, the caller can't do anything
about it, as they already released their reference.  Probably best to just make
it a unique_ptr<> and drop the explicit delete.

[Then you might be able to just pass the statement.Run() directly to
base::Bind().]

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:21: class
StatementID;
Of these, AFAICT only Connection is necessary here.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:26: }
I'd put the base namespace before sql namespace.  YMMV.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:50: void
SetInMemoryDatabaseForTesting(bool in_memory);
I was going to suggest making this private and using FRIEND_TEST_ALL_PREFIXES()
to give appropriate access, but then I couldn't find any callers.

[Of course, if there are no callers, use_in_memory_ can go away, as can
propagating it.]

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:56: // 'runner' is
where to run the callback.
Is there any benefit to a private static method over having them in an anonymous
namespace?  AFAICT the enum and typedefs from OfflinePageMetadataStore are
public:, so you could skip the OfflinePageMetadataStore:: in some cases (but
mostly you don't, so I might be wrong).  On the savings side, you could skip
OfflinePageMetadataStoreSQL:: in some places with an anonymous namespace.

I guess I could see some rationale to keeping in sync with the existing code,
but against that you should consider where you want the code to be long term
when the leveldb version is long gone.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:83:
scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
Either a blank line here, or consistently run all your instance variables
together.  If I were going to group things, I'd group the path with the db.

bburns

bburns@chromium.org changed reviewers: + mpearson@chromium.org

4 years, 7 months ago (2016-05-02 21:44:16 UTC) #63

bburns

mpearson: OWNERS for histogram.xml Comments addressed, please take another look. thanks! --brendan https://codereview.chromium.org/1834563002/diff/550001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc ...

4 years, 7 months ago (2016-05-02 21:44:18 UTC) #64

mpearson: OWNERS for histogram.xml

Comments addressed, please take another look.

thanks!
--brendan

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:18: #include
"sql/error_delegate_util.h"
On 2016/05/02 20:36:18, Scott Hess wrote:
> Don't think you're using this anywhere.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:38: " user_initiated
INTEGER,"  // this is actually a boolean
On 2016/05/02 20:36:18, Scott Hess wrote:
> Will this be used later?  If the boolean-ness is really considered subtle,
> suggest you enumerate the values you're using for true and false (I don't
think
> it's subtle).  Also, since you aren't setting it right now, it's going to be
> NULL in the database.
> 
> Your INSERT OR REPLACE will also leave [status] NULL, and [offline_url]. 
> ColumnInt() will transform NULL to 0, and ColumnString() will transform NULL
to
> "", so it's probably safe for this code as-is, but if you ever do SQL queries
> against those columns, NULL may not be what you want (for instance, "WHERE
> user_initiated<>1" will succeed for values 0 and NULL, but if you say "WHERE
> user_initiated=0" it will not succeed for NULL).

Thanks for the explanation. We do anticipate using this in the future.  For now
I added a note capturing your comment.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
On 2016/05/02 20:36:18, Scott Hess wrote:
> Either provide optional {}, or don't, but don't be inconsistent within a file.
> 
> I prefer to always show the {}, but I've gotten enough pushback in reviews
that
> I wouldn't recommend it.  AFAICT the safest route is no {} unless either the
> body is more than one line or the if() is more than one line.

Ah, you're running up against my personal preferences vs. the team's
preferences, its subconcious on my part.  fixed.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:144: std::string
path_string = base::WideToUTF8(item.file_path.value());
On 2016/05/02 20:36:18, Scott Hess wrote:
> Man, I hate this pattern.  But I have nothing for you WRT resolving it (maybe
it
> comes up enough to warrant Path versions in sql::Statement).  But I would put
> #else with an #error to protect against typos in your OS_ references.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:160:
db->set_histogram_tag("OfflinePageMetadata");
On 2016/05/02 20:36:18, Scott Hess wrote:
> Missing histograms.xml change.
> 
> If you're worried about Yet Another OWNER syndrome, I've found the histograms
> owners to be very responsive.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:166: }
On 2016/05/02 20:36:18, Scott Hess wrote:
> Another case to change if you decide to omit optional {}.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:181:
CreateSchema(db);
On 2016/05/02 20:36:18, Scott Hess wrote:
> Why have CreateSchema() return a flag that you're just burying the result?
> 
> I'm willing to be relaxed about actually implementing error-handling at this
> point, but in that case please log a tracking bug and a TODO referencing the
> tracking bug (probably here would be reasonable).

This was an oversight.  fixed.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:190: :
background_task_runner_(background_task_runner),
On 2016/05/02 20:36:18, Scott Hess wrote:
> Maybe std::move().  For most structures I wouldn't care, but scoped_refptr
> implies a lock somewhere, or some other sort of delicate code.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:235: DCHECK(db);
On 2016/05/02 20:36:18, Scott Hess wrote:
> This DCHECK is probably redundant.  If you feel that it is important for
> purposes of providing a sensible heads-up, then I'd put it in
> AddOrUpdateOfflinePage() before posting the task in the first place.

Done.  (this DCHECK was requested by a different reviewer...)

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:246: DCHECK(db);
On 2016/05/02 20:36:18, Scott Hess wrote:
> Likewise here, I think.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:268:
sql::Connection* db,
On 2016/05/02 20:36:18, Scott Hess wrote:
> This is taking ownership.  If the delete fails, the caller can't do anything
> about it, as they already released their reference.  Probably best to just
make
> it a unique_ptr<> and drop the explicit delete.
> 
> [Then you might be able to just pass the statement.Run() directly to
> base::Bind().]

It turns out unique_ptr and base::Bind don't get along (at least I couldn't
convince them to) I think this is because it attempts to naively copy the struct
without using std::move and so unique_ptr barfs because there is no copy
constructor.

I did clean up the code somewhat.

I'm open to greater C++-fu than mine...

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:21: class
StatementID;
On 2016/05/02 20:36:19, Scott Hess wrote:
> Of these, AFAICT only Connection is necessary here.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:26: }
On 2016/05/02 20:36:19, Scott Hess wrote:
> I'd put the base namespace before sql namespace.  YMMV.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:50: void
SetInMemoryDatabaseForTesting(bool in_memory);
On 2016/05/02 20:36:19, Scott Hess wrote:
> I was going to suggest making this private and using
FRIEND_TEST_ALL_PREFIXES()
> to give appropriate access, but then I couldn't find any callers.
> 
> [Of course, if there are no callers, use_in_memory_ can go away, as can
> propagating it.]

Removed.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:56: // 'runner' is
where to run the callback.
On 2016/05/02 20:36:19, Scott Hess wrote:
> Is there any benefit to a private static method over having them in an
anonymous
> namespace?  AFAICT the enum and typedefs from OfflinePageMetadataStore are
> public:, so you could skip the OfflinePageMetadataStore:: in some cases (but
> mostly you don't, so I might be wrong).  On the savings side, you could skip
> OfflinePageMetadataStoreSQL:: in some places with an anonymous namespace.
> 
> I guess I could see some rationale to keeping in sync with the existing code,
> but against that you should consider where you want the code to be long term
> when the leveldb version is long gone.

I think that actually I prefer it since it makes the header a little more
self-documenting without diving into the .cc, but I could go either way.  Let me
know if you feel strongly.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:83:
scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
On 2016/05/02 20:36:18, Scott Hess wrote:
> Either a blank line here, or consistently run all your instance variables
> together.  If I were going to group things, I'd group the path with the db.

Done.

Scott Hess - ex-Googler

OK, I think my only substantive point is the handling of the db_ ownership in ...

4 years, 7 months ago (2016-05-02 23:33:32 UTC) #65

OK, I think my only substantive point is the handling of the db_ ownership in
the code doing the reset.  The others are kinda style and "just mentioning" type
things.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:38: " user_initiated
INTEGER,"  // this is actually a boolean
On 2016/05/02 21:44:18, bburns wrote:
> On 2016/05/02 20:36:18, Scott Hess wrote:
> > Will this be used later?  If the boolean-ness is really considered subtle,
> > suggest you enumerate the values you're using for true and false (I don't
> think
> > it's subtle).  Also, since you aren't setting it right now, it's going to be
> > NULL in the database.
> > 
> > Your INSERT OR REPLACE will also leave [status] NULL, and [offline_url]. 
> > ColumnInt() will transform NULL to 0, and ColumnString() will transform NULL
> to
> > "", so it's probably safe for this code as-is, but if you ever do SQL
queries
> > against those columns, NULL may not be what you want (for instance, "WHERE
> > user_initiated<>1" will succeed for values 0 and NULL, but if you say "WHERE
> > user_initiated=0" it will not succeed for NULL).
> 
> Thanks for the explanation. We do anticipate using this in the future.  For
now
> I added a note capturing your comment.

Do you intend the same for [status] and [offline_url]?  Mostly I ask because
this is the kind of thing that in my experience leads to weirdo edge cases. 
Nobody writes their queries expecting NULL.  But in the end I think this is a
style issue.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
On 2016/05/02 21:44:18, bburns wrote:
> On 2016/05/02 20:36:18, Scott Hess wrote:
> > Either provide optional {}, or don't, but don't be inconsistent within a
file.
> > 
> > I prefer to always show the {}, but I've gotten enough pushback in reviews
> that
> > I wouldn't recommend it.  AFAICT the safest route is no {} unless either the
> > body is more than one line or the if() is more than one line.
> 
> Ah, you're running up against my personal preferences vs. the team's
> preferences, its subconcious on my part.  fixed.

:-).  My theory has always been to just have the {} because that way I don't
have to ever think about whether the code is right or wrong.  And I find it
makes code appropriately less dense.  But much/most of Chromium disagrees, so
... *shrug*.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:235: DCHECK(db);
On 2016/05/02 21:44:17, bburns wrote:
> On 2016/05/02 20:36:18, Scott Hess wrote:
> > This DCHECK is probably redundant.  If you feel that it is important for
> > purposes of providing a sensible heads-up, then I'd put it in
> > AddOrUpdateOfflinePage() before posting the task in the first place.
> 
> Done.  (this DCHECK was requested by a different reviewer...)

I'm fine with the DCHECK (others might not be), mostly it just seems like here
it will require searching for the caller if you ever see it.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:268:
sql::Connection* db,
On 2016/05/02 21:44:18, bburns wrote:
> On 2016/05/02 20:36:18, Scott Hess wrote:
> > This is taking ownership.  If the delete fails, the caller can't do anything
> > about it, as they already released their reference.  Probably best to just
> make
> > it a unique_ptr<> and drop the explicit delete.
> > 
> > [Then you might be able to just pass the statement.Run() directly to
> > base::Bind().]
> 
> It turns out unique_ptr and base::Bind don't get along (at least I couldn't
> convince them to) I think this is because it attempts to naively copy the
struct
> without using std::move and so unique_ptr barfs because there is no copy
> constructor.
> 
> I did clean up the code somewhat.
> 
> I'm open to greater C++-fu than mine...

It's a little funky, you use base::Passed(&db_) to base::Bind().  See
internal::PassedWrapper in base/bind_helpers.h .  Here's a Google Groups thread
which references this:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/roY78iTblYc

Personally, I find base::Passed(std::move(db_)) easier to parse.  std::move() to
say "STL, shift the reference" and base::Passed() to say "base::Bind(), this
isn't an accident".  But I don't see that case being used, whereas I do see the
& usage (for instance BindTest.BindMoveOnlyVector in bind_unittest.cc).

Just to be clear, I'm pushing the move semantics here even though it adds some
clunkiness in a bid to reduce the chance for leaks deleting the db.  Having
ownership shift via base::Bind() will delete the database automatically for this
function, and will also delete the database in some cases where the task is
entirely cancelled (and other cases, like the thread is already terminated,
already must allow for leaks).

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:56: // 'runner' is
where to run the callback.
On 2016/05/02 21:44:18, bburns wrote:
> On 2016/05/02 20:36:19, Scott Hess wrote:
> > Is there any benefit to a private static method over having them in an
> anonymous
> > namespace?  AFAICT the enum and typedefs from OfflinePageMetadataStore are
> > public:, so you could skip the OfflinePageMetadataStore:: in some cases (but
> > mostly you don't, so I might be wrong).  On the savings side, you could skip
> > OfflinePageMetadataStoreSQL:: in some places with an anonymous namespace.
> > 
> > I guess I could see some rationale to keeping in sync with the existing
code,
> > but against that you should consider where you want the code to be long term
> > when the leveldb version is long gone.
> 
> I think that actually I prefer it since it makes the header a little more
> self-documenting without diving into the .cc, but I could go either way.  Let
me
> know if you feel strongly.

I was more thinking in terms of not exposing any implementation details that
clients don't need to know about :-).  But IMHO this is a style issue, and it's
your code, so your preference is fine with me.

https://codereview.chromium.org/1834563002/diff/570001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/570001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:37: // a note on
this field.  It will be NULL for now, and is reserved for
Being a comment implies that it's a note on this field, so no need to reference
that.  Also, Mark Mentovai's memory in my head requires me to complain about
comments which aren't legitimate sentences with appropriate grammar and
punctuation and stuff.

If you're going to note the un-set fields, I'd note them all above
kOfflinePagesColumns.  Or provide "NOT NULL DEFAULT x" values as appropriate.

[It might be worthwhile to have "NOT NULL" for all the fields you already do
set, to catch statement errors.  Fields should generally only be null if you
intend to actually check that case.  But I think this is a DCHECK-ish style
issue.]

https://codereview.chromium.org/1834563002/diff/570001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:148: std::string
path_string = base::WideToUTF8(item.file_path.value());
WRT the OS_* pattern, gotta catch all the cases!

https://codereview.chromium.org/1834563002/diff/570001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:267: }
Optional {}.  But the delete should not be conditional in the first place,
unless you're going to post it to someone else in case of error.  The site
posting the task already released the reference.

https://codereview.chromium.org/1834563002/diff/570001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:298:
DCHECK(db_.get());
Previously, SetInMemoryDatabaseForTesting() used DCHECK(db_.get() == nullptr). 
Personally I am fine with the above as is, I'm just calling it out in case
someone previously made suggestions which led to that formulation.  Also below,
if you need a change.

https://codereview.chromium.org/1834563002/diff/570001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1834563002/diff/570001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:93373: +  <suffix
name="OfflinePageMetadata" label="OfflinePageMetadata"/>
Yep.  I believe with this things will just show up on the dashboard once you've
rolled things out.

bburns

Comments addressed. please re-check. thanks! --brendan https://codereview.chromium.org/1834563002/diff/550001/components/offline_pages/offline_page_metadata_store_sql.cc File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/1834563002/diff/550001/components/offline_pages/offline_page_metadata_store_sql.cc#newcode38 components/offline_pages/offline_page_metadata_store_sql.cc:38: " user_initiated INTEGER," ...

4 years, 7 months ago (2016-05-03 17:15:55 UTC) #66

Comments addressed.  please re-check.

thanks!
--brendan

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.cc (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:38: " user_initiated
INTEGER,"  // this is actually a boolean
On 2016/05/02 23:33:31, Scott Hess wrote:
> On 2016/05/02 21:44:18, bburns wrote:
> > On 2016/05/02 20:36:18, Scott Hess wrote:
> > > Will this be used later?  If the boolean-ness is really considered subtle,
> > > suggest you enumerate the values you're using for true and false (I don't
> > think
> > > it's subtle).  Also, since you aren't setting it right now, it's going to
be
> > > NULL in the database.
> > > 
> > > Your INSERT OR REPLACE will also leave [status] NULL, and [offline_url]. 
> > > ColumnInt() will transform NULL to 0, and ColumnString() will transform
NULL
> > to
> > > "", so it's probably safe for this code as-is, but if you ever do SQL
> queries
> > > against those columns, NULL may not be what you want (for instance, "WHERE
> > > user_initiated<>1" will succeed for values 0 and NULL, but if you say
"WHERE
> > > user_initiated=0" it will not succeed for NULL).
> > 
> > Thanks for the explanation. We do anticipate using this in the future.  For
> now
> > I added a note capturing your comment.
> 
> Do you intend the same for [status] and [offline_url]?  Mostly I ask because
> this is the kind of thing that in my experience leads to weirdo edge cases. 
> Nobody writes their queries expecting NULL.  But in the end I think this is a
> style issue.

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:86: if
(db->DoesTableExist(kOfflinePagesTable.table_name)) {
On 2016/05/02 23:33:31, Scott Hess wrote:
> On 2016/05/02 21:44:18, bburns wrote:
> > On 2016/05/02 20:36:18, Scott Hess wrote:
> > > Either provide optional {}, or don't, but don't be inconsistent within a
> file.
> > > 
> > > I prefer to always show the {}, but I've gotten enough pushback in reviews
> > that
> > > I wouldn't recommend it.  AFAICT the safest route is no {} unless either
the
> > > body is more than one line or the if() is more than one line.
> > 
> > Ah, you're running up against my personal preferences vs. the team's
> > preferences, its subconcious on my part.  fixed.
> 
> :-).  My theory has always been to just have the {} because that way I don't
> have to ever think about whether the code is right or wrong.  And I find it
> makes code appropriately less dense.  But much/most of Chromium disagrees, so
> ... *shrug*.

Acknowledged.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:235: DCHECK(db);
On 2016/05/02 23:33:31, Scott Hess wrote:
> On 2016/05/02 21:44:17, bburns wrote:
> > On 2016/05/02 20:36:18, Scott Hess wrote:
> > > This DCHECK is probably redundant.  If you feel that it is important for
> > > purposes of providing a sensible heads-up, then I'd put it in
> > > AddOrUpdateOfflinePage() before posting the task in the first place.
> > 
> > Done.  (this DCHECK was requested by a different reviewer...)
> 
> I'm fine with the DCHECK (others might not be), mostly it just seems like here
> it will require searching for the caller if you ever see it.

Acknowledged.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.cc:268:
sql::Connection* db,
On 2016/05/02 23:33:31, Scott Hess wrote:
> On 2016/05/02 21:44:18, bburns wrote:
> > On 2016/05/02 20:36:18, Scott Hess wrote:
> > > This is taking ownership.  If the delete fails, the caller can't do
anything
> > > about it, as they already released their reference.  Probably best to just
> > make
> > > it a unique_ptr<> and drop the explicit delete.
> > > 
> > > [Then you might be able to just pass the statement.Run() directly to
> > > base::Bind().]
> > 
> > It turns out unique_ptr and base::Bind don't get along (at least I couldn't
> > convince them to) I think this is because it attempts to naively copy the
> struct
> > without using std::move and so unique_ptr barfs because there is no copy
> > constructor.
> > 
> > I did clean up the code somewhat.
> > 
> > I'm open to greater C++-fu than mine...
> 
> It's a little funky, you use base::Passed(&db_) to base::Bind().  See
> internal::PassedWrapper in base/bind_helpers.h .  Here's a Google Groups
thread
> which references this:
>   
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/roY78iTblYc
> 
> Personally, I find base::Passed(std::move(db_)) easier to parse.  std::move()
to
> say "STL, shift the reference" and base::Passed() to say "base::Bind(), this
> isn't an accident".  But I don't see that case being used, whereas I do see
the
> & usage (for instance BindTest.BindMoveOnlyVector in bind_unittest.cc).
> 
> Just to be clear, I'm pushing the move semantics here even though it adds some
> clunkiness in a bid to reduce the chance for leaks deleting the db.  Having
> ownership shift via base::Bind() will delete the database automatically for
this
> function, and will also delete the database in some cases where the task is
> entirely cancelled (and other cases, like the thread is already terminated,
> already must allow for leaks). 

Done.

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
File components/offline_pages/offline_page_metadata_store_sql.h (right):

https://codereview.chromium.org/1834563002/diff/550001/components/offline_pag...
components/offline_pages/offline_page_metadata_store_sql.h:56: // 'runner' is
where to run the callback.
On 2016/05/02 23:33:31, Scott Hess wrote:
> On 2016/05/02 21:44:18, bburns wrote:
> > On 2016/05/02 20:36:19, Scott Hess wrote:
> > > Is there any benefit to a private static method over having them in an
> > anonymous
> > > namespace?  AFAICT the enum and typedefs from OfflinePageMetadataStore are
> > > public:, so you could skip the OfflinePageMetadataStore:: in some cases
(but
> > > mostly you don't, so I might be wrong).  On the savings side, you could
skip
> > > OfflinePageMetadataStoreSQL:: in some places with an anonymous namespace.
> > > 
> > > I guess I could see some rationale to keeping in sync with the existing
> code,
> > > but against that you should consider where you want the code to be long
term
> > > when the leveldb version is long gone.
> > 
> > I think that actually I prefer it since it makes the header a little more
> > self-documenting without diving into the .cc, but I could go either way. 
Let
> me
> > know if you feel strongly.
> 
> I was more thinking in terms of not exposing any implementation details that
> clients don't need to know about :-).  But IMHO this is a style issue, and
it's
> your code, so your preference is fine with me.

Acknowledged.

Scott Hess - ex-Googler

LGTM, after you resolve the nits. If you resolve them as indicated, no need to ...

4 years, 7 months ago (2016-05-03 18:09:03 UTC) #68

bburns

Comments resolved as suggested. No need to recheck unless you want to. Many thanks for ...

4 years, 7 months ago (2016-05-03 19:39:32 UTC) #69

bburns

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, jianli@chromium.org, mpearson@chromium.org, shess@chromium.org ...

4 years, 7 months ago (2016-05-03 23:02:32 UTC) #71