Store original request URL in offline page metadata table

Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
TBR=bauerb@chromium.org

Committed: https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2
Cr-Commit-Position: refs/heads/master@{#432632}

[jianli, 2016-11-10 21:59:01]

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

[jianli, 2016-11-10 21:59:05]

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

[commit-bot: I haz the power, 2016-11-10 21:59:40]

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-11-10 22:11:38]

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

[commit-bot: I haz the power, 2016-11-10 22:11:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fgorski, 2016-11-10 23:56:14]

mostly looks good.

Please consider adding shess@ for a quick look at our SQL store. I don't expect
surprises, but you never know.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:223:
ASSERT_TRUE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "title"));
this needs updating to check for the new column.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:586: //
Loads a string with schema from M54.
M55

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:803:
offline_page.original_url = GURL("https://example.com/bar");
please make a similar update to one of the add* tests.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:37: // * In M56
original_url was added.
Thank you for remembering to do this!

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:51: struct SavePageInfo {
How about renaming it to SavePageParams?
Why? Because the purpose of this structure is to simplify SavePage interface,
and I hope we won't use it for anything else. We already have relevant
structures for other applications:
* OfflinePageInfo
* SavePageRequest

Please, update documentation respectively.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:104: // Attempts to save a page
addressed by |url| offline. Requires that the model
url is not longer passed here.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:361: offline_id =
GenerateOfflineId();
why don't you set it on the info/params object?

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:730: int64_t offline_id,
needed? (relevant with the prior comment)

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:129: void
AsyncSavePageWithArchiver(
nit: I like to put Async/Sync at the end. (Sorts better in code search) Up to
you thou.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:348:
SavePageWithArchiver(url, client_id, std::move(archiver));
please don't chain methods like that.
It is better to call final implementation (Async/Pump).

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:353:
SavePageWithArchiverResult(
Same here. Please update.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:491: ASSERT_EQ(1UL,
offline_pages.size());
Good!

[jianli, 2016-11-16 01:21:16]

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

[commit-bot: I haz the power, 2016-11-16 01:21:45]

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

[jianli, 2016-11-16 01:25:40]

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:223:
ASSERT_TRUE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "title"));
On 2016/11/10 23:56:13, fgorski wrote:
> this needs updating to check for the new column.

I think this is used to build the old store which did not contain the new
column.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:586: //
Loads a string with schema from M54.
On 2016/11/10 23:56:13, fgorski wrote:
> M55

Done.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:803:
offline_page.original_url = GURL("https://example.com/bar");
On 2016/11/10 23:56:13, fgorski wrote:
> please make a similar update to one of the add* tests.

Done.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:37: // * In M56
original_url was added.
On 2016/11/10 23:56:13, fgorski wrote:
> Thank you for remembering to do this!

Acknowledged.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:51: struct SavePageInfo {
On 2016/11/10 23:56:13, fgorski wrote:
> How about renaming it to SavePageParams?
> Why? Because the purpose of this structure is to simplify SavePage interface,
> and I hope we won't use it for anything else. We already have relevant
> structures for other applications:
> * OfflinePageInfo
> * SavePageRequest
> 
> Please, update documentation respectively.

Done.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:104: // Attempts to save a page
addressed by |url| offline. Requires that the model
On 2016/11/10 23:56:13, fgorski wrote:
> url is not longer passed here.

Done.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:361: offline_id =
GenerateOfflineId();
On 2016/11/10 23:56:13, fgorski wrote:
> why don't you set it on the info/params object?

I can't do it because save_page_info is const.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:730: int64_t offline_id,
On 2016/11/10 23:56:13, fgorski wrote:
> needed? (relevant with the prior comment)

Yes because save_page_info can't be changed. Also I think passing a finalized
offline id sounds better to try to reuse the proposed offline id.

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:129: void
AsyncSavePageWithArchiver(
On 2016/11/10 23:56:13, fgorski wrote:
> nit: I like to put Async/Sync at the end. (Sorts better in code search) Up to
> you thou.

Done.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:348:
SavePageWithArchiver(url, client_id, std::move(archiver));
On 2016/11/10 23:56:13, fgorski wrote:
> please don't chain methods like that.
> It is better to call final implementation (Async/Pump).

Done.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:353:
SavePageWithArchiverResult(
On 2016/11/10 23:56:14, fgorski wrote:
> Same here. Please update.

Done.

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:491: ASSERT_EQ(1UL,
offline_pages.size());
On 2016/11/10 23:56:13, fgorski wrote:
> Good!

Acknowledged.

[commit-bot: I haz the power, 2016-11-16 02:39:02]

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

[commit-bot: I haz the power, 2016-11-16 02:39:03]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-11-16 05:20:25]

lgtm, but please update the test (see my response)

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:223:
ASSERT_TRUE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "title"));
On 2016/11/16 01:25:39, jianli wrote:
> On 2016/11/10 23:56:13, fgorski wrote:
> > this needs updating to check for the new column.
> 
> I think this is used to build the old store which did not contain the new
> column.

Exactly, that is why you assert false, as in line 129, where the title column
was missing.
ASSERT_FALSE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "new_column"));

[jianli, 2016-11-16 21:17:48]

Description was changed from

==========
Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
==========

to

==========
Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
TBR=bauerb@chromium.org
==========

[jianli, 2016-11-16 21:18:12]

jianli@chromium.org changed reviewers:
+ bauerb@chromium.org

[jianli, 2016-11-16 21:18:13]

TBR bauerb@chromium.org for simple change to chrome/browser/ui/webui

[jianli, 2016-11-16 21:18:22]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2016-11-16 21:19:44]

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-11-16 21:30:58]

Description was changed from

==========
Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
TBR=bauerb@chromium.org
==========

to

==========
Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
TBR=bauerb@chromium.org
==========

[commit-bot: I haz the power, 2016-11-16 21:31:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-16 21:39:36]

Description was changed from

==========
Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
TBR=bauerb@chromium.org
==========

to

==========
Store original request URL in offline page metadata table

This will be used to support triggering redirect from original request
URL to last committed URL which is used to identify the offline page.

BUG=618716
TBR=bauerb@chromium.org

Committed: https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2
Cr-Commit-Position: refs/heads/master@{#432632}
==========

[commit-bot: I haz the power, 2016-11-16 21:39:38]

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

[jianli, 2016-11-16 21:58:11]

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

https://codereview.chromium.org/2484223005/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:223:
ASSERT_TRUE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "title"));
On 2016/11/16 05:20:25, fgorski wrote:
> On 2016/11/16 01:25:39, jianli wrote:
> > On 2016/11/10 23:56:13, fgorski wrote:
> > > this needs updating to check for the new column.
> > 
> > I think this is used to build the old store which did not contain the new
> > column.
> 
> Exactly, that is why you assert false, as in line 129, where the title column
> was missing.
> ASSERT_FALSE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1,
"new_column"));

I applied the change to the wrong branch. I will include this change in my next
patch you're currently reviewing.