blimp: Add settings to allow the client to download the whole page.

blimp: Add settings to allow the client to download the whole page.

- Add a settings feature to allow the engine and client to send global
  settings flags
- Add a switch on the client to request the whole document.

BUG=591575
Committed: https://crrev.com/00c406052433085e22fa4f73d990c8e4ff3320b3
Cr-Commit-Position: refs/heads/master@{#383547}

[Khushal, 2016-03-19 02:11:07]

khushalsagar@chromium.org changed reviewers:
+ haibinlu@chromium.org, nyquist@chromium.org

[haibinlu, 2016-03-21 18:25:23]

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
File blimp/client/feature/settings_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.cc:42: callback.Run(net::OK);
NOTREACHED() << "<error message here>";

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
File blimp/client/feature/settings_feature.h (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:15: // The feature is used to send and
receive global settings from the engine.
does client need to receive settings from Engine?

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:26: void SetRecordWholeDocument(bool
record_whole_document);
use name consistently? 

"record" --> "download"

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:37: bool record_whole_document_;
ditto

https://codereview.chromium.org/1810213009/diff/1/blimp/common/proto/settings...
File blimp/common/proto/settings.proto (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/common/proto/settings...
blimp/common/proto/settings.proto:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
2016

https://codereview.chromium.org/1810213009/diff/1/blimp/common/proto/settings...
blimp/common/proto/settings.proto:24: message ClientSettingsMessage {
what global settings will Engine send to Client? 

remove client/engine settings message, and have a one settings message here,
which list all settings fields?

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:23:
settings_feature_->AddObserver(this);
Shall we avoid dependency between features?

Discussed with Kevin on the design. we can have a simple Settings object owned
by SettingsManager, which is owned by EngineSession.

SettingsManager has the nested observer (add/remove observer). 

   --Add/RemoveObserver
   --UpdateSettings(Settings&) // called by SettingsFeature
   --Settings settings()
   --Observer
      * OnSettingsUpdated()

WDYT?

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_settings_feature.h (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:22: // Called when the
WebPreferences set from the client have changed.
can you list a few web preferences in the method doc?

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:31: void
set_outgoing_message_processor(
remove? since Engine does not send settings to Client.

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:37: // Called to update the
WebPreferences for the RenderViewHost.
this method goes to SettingsManager if we go with that approach.

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:48: // Used to send
BlimpMessage::TAB_CONTROL messages to the engine.
Settings message

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.h:176: // Handles all incoming and
outgoing messages for type SETTINGS.
not outgoing.

[Khushal, 2016-03-22 04:11:43]

Added the EngineSettings and SettingsManager, made sense to keep these separate.
PTAL.

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
File blimp/client/feature/settings_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/03/21 18:25:22, haibinlu wrote:
> 2016

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.cc:42: callback.Run(net::OK);
On 2016/03/21 18:25:22, haibinlu wrote:
> NOTREACHED() << "<error message here>";

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
File blimp/client/feature/settings_feature.h (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/03/21 18:25:22, haibinlu wrote:
> 2016

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:15: // The feature is used to send and
receive global settings from the engine.
On 2016/03/21 18:25:22, haibinlu wrote:
> does client need to receive settings from Engine?

Not right now. Removed that code.

https://codereview.chromium.org/1810213009/diff/1/blimp/client/feature/settin...
blimp/client/feature/settings_feature.h:26: void SetRecordWholeDocument(bool
record_whole_document);
On 2016/03/21 18:25:22, haibinlu wrote:
> use name consistently? 
> 
> "record" --> "download" 

So the name of the web preference is record_whole_document and that's what seems
better for the proto, so it is mapped to the web preference flag name.
I wasn't sure if record_whole_document sounds correct for the client switch
name, I can change it to that if it would be better.

https://codereview.chromium.org/1810213009/diff/1/blimp/common/proto/settings...
File blimp/common/proto/settings.proto (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/common/proto/settings...
blimp/common/proto/settings.proto:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/03/21 18:25:22, haibinlu wrote:
> 2016

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/common/proto/settings...
blimp/common/proto/settings.proto:24: message ClientSettingsMessage {
On 2016/03/21 18:25:22, haibinlu wrote:
> what global settings will Engine send to Client? 
> 
> remove client/engine settings message, and have a one settings message here,
> which list all settings fields?

We don't have any settings that need to be sent to the client right now, but if
we do later, this feature would be the place to add them. I have removed the
client settings message, but would it be better to have the Settings message
include a distinct message type for whether the settings are intended for the
engine?

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:23:
settings_feature_->AddObserver(this);
On 2016/03/21 18:25:22, haibinlu wrote:
> Shall we avoid dependency between features?
> 
> Discussed with Kevin on the design. we can have a simple Settings object owned
> by SettingsManager, which is owned by EngineSession.

Sounds like a great idea. I looked at the code again, and it made sense to add a
simple object for all the settings, I've added it as EngineSettings.

> 
> SettingsManager has the nested observer (add/remove observer). 
> 
>    --Add/RemoveObserver
>    --UpdateSettings(Settings&) // called by SettingsFeature
>    --Settings settings()
>    --Observer
>       * OnSettingsUpdated()
> 
> WDYT?

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_settings_feature.h (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/03/21 18:25:22, haibinlu wrote:
> 2016

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:22: // Called when the
WebPreferences set from the client have changed.
On 2016/03/21 18:25:22, haibinlu wrote:
> can you list a few web preferences in the method doc?

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:31: void
set_outgoing_message_processor(
On 2016/03/21 18:25:22, haibinlu wrote:
> remove? since Engine does not send settings to Client.

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_settings_feature.h:37: // Called to update the
WebPreferences for the RenderViewHost.
On 2016/03/21 18:25:23, haibinlu wrote:
> this method goes to SettingsManager if we go with that approach.

Done.

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1810213009/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.h:176: // Handles all incoming and
outgoing messages for type SETTINGS.
On 2016/03/21 18:25:23, haibinlu wrote:
> not outgoing.

Done.

[haibinlu, 2016-03-22 17:28:39]

looks great! a few nits.

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
File blimp/client/feature/settings_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
blimp/client/feature/settings_feature.cc:41: callback.Run(net::OK);
remove callback.Run(net::OK);

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
File blimp/client/feature/settings_feature.h (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
blimp/client/feature/settings_feature.h:33: // Used to send
BlimpMessage::TAB_CONTROL messages to the engine.
Settings message

https://codereview.chromium.org/1810213009/diff/40001/blimp/common/proto/sett...
File blimp/common/proto/settings.proto (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/common/proto/sett...
blimp/common/proto/settings.proto:17: message EngineSettingsMessage {
you can put record_whole_document directly in SettingsMessage, and remove 
EngineSettingsMessage

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:22:
DCHECK(settings_manager_);
no need for DCHECK since the next line uses it.

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:27:
DCHECK(settings_manager_);
ditto

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:186: void
EngineRenderWidgetFeature::OnWebPreferencesChanged() {
can you add a test case for this method?

[Khushal, 2016-03-25 23:00:22]

Thanks Haibin.
I had overlooked the fact that the WebPreferences can be changed internally in
content, in which case it wouldn't have the changes made by the SettingsManager
in blimp, so I am overriding the method on ContentBrowserClient to set them
instead.
Please take another look.

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
File blimp/client/feature/settings_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
blimp/client/feature/settings_feature.cc:41: callback.Run(net::OK);
On 2016/03/22 17:28:39, haibinlu wrote:
> remove callback.Run(net::OK);

Are we sure we want to do this? In case of an error, the NOTREACHED() would just
log in release builds. If we don't run the callback, it would stall the complete
message pipeline.

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
File blimp/client/feature/settings_feature.h (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/client/feature/se...
blimp/client/feature/settings_feature.h:33: // Used to send
BlimpMessage::TAB_CONTROL messages to the engine.
On 2016/03/22 17:28:39, haibinlu wrote:
> Settings message

Done.

https://codereview.chromium.org/1810213009/diff/40001/blimp/common/proto/sett...
File blimp/common/proto/settings.proto (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/common/proto/sett...
blimp/common/proto/settings.proto:17: message EngineSettingsMessage {
On 2016/03/22 17:28:39, haibinlu wrote:
> you can put record_whole_document directly in SettingsMessage, and remove 
> EngineSettingsMessage

I was putting it in EngineSettingsMessage so in case we add settings that go
from the engine to client, they can distinctly be added here. You think it would
be better if it were in a single message?

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:22:
DCHECK(settings_manager_);
On 2016/03/22 17:28:39, haibinlu wrote:
> no need for DCHECK since the next line uses it.

Dereferencing it in the next line causes a seg fault, the DCHECK would give a
good stack trace as it dies. Makes debugging easier. :)

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:186: void
EngineRenderWidgetFeature::OnWebPreferencesChanged() {
On 2016/03/22 17:28:39, haibinlu wrote:
> can you add a test case for this method?

The method is using RenderViewHost::From(RenderWidgetHost*) which is not a
virtual method on this class. Is there is a good way to mock this in the test?

[haibinlu, 2016-03-25 23:53:13]

lgtm

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:186: void
EngineRenderWidgetFeature::OnWebPreferencesChanged() {
On 2016/03/25 23:00:22, Khushal wrote:
> On 2016/03/22 17:28:39, haibinlu wrote:
> > can you add a test case for this method?
> 
> The method is using RenderViewHost::From(RenderWidgetHost*) which is not a
> virtual method on this class. Is there is a good way to mock this in the test?

can we use RenderViewHostImplTestHarness as in
render_frame_host_manager_unittest.cc?

[Khushal, 2016-03-28 19:20:11]

Thanks Haibin!

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/1810213009/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:186: void
EngineRenderWidgetFeature::OnWebPreferencesChanged() {
On 2016/03/25 23:53:13, haibinlu wrote:
> On 2016/03/25 23:00:22, Khushal wrote:
> > On 2016/03/22 17:28:39, haibinlu wrote:
> > > can you add a test case for this method?
> > 
> > The method is using RenderViewHost::From(RenderWidgetHost*) which is not a
> > virtual method on this class. Is there is a good way to mock this in the
test?
> 
> can we use RenderViewHostImplTestHarness as in
> render_frame_host_manager_unittest.cc?

We could possibly use it. I'll take a look into structuring the tests using it
in a separate patch.

[Khushal, 2016-03-28 19:20:16]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-03-28 19:20:38]

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

[commit-bot: I haz the power, 2016-03-28 20:00:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-28 20:02:29]

Description was changed from

==========
blimp: Add settings to allow the client to download the whole page.

- Add a settings feature to allow the engine and client to send global
  settings flags
- Add a switch on the client to request the whole document.

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

to

==========
blimp: Add settings to allow the client to download the whole page.

- Add a settings feature to allow the engine and client to send global
  settings flags
- Add a switch on the client to request the whole document.

BUG=591575

Committed: https://crrev.com/00c406052433085e22fa4f73d990c8e4ff3320b3
Cr-Commit-Position: refs/heads/master@{#383547}
==========

[commit-bot: I haz the power, 2016-03-28 20:02:30]

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