[Offline Prefetch] Send GeneratePageBundleRequest to the server

[Offline Prefetch] Send GeneratePageBundleRequest to the server

BUG=717305
TEST=new tests

Review-Url: https://codereview.chromium.org/2873383004
Cr-Commit-Position: refs/heads/master@{#471420}
Committed: https://chromium.googlesource.com/chromium/src/+/874c38a6ffddc2794f18a913297cd1cdfbae3526

[jianli, 2017-05-10 22:25:23]

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

[jianli, 2017-05-10 22:26:00]

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

[commit-bot: I haz the power, 2017-05-10 22:26:35]

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, 2017-05-10 22:39:53]

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

[commit-bot: I haz the power, 2017-05-10 22:39:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dewittj, 2017-05-11 17:08:07]

Description was changed from

==========
[Offline Prefetch] Send GeneratePageBundleRequest to the server

BUG=717305
TEST=new tests
==========

to

==========
[Offline Prefetch] Send GeneratePageBundleRequest to the server

BUG=717305
TEST=new tests
==========

[dewittj, 2017-05-11 17:08:07]

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

[dewittj, 2017-05-11 18:46:43]

some partial feedback.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
File components/offline_pages/core/prefetch/generate_page_bundle_request.cc
(right):

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:74: if
(operation.result_case() == proto::Operation::kError) {
kError is not defined?

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:87:
DVLOG(1) << "metadata not found in GeneratePageBundle response";
Is metadata guaranteed?

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_types.h (right):

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:17: enum class
RequestStatus {
This is too generic a name, I think.  PrefetchRequestFetcherStatus? or something
would be better.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:38: PENDING_TO_RENDER,
nit: PENDING

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:40: FAILED_TO_RENDER,
nit: FAILED

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:46: PageInfo() :
status(RenderStatus::FAILED_TO_RENDER), body_length(0) {}
nit: remove constructor, use:

RenderStatus staus = RenderStatus::FAILED;

down on line 53.  Same with body_length.

[jianli, 2017-05-11 20:45:09]

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

[jianli, 2017-05-11 20:45:14]

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
File components/offline_pages/core/prefetch/generate_page_bundle_request.cc
(right):

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:74: if
(operation.result_case() == proto::Operation::kError) {
On 2017/05/11 18:46:43, dewittj wrote:
> kError is not defined?

It is defined in the generated file operation.pb.h to identify oneof types.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:87:
DVLOG(1) << "metadata not found in GeneratePageBundle response";
On 2017/05/11 18:46:43, dewittj wrote:
> Is metadata guaranteed?

Yes. The server will return something in metadata if operation is not done.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_types.h (right):

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:17: enum class
RequestStatus {
On 2017/05/11 18:46:43, dewittj wrote:
> This is too generic a name, I think.  PrefetchRequestFetcherStatus? or
something
> would be better.  

Changed to PrefetchRequestStatus. This enum is used in both fetcher and proto
parsing. So I don't want it to be tied with fetcher only.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:38: PENDING_TO_RENDER,
On 2017/05/11 18:46:43, dewittj wrote:
> nit: PENDING

Done.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:40: FAILED_TO_RENDER,
On 2017/05/11 18:46:43, dewittj wrote:
> nit: FAILED

Done.

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_types.h:46: PageInfo() :
status(RenderStatus::FAILED_TO_RENDER), body_length(0) {}
On 2017/05/11 18:46:43, dewittj wrote:
> nit: remove constructor, use:
> 
> RenderStatus staus = RenderStatus::FAILED;
> 
> down on line 53.  Same with body_length.

Moved initialization to in place.

I can't remove the default constructor because I have to add copy constructor in
order to fix error "Complex class/struct needs an explicit out-of-line copy
constructor". With copy constructor, default constructor has to be present.

[commit-bot: I haz the power, 2017-05-11 20:46:30]

Dry run: CQ is trying da patch.

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

[dewittj, 2017-05-11 21:51:48]

in general this lgtm.  Thanks!

nits follow:

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
File components/offline_pages/core/prefetch/generate_page_bundle_request.cc
(right):

https://codereview.chromium.org/2873383004/diff/20001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:29: const
std::vector<std::string>& page_urls,
why is this strings and not GURL?

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File components/offline_pages/core/prefetch/generate_page_bundle_request.cc
(right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:96: if
(!base::StartsWith(any_data.type_url(), "type.googleapis.com/",
We should probably stick type_url into a GURL and then extract the interesting
parts.

Or we should get an exact URL from the server folks and check it here.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File
components/offline_pages/core/prefetch/generate_page_bundle_request_unittest.cc
(right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request_unittest.cc:55:
class DoneOperationBuilder : public OperationBuilder {
Are there any textproto decoders in Chrome? It might be nice to have the test
cases expressed as text protocol buffers directly.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request_unittest.cc:62:
operation.set_done(true);
These two builders only differ in one line.  Can this be factored into a helper
function so there's less duplication?

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_request_fetcher.cc (right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:29:
net::DefineNetworkTrafficAnnotation("offline_pages", R"(
I wonder if this should be offline_prefetch still.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_request_test_base.h
(right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_request_test_base.h:16: class
PrefetchRequestTestBase : public testing::Test {
nit: Class comment please

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_request_test_base.h:30:
base::MessageLoop message_loop_;
is this used?

[commit-bot: I haz the power, 2017-05-11 22:29:26]

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

[commit-bot: I haz the power, 2017-05-11 22:29:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jianli, 2017-05-11 23:07:28]

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File components/offline_pages/core/prefetch/generate_page_bundle_request.cc
(right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request.cc:96: if
(!base::StartsWith(any_data.type_url(), "type.googleapis.com/",
On 2017/05/11 21:51:48, dewittj wrote:
> We should probably stick type_url into a GURL and then extract the interesting
> parts.
> 
> Or we should get an exact URL from the server folks and check it here.

Changed to validate the full URL.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File
components/offline_pages/core/prefetch/generate_page_bundle_request_unittest.cc
(right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request_unittest.cc:55:
class DoneOperationBuilder : public OperationBuilder {
On 2017/05/11 21:51:48, dewittj wrote:
> Are there any textproto decoders in Chrome? It might be nice to have the test
> cases expressed as text protocol buffers directly.

I don't find one.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/generate_page_bundle_request_unittest.cc:62:
operation.set_done(true);
On 2017/05/11 21:51:48, dewittj wrote:
> These two builders only differ in one line.  Can this be factored into a
helper
> function so there's less duplication?

Done.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_request_fetcher.cc (right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:29:
net::DefineNetworkTrafficAnnotation("offline_pages", R"(
On 2017/05/11 21:51:48, dewittj wrote:
> I wonder if this should be offline_prefetch still. 

Done. Thanks for catching this that is due to global renaming effort.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_request_test_base.h
(right):

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_request_test_base.h:16: class
PrefetchRequestTestBase : public testing::Test {
On 2017/05/11 21:51:48, dewittj wrote:
> nit: Class comment please

Done.

https://codereview.chromium.org/2873383004/diff/60001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_request_test_base.h:30:
base::MessageLoop message_loop_;
On 2017/05/11 21:51:48, dewittj wrote:
> is this used?

This is needed in order to create a TaskRunner. Switched to creating
TestSimpleTaskRunner, instead of relying on MessageLoop.

[jianli, 2017-05-11 23:07:56]

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

[commit-bot: I haz the power, 2017-05-11 23:09:07]

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, 2017-05-12 01:03:52]

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

[commit-bot: I haz the power, 2017-05-12 01:03:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jianli, 2017-05-12 18:30:25]

The CQ bit was checked by jianli@chromium.org

[jianli, 2017-05-12 18:30:26]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2873383004/#ps80001
(title: "Address more feedback")

[commit-bot: I haz the power, 2017-05-12 18:31:40]

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, 2017-05-12 20:00:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494613825687360,
"parent_rev": "38f94f533d809b4202db39ac8c17be4442eb4256", "commit_rev":
"874c38a6ffddc2794f18a913297cd1cdfbae3526"}

[commit-bot: I haz the power, 2017-05-12 20:00:23]

Description was changed from

==========
[Offline Prefetch] Send GeneratePageBundleRequest to the server

BUG=717305
TEST=new tests
==========

to

==========
[Offline Prefetch] Send GeneratePageBundleRequest to the server

BUG=717305
TEST=new tests

Review-Url: https://codereview.chromium.org/2873383004
Cr-Commit-Position: refs/heads/master@{#471420}
Committed:
https://chromium.googlesource.com/chromium/src/+/874c38a6ffddc2794f18a913297c...
==========

[commit-bot: I haz the power, 2017-05-12 20:00:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/874c38a6ffddc2794f18a913297c...