[Offline pages] OPM SQL store: moving load to constructor, updating read to GetOfflinePages

[Offline pages] OPM SQL store: moving load to constructor, updating read to GetOfflinePages

BUG=645522
Committed: https://crrev.com/6ae5bda04b7bc9a539b568d5cb5d724594102ad6
Cr-Commit-Position: refs/heads/master@{#418083}

[fgorski, 2016-09-11 05:05:33]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-11 05:05:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-11 05:50:12]

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

[commit-bot: I haz the power, 2016-09-11 05:50:13]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-09-12 17:22:27]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[fgorski, 2016-09-12 17:22:51]

fgorski@chromium.org changed reviewers:
+ petewil@chromium.org

[fgorski, 2016-09-12 17:22:52]

PTAL

[commit-bot: I haz the power, 2016-09-12 17:22:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2016-09-12 17:32:51]

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

[Pete Williamson, 2016-09-12 17:55:21]

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

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:284: const char
kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
What does the '"' do in this SQL statement?

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:315: state =
OfflinePageMetadataStore::FAILED_RESET;
Should we log this failure?

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:443: //
Unfortunately we were not able to open DB connection.
This comment is slightly out of context.  We haven't actually checked yet to see
if the connection is bad.  Maybe instead:

// In case we were not able to open a DB connection.
-or-
// Handle failure to open a DB connection.
-or-
<insert blank line above so that the comment seems like part of the if>

[Dmitry Titov, 2016-09-12 18:05:30]

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store.h (right):

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:44:
FAILED_INITIALIZATION,
These 2 'state' enums (LoadStatus and StoreState) are very confusing since they
deliver similar information, especially in the code where they are used (w/o
type names since they are not "enum class").

Can this be a single state? Or maybe LoadStatus may loose some duplicate values
(STORE_INIT_FAILED for example)

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

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:68: // Helper
function to return immediately if no database is found.
Lets add to the comment when it returns true/false and that it only calls
callback in case of failure.

[commit-bot: I haz the power, 2016-09-12 18:13:47]

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

[commit-bot: I haz the power, 2016-09-12 18:13:47]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-09-12 21:04:10]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 21:04:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2016-09-12 21:07:43]

Addressed comments. PTAL

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store.h (right):

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store.h:44:
FAILED_INITIALIZATION,
On 2016/09/12 18:05:29, Dmitry Titov wrote:
> These 2 'state' enums (LoadStatus and StoreState) are very confusing since
they
> deliver similar information, especially in the code where they are used (w/o
> type names since they are not "enum class").
> 
> Can this be a single state? Or maybe LoadStatus may loose some duplicate
values
> (STORE_INIT_FAILED for example) 

TODO added. You are correct and this enum is meant to replace the one above.

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

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:284: const char
kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
On 2016/09/12 17:55:20, Pete Williamson wrote:
> What does the '"' do in this SQL statement?

This has nothing to do with SQL, it is a C/C++ compile time string
concatenation.
Notice that OFFLINE_PAGES_TABLE_NAME is a macro.

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:315: state =
OfflinePageMetadataStore::FAILED_RESET;
On 2016/09/12 17:55:21, Pete Williamson wrote:
> Should we log this failure?

I have a bug tracking properly calling the reset functionality and will get to
it. We are actually going to report these values in UMA as well. TODO is added
in the .h file.

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:443: //
Unfortunately we were not able to open DB connection.
On 2016/09/12 17:55:20, Pete Williamson wrote:
> This comment is slightly out of context.  We haven't actually checked yet to
see
> if the connection is bad.  Maybe instead:
> 
> // In case we were not able to open a DB connection.
> -or-
> // Handle failure to open a DB connection.
> -or-
> <insert blank line above so that the comment seems like part of the if>

Done.

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

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:68: // Helper
function to return immediately if no database is found.
On 2016/09/12 18:05:30, Dmitry Titov wrote:
> Lets add to the comment when it returns true/false and that it only calls
> callback in case of failure.

Done.

[Pete Williamson, 2016-09-12 21:52:45]

lgtm

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

https://codereview.chromium.org/2329283002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:284: const char
kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
On 2016/09/12 21:07:42, fgorski wrote:
> On 2016/09/12 17:55:20, Pete Williamson wrote:
> > What does the '"' do in this SQL statement?
> 
> This has nothing to do with SQL, it is a C/C++ compile time string
> concatenation.
> Notice that OFFLINE_PAGES_TABLE_NAME is a macro. 

Ah, by bad, I thought I saw one at the end of the string, and wondered why we
had an extra.  Sure, that's fine.

[Dmitry Titov, 2016-09-12 21:55:10]

lgtm

[fgorski, 2016-09-12 21:57:08]

The CQ bit was unchecked by fgorski@chromium.org

[fgorski, 2016-09-12 21:57:09]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-09-12 21:57:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 22:46:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-12 22:48:41]

Description was changed from

==========
[Offline pages] OPM SQL store: moving load to constructor, updating read to
GetOfflinePages

BUG=645522
==========

to

==========
[Offline pages] OPM SQL store: moving load to constructor, updating read to
GetOfflinePages

BUG=645522

Committed: https://crrev.com/6ae5bda04b7bc9a539b568d5cb5d724594102ad6
Cr-Commit-Position: refs/heads/master@{#418083}
==========

[commit-bot: I haz the power, 2016-09-12 22:48:43]

Patchset 3 (id:??) landed as
https://crrev.com/6ae5bda04b7bc9a539b568d5cb5d724594102ad6
Cr-Commit-Position: refs/heads/master@{#418083}