Adding a new component, Previews.

Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this
will be consumed by //components/data_reduction_proxy/ and
//components/offline_pages/.

BUG=615563
Committed: https://crrev.com/4adf64f3261af5348c5675a8eace7453f319958c
Cr-Commit-Position: refs/heads/master@{#408444}

[RyanSturm, 2016-07-26 17:39:11]

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

[commit-bot: I haz the power, 2016-07-26 17:39:30]

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

[RyanSturm, 2016-07-26 17:45:11]

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

[commit-bot: I haz the power, 2016-07-26 17:45:32]

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

[RyanSturm, 2016-07-26 17:53:44]

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

[commit-bot: I haz the power, 2016-07-26 17:53:51]

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

[RyanSturm, 2016-07-26 17:54:06]

Patchset #2 (id:20001) has been deleted

[RyanSturm, 2016-07-26 17:54:51]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2016-07-26 17:55:44]

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

[RyanSturm, 2016-07-26 17:55:53]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-07-26 17:56:02]

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

[RyanSturm, 2016-07-26 18:03:53]

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

[commit-bot: I haz the power, 2016-07-26 18:04:15]

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

[RyanSturm, 2016-07-26 18:04:38]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-07-26 18:28:20]

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

[commit-bot: I haz the power, 2016-07-26 18:28:21]

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

[RyanSturm, 2016-07-26 18:38:13]

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

[commit-bot: I haz the power, 2016-07-26 18:38:50]

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

[RyanSturm, 2016-07-26 19:47:40]

ryansturm@chromium.org changed reviewers:
+ bengr@chromium.org

[RyanSturm, 2016-07-26 19:47:41]

bengr: PTAL, thanks

[commit-bot: I haz the power, 2016-07-26 19:52:31]

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

[commit-bot: I haz the power, 2016-07-26 19:52:31]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-07-26 20:11:36]

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

[tbansal1, 2016-07-26 20:11:37]

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

[tbansal1, 2016-07-26 20:11:38]

Looked at this CL as part of reviewing
https://codereview.chromium.org/2166363003

https://codereview.chromium.org/2179863003/diff/100001/components/previews/PR...
File components/previews/PRESUBMIT.py (right):

https://codereview.chromium.org/2179863003/diff/100001/components/previews/PR...
components/previews/PRESUBMIT.py:5: """Top-level presubmit script for the
data_reduction_proxy component.
s/data_reduction_proxy/??/

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
File components/previews/previews_experiments.cc (right):

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:17:
"DataReductionProxyClientSideExperimentsFieldTrial";
I do not think that the field trial name should contain DRP.

s/DataReductionProxyClientSideExperimentsFieldTrial/ClientSidePreviewsFieldTrial/
maybe?

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:29: return
base::FieldTrialList::FindFullName(kClientSideExperimentsFieldTrial) ==
Check if prefix is kEnabled (instead of exact matching)

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:38: &client_side_params))
Add braces

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:44: return it->second == "1";
s/"1"/"true"/

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:44: return it->second == "1";
nit: May be combine with above:
return it != ..end() && it->second == "true";

[tbansal1, 2016-07-26 20:11:38]

Looked at this CL as part of reviewing
https://codereview.chromium.org/2166363003

[tbansal1, 2016-07-26 20:11:58]

btw: lgtm since all comments are nits.

[RyanSturm, 2016-07-26 20:42:36]

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

[commit-bot: I haz the power, 2016-07-26 20:43:27]

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-07-26 21:36:47]

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

[commit-bot: I haz the power, 2016-07-26 21:36:48]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-07-26 21:56:52]

bengr: PTAL

https://codereview.chromium.org/2179863003/diff/100001/components/previews/PR...
File components/previews/PRESUBMIT.py (right):

https://codereview.chromium.org/2179863003/diff/100001/components/previews/PR...
components/previews/PRESUBMIT.py:5: """Top-level presubmit script for the
data_reduction_proxy component.
On 2016/07/26 20:11:37, tbansal1 wrote:
> s/data_reduction_proxy/??/

Done.

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
File components/previews/previews_experiments.cc (right):

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:17:
"DataReductionProxyClientSideExperimentsFieldTrial";
On 2016/07/26 20:11:37, tbansal1 wrote:
> I do not think that the field trial name should contain DRP.
> 
>
s/DataReductionProxyClientSideExperimentsFieldTrial/ClientSidePreviewsFieldTrial/
> maybe?

Done.

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:29: return
base::FieldTrialList::FindFullName(kClientSideExperimentsFieldTrial) ==
On 2016/07/26 20:11:37, tbansal1 wrote:
> Check if prefix is kEnabled (instead of exact matching)

Done.

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:38: &client_side_params))
On 2016/07/26 20:11:37, tbansal1 wrote:
> Add braces

Done.

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:44: return it->second == "1";
On 2016/07/26 20:11:37, tbansal1 wrote:
> s/"1"/"true"/

Done.

https://codereview.chromium.org/2179863003/diff/100001/components/previews/pr...
components/previews/previews_experiments.cc:44: return it->second == "1";
On 2016/07/26 20:11:37, tbansal1 wrote:
> nit: May be combine with above:
> return it != ..end() && it->second == "true";

Done.

[RyanSturm, 2016-07-26 22:00:18]

ryansturm@chromium.org changed reviewers:
+ caitkp@chromium.org

[RyanSturm, 2016-07-26 22:00:19]

bengr, caitkp: PTAL

[bengr, 2016-07-26 23:54:22]

lgtm. Please address the nits.

