content: Make record_whole_document available for all platforms.

content: Make record_whole_document available for all platforms.

Move record_whole_document in WebPreferences from OS_ANDROID to all platforms. This will be used on the blimp engine to record the whole page rather than just the interest rect.

BUG=591575
Committed: https://crrev.com/2da2b2322423c358333c95f017539d835f677884
Cr-Commit-Position: refs/heads/master@{#382937}

[Khushal, 2016-03-17 23:20:23]

khushalsagar@chromium.org changed reviewers:
+ sievers@chromium.org

[Khushal, 2016-03-17 23:22:11]

khushalsagar@chromium.org changed reviewers:
+ palmer@chromium.org

[no sievers, 2016-03-17 23:49:31]

lgtm

[Khushal, 2016-03-21 22:59:45]

Description was changed from

==========
content: Make record_whole_document available for all platforms

This will be used for benchmarking in Blimp to record the whole page
rather than just the interest rect.

BUG= 591575
==========

to

==========
content: Add build flag for Web Preferences for the Blimp Engine

This will be used for benchmarking in Blimp to record the whole page
rather than just the interest rect.

BUG= 591575
==========

[Khushal, 2016-03-21 23:01:28]

Description was changed from

==========
content: Add build flag for Web Preferences for the Blimp Engine

This will be used for benchmarking in Blimp to record the whole page
rather than just the interest rect.

BUG= 591575
==========

to

==========
content: Add build flag for Web Preferences for the Blimp Engine

- Add a build flag to enable web preferences specific to the Blimp
  Engine.
- Move record_whole_document to Android and the Blimp Engine. This will
  be used for benchmarking in Blimp to record the whole page rather than
  just the interest rect.

BUG= 591575
==========

[Khushal, 2016-03-21 23:03:34]

palmer@, I've put the settings being a build flag for web preferences used on
the blimp engine. PTAL.

+wez

[Wez, 2016-03-22 00:18:13]

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

[Wez, 2016-03-22 00:18:14]

https://codereview.chromium.org/1808363002/diff/40001/content/public/common/B...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/1808363002/diff/40001/content/public/common/B...
content/public/common/BUILD.gn:78: defines = [ "ENABLE_BLIMP_WEB_PREFS" ]
Could we instead have this be enable_record_whole_document or something? Seems
strange to bake any understanding of Blimp in at this level.

[Khushal, 2016-03-22 00:21:38]

https://codereview.chromium.org/1808363002/diff/40001/content/public/common/B...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/1808363002/diff/40001/content/public/common/B...
content/public/common/BUILD.gn:78: defines = [ "ENABLE_BLIMP_WEB_PREFS" ]
On 2016/03/22 00:18:14, Wez wrote:
> Could we instead have this be enable_record_whole_document or something? Seems
> strange to bake any understanding of Blimp in at this level.

I assume we would need to add more web prefs for Blimp going forward, for
instance to disable custom fonts. So might be better to put all such changes for
web prefs behind a single flag.

[Khushal, 2016-03-22 01:07:47]

khushalsagar@chromium.org changed reviewers:
+ agrieve@chromium.org

[Khushal, 2016-03-22 01:07:47]

+agrieve, changed it to public_configs like you suggested.

[no sievers, 2016-03-22 18:55:49]

So generally a blimp specific compile time config makes sense to me especially
since we are already expecting that a lot of things that are currently behind
the vague notion of the platform don't work here (because we have two platforms
involved, one that is the host and the other one which is the content target).

But I'm also wondering if having #ifdef all over the place defends the purpose
of having generic configuration structures a bit (such as WebPreferences or
LayerTreeSettings) that ideally the embedder would configure.

And finally it would also be nice if the notion of the blimp engine build config
was a higher level meta-config of specific features somehow, instead of being
visible throughout the codebase in many places.

[Khushal, 2016-03-23 02:18:05]

On 2016/03/22 18:55:49, sievers wrote:
> So generally a blimp specific compile time config makes sense to me especially
> since we are already expecting that a lot of things that are currently behind
> the vague notion of the platform don't work here (because we have two
platforms
> involved, one that is the host and the other one which is the content target).
> 
> But I'm also wondering if having #ifdef all over the place defends the purpose
> of having generic configuration structures a bit (such as WebPreferences or
> LayerTreeSettings) that ideally the embedder would configure.