https://codereview.chromium.org/2179863003/diff/120001/components/previews/RE...
File components/previews/README (right):

https://codereview.chromium.org/2179863003/diff/120001/components/previews/RE...
components/previews/README:2: web pages to improve data savings or improve
performance.

nit: or improve performance -> and/or performance

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
File components/previews/previews_experiments.cc (right):

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:17: const char
kClientSidePreviewsFieldTrial[] = "ClientSidePreviewsFieldTrial";
I would use the string "ClientSidePreviews" instead of
"ClientSidePreviewsFieldTrial"

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:29: return base::StartsWith(
Add a comment that says:
// By convention, an experiment in the client-side previews study enables use of
at least one client-side previews optimization if its name begins with
"Enabled."

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:34: bool
IsIncludedInOfflinePagesSlowConnectionFieldTrial() {
Change the name to IsOfflinePreviewsEnabled()

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:37: std::map<std::string,
std::string> client_side_params;
client_side_params -> experiment_params

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
File components/previews/previews_experiments.h (right):

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.h:10: // Returns true if any client
side previews experiment is active.
client side -> client-side

[blundell, 2016-07-27 08:05:31]

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

[blundell, 2016-07-27 08:05:32]

drive-by: Can you please add to the CL description the motivation for this
feature being added to //components rather than //chrome? I assume it's because
it's going to be consumed by //components/data_reduction_proxy?

[RyanSturm, 2016-07-27 16:05:59]

Description was changed from

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

BUG=615563
==========

to

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this will be
consumed by //components/data_reduction_proxy/ and //components/offline_pages/. 

BUG=615563
==========

[RyanSturm, 2016-07-27 16:18:30]

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

[commit-bot: I haz the power, 2016-07-27 16:18:42]

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

[RyanSturm, 2016-07-27 16:27:48]

https://codereview.chromium.org/2179863003/diff/120001/components/previews/RE...
File components/previews/README (right):

https://codereview.chromium.org/2179863003/diff/120001/components/previews/RE...
components/previews/README:2: web pages to improve data savings or improve
performance.
On 2016/07/26 23:54:21, bengr wrote:
> 
> nit: or improve performance -> and/or performance

Done.

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
File components/previews/previews_experiments.cc (right):

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:17: const char
kClientSidePreviewsFieldTrial[] = "ClientSidePreviewsFieldTrial";
On 2016/07/26 23:54:21, bengr wrote:
> I would use the string "ClientSidePreviews" instead of
> "ClientSidePreviewsFieldTrial"

Done.

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:29: return base::StartsWith(
On 2016/07/26 23:54:21, bengr wrote:
> Add a comment that says:
> // By convention, an experiment in the client-side previews study enables use
of
> at least one client-side previews optimization if its name begins with
> "Enabled."

Done.

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:34: bool
IsIncludedInOfflinePagesSlowConnectionFieldTrial() {
On 2016/07/26 23:54:21, bengr wrote:
> Change the name to IsOfflinePreviewsEnabled()

Done.

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.cc:37: std::map<std::string,
std::string> client_side_params;
On 2016/07/26 23:54:21, bengr wrote:
> client_side_params -> experiment_params

Done.

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
File components/previews/previews_experiments.h (right):

https://codereview.chromium.org/2179863003/diff/120001/components/previews/pr...
components/previews/previews_experiments.h:10: // Returns true if any client
side previews experiment is active.
On 2016/07/26 23:54:21, bengr wrote:
> client side -> client-side

Done.

[RyanSturm, 2016-07-27 16:50:18]

Description was changed from

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this will be
consumed by //components/data_reduction_proxy/ and //components/offline_pages/. 

BUG=615563
==========

to

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this
will be consumed by //components/data_reduction_proxy/ and
//components/offline_pages/. 

BUG=615563
==========

[commit-bot: I haz the power, 2016-07-27 17:39:21]

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

[commit-bot: I haz the power, 2016-07-27 17:39:22]

Dry run: This issue passed the CQ dry run.

[Cait (Slow), 2016-07-28 15:38:15]

lgtm

[RyanSturm, 2016-07-28 15:56:24]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-07-28 15:56:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
tbansal@chromium.org, bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2179863003/#ps140001
(title: "bengr comments")

[commit-bot: I haz the power, 2016-07-28 15:56: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-07-28 18:52:09]

Description was changed from

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this
will be consumed by //components/data_reduction_proxy/ and
//components/offline_pages/. 

BUG=615563
==========

to

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this
will be consumed by //components/data_reduction_proxy/ and
//components/offline_pages/. 

BUG=615563
==========

[commit-bot: I haz the power, 2016-07-28 18:52:11]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-07-28 18:53:49]

Description was changed from

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this
will be consumed by //components/data_reduction_proxy/ and
//components/offline_pages/. 

BUG=615563
==========

to

==========
Adding a new component, Previews.

This component will focus on reducing the data and page load performance
footprint of loading pages by performing various optimizations to
transform the content of those pages.

This is being added to //components (rather than //chrome) because this
will be consumed by //components/data_reduction_proxy/ and
//components/offline_pages/.

BUG=615563

Committed: https://crrev.com/4adf64f3261af5348c5675a8eace7453f319958c
Cr-Commit-Position: refs/heads/master@{#408444}
==========

[commit-bot: I haz the power, 2016-07-28 18:53:51]

Patchset 5 (id:??) landed as
https://crrev.com/4adf64f3261af5348c5675a8eace7453f319958c
Cr-Commit-Position: refs/heads/master@{#408444}