We have been trying to avoid defining a global define for blimp and rather use
feature specific flags. However, that seemed hard to do in this case since the
aim was to restrict a publicly exposed config for the embedder to the blimp
engine.

In this particular case, we can have the define as allow_record_whole_document,
instead of a blanket flag for blimp, like Wez was also suggesting if that's
preferable.

+dpranke.

> 
> And finally it would also be nice if the notion of the blimp engine build
config
> was a higher level meta-config of specific features somehow, instead of being
> visible throughout the codebase in many places.

[Khushal, 2016-03-23 02:18:41]

khushalsagar@chromium.org changed reviewers:
+ dpranke@chromium.org

[Khushal, 2016-03-23 18:16:00]

khushalsagar@chromium.org changed reviewers:
+ tsepez@chromium.org

[Khushal, 2016-03-23 18:16:01]

+tsepez since palmer@ is OOO.

[Tom Sepez, 2016-03-23 20:03:25]

Messages LGTM.

[Khushal, 2016-03-23 20:37:19]

Description was changed from

==========
content: Add build flag for Web Preferences for the Blimp Engine

- Add a build flag to enable web preferences specific to the Blimp
  Engine.
- Move record_whole_document to Android and the Blimp Engine. This will
  be used for benchmarking in Blimp to record the whole page rather than
  just the interest rect.

BUG= 591575
==========

to

==========
content: Make record_whole_document available for all platforms.

Move record_whole_document in WebPreferences from OS_ANDROID to all platforms.
This will be used on the blimp engine to record the whole page rather than just
the interest rect.

BUG= 591575
==========

[Khushal, 2016-03-23 20:38:21]

On 2016/03/23 20:03:25, Tom Sepez wrote:
> Messages LGTM.

Spoke with Tom, and he agreed that a build time flag is not necessary for this
change, so back to the original patch#1.

[Khushal, 2016-03-23 20:40:01]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-03-23 20:40:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1808363002/#ps80001
(title: "Back to patch #1.")

[commit-bot: I haz the power, 2016-03-23 20:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808363002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808363002/80001

[Dirk Pranke, 2016-03-23 21:43:11]

For the record, this seems like a perfectly reasonable thing to be
unconditionally available everywhere to me, so patchset #1 lgtm , too.

(though we can make adding flags work also, of course).

[commit-bot: I haz the power, 2016-03-23 22:11:45]

Description was changed from

==========
content: Make record_whole_document available for all platforms.

Move record_whole_document in WebPreferences from OS_ANDROID to all platforms.
This will be used on the blimp engine to record the whole page rather than just
the interest rect.

BUG= 591575
==========

to

==========
content: Make record_whole_document available for all platforms.

Move record_whole_document in WebPreferences from OS_ANDROID to all platforms.
This will be used on the blimp engine to record the whole page rather than just
the interest rect.

BUG= 591575
==========

[commit-bot: I haz the power, 2016-03-23 22:11:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-23 22:13:11]

Description was changed from

==========
content: Make record_whole_document available for all platforms.

Move record_whole_document in WebPreferences from OS_ANDROID to all platforms.
This will be used on the blimp engine to record the whole page rather than just
the interest rect.

BUG= 591575
==========

to

==========
content: Make record_whole_document available for all platforms.

Move record_whole_document in WebPreferences from OS_ANDROID to all platforms.
This will be used on the blimp engine to record the whole page rather than just
the interest rect.

BUG= 591575

Committed: https://crrev.com/2da2b2322423c358333c95f017539d835f677884
Cr-Commit-Position: refs/heads/master@{#382937}
==========

[commit-bot: I haz the power, 2016-03-23 22:13:12]

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

[no sievers, 2016-03-24 02:05:24]

On 2016/03/23 21:43:11, Dirk Pranke wrote:
> For the record, this seems like a perfectly reasonable thing to be
> unconditionally available everywhere to me, so patchset #1 lgtm , too.
> 
> (though we can make adding flags work also, of course).

Agreed, let's try to stick with either runtime plumbed through layers so the
embedder or so can set it.
Or compile time where we can set it in one place. But both is weird esp. since
the plumbing requires ifdef'ing all over the place too then.