Settings should not call into inspector

Settings should not call into inspector

Settings should not call into inspector,
Inspector calls in settings are moved into actual
call sites.

BUG=327476
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166295

[gnana, 2014-01-24 14:29:11]

Please have a look.

[pfeldman, 2014-01-24 16:25:20]

I think this approach breaks scenarios such as:

1) User disables scripts from devtools while debugging, then disables / enables
them in chrome settings
2) User turns on device emulation in devtools, then toggles accessibility option
that changes font size or similar.

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/In...
File Source/core/inspector/InspectorPageAgent.h (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorPageAgent.h:130: bool overrideTextAutosizing();
These are now private then.

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:112: void
PageRuntimeAgent::scriptsEnabled(Frame* frame, bool isEnabled)
You don't need these.

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.h (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.h:58: void scriptsEnabled(Frame*, bool
isEnabled);
You don't need these.

https://codereview.chromium.org/146683003/diff/90001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/page/Page.cp...
Source/core/page/Page.cpp:513:
InspectorInstrumentation::textAutosizingEnabled(mainFrame(),
settings().textAutosizingEnabled());
I don't think we need this complexity in page, consider sending
InspectorInstrumentation::settingsChanged(this, changeType) in the end of this
method instead.

In fact, InspectorInstrumentation:: is only used in core for sniffing and
sending instrumenting signals within core to agents. Changes to settings are
more like messages from embedder (ex-WebKit) that get to the agents via
InspectorController. So I would suggest to

- call inspectorController()->settingsChanged(changeType) here
- in InspectorController, cache the pointer to InspectorPageAgent and delegate
settingsChanged() to it.

[gnana, 2014-01-27 16:31:06]

Thanks pfeldman for the review.
I verified the two usecases mentioned above after the patch and they are working
fine.

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/In...
File Source/core/inspector/InspectorPageAgent.h (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/In...
Source/core/inspector/InspectorPageAgent.h:130: bool overrideTextAutosizing();
On 2014/01/24 16:25:21, pfeldman wrote:
> These are now private then.

Done.

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.cpp (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.cpp:112: void
PageRuntimeAgent::scriptsEnabled(Frame* frame, bool isEnabled)
On 2014/01/24 16:25:21, pfeldman wrote:
> You don't need these.

Done.

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
File Source/core/inspector/PageRuntimeAgent.h (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/inspector/Pa...
Source/core/inspector/PageRuntimeAgent.h:58: void scriptsEnabled(Frame*, bool
isEnabled);
On 2014/01/24 16:25:21, pfeldman wrote:
> You don't need these.

Done.

https://codereview.chromium.org/146683003/diff/90001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/146683003/diff/90001/Source/core/page/Page.cp...
Source/core/page/Page.cpp:513:
InspectorInstrumentation::textAutosizingEnabled(mainFrame(),
settings().textAutosizingEnabled());
On 2014/01/24 16:25:21, pfeldman wrote:
> I don't think we need this complexity in page, consider sending
> InspectorInstrumentation::settingsChanged(this, changeType) in the end of this
> method instead.
> 
> In fact, InspectorInstrumentation:: is only used in core for sniffing and
> sending instrumenting signals within core to agents. Changes to settings are
> more like messages from embedder (ex-WebKit) that get to the agents via
> InspectorController. So I would suggest to
> 
> - call inspectorController()->settingsChanged(changeType) here
> - in InspectorController, cache the pointer to InspectorPageAgent and delegate
> settingsChanged() to it.

Done.

[pfeldman, 2014-01-27 16:43:03]

On 2014/01/27 16:31:06, gnana wrote:
> Thanks pfeldman for the review.
> I verified the two usecases mentioned above after the patch and they are
working
> fine.

Do you have a logical explanation for that (because I am missing one)?

[gnana, 2014-01-27 19:28:06]

On 2014/01/27 16:43:03, pfeldman wrote:
> On 2014/01/27 16:31:06, gnana wrote:
> > Thanks pfeldman for the review.
> > I verified the two usecases mentioned above after the patch and they are
> working
> > fine.
> 
> Do you have a logical explanation for that (because I am missing one)?

This will not break these scenarios. because with the patch i m trying to do the
same things.
Before the patch instrumentation::scriptsenabled was called which internally
called pageagent's scriptsenabled.
With the patch i m doing same thing but only difference is the path.

Before the patch overrideTextAutosizing and overrideFontScaleFactor was
overriding Settings::textAutosizingEnabled() and
Settings::deviceScaleAdjustment() respectively. This override was effective only
when devicemetrics in pageagent was changed.
With the patch, i m caching these settings in pageagent and when pageagent is
enabled and devicemetrics changes, i set the settings with the override value.
Again when pageagent is disabled, i reset the settings with the cached settings.

So the behaviour before and after patch will be same. I tested these usecases
and it works as expected.

This patch was made  with a intention to remove the page dependency from
settings.cpp

[pfeldman, 2014-01-27 20:17:58]

To make sure I understand things properly.

Steps:
1. enable font scale override in inspector, set it to "A".
2. adjust device scale override via settings, set it to "B".

Before patch: text autosizer uses "A"
After patch: text autosizer uses "B"

[eseidel, 2014-01-27 20:33:57]

looks fine to me.  Please answer pfeldman's concerns before landing.

https://codereview.chromium.org/146683003/diff/290001/Source/core/frame/Setti...
File Source/core/frame/Settings.cpp (right):

https://codereview.chromium.org/146683003/diff/290001/Source/core/frame/Setti...
Source/core/frame/Settings.cpp:70: SETTINGS_INITIALIZER_LIST
Can we just reorder the members so that we can leave this after this #if?

[gnana, 2014-01-30 16:03:01]

Thanks pfeldman, you were right. Thanks for pointing this one. I modified the
source to fix this in inspectorpageagent.cpp. Please have a look.

Thanks eseidel for review.

https://codereview.chromium.org/146683003/diff/290001/Source/core/frame/Setti...
File Source/core/frame/Settings.cpp (right):

https://codereview.chromium.org/146683003/diff/290001/Source/core/frame/Setti...
Source/core/frame/Settings.cpp:70: SETTINGS_INITIALIZER_LIST
On 2014/01/27 20:33:58, eseidel wrote:
> Can we just reorder the members so that we can leave this after this #if?

Done.

[pfeldman, 2014-01-30 16:34:08]

https://codereview.chromium.org/146683003/diff/450001/Source/core/inspector/I...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/146683003/diff/450001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.cpp:1058:
m_frontend->scriptsEnabled(settings->scriptEnabled());
We don't want to call scriptsEnabled with the same value upon every setting
change.

https://codereview.chromium.org/146683003/diff/450001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.cpp:1061:
m_settingsCacheTextAutosizingEnabled = settings->textAutosizingEnabled();
You want to store these unconditionally so that you could restore proper ones.

https://codereview.chromium.org/146683003/diff/450001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.cpp:1066:
settings->setTextAutosizingEnabled(overrideTextAutosizing());
So upon setTextAutosizingEnabled("A"), you will actually bring it back to "B"?
This looks extremely fragile to me.

It seems like instead, you want all call sites to respect the override. Either
we need another setting "textAutosizingOverride" that call sites respect or we
need to use InspectorInstrumentation::overrideTextAutosizing that was used in
settings, but place it into the actual call sites instead. We tend to do the
latter.

[gnana, 2014-01-31 14:21:09]

Please have a look

https://codereview.chromium.org/146683003/diff/450001/Source/core/inspector/I...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/146683003/diff/450001/Source/core/inspector/I...
Source/core/inspector/InspectorPageAgent.cpp:1066:
settings->setTextAutosizingEnabled(overrideTextAutosizing());
On 2014/01/30 16:34:08, pfeldman wrote:
> So upon setTextAutosizingEnabled("A"), you will actually bring it back to "B"?
> This looks extremely fragile to me.
> 
> It seems like instead, you want all call sites to respect the override. Either
> we need another setting "textAutosizingOverride" that call sites respect or we
> need to use InspectorInstrumentation::overrideTextAutosizing that was used in
> settings, but place it into the actual call sites instead. We tend to do the
> latter.

After Placing InspectorInstrumentation::overrideTextAutosizing &
overrideFontScaleFactor in actual call sites, we dont need to cache the settings
anymore.

[pfeldman, 2014-01-31 14:59:16]

lgtm with a nit.

[gnana, 2014-02-03 06:34:55]

The CQ bit was checked by gnanasekar.s@samsung.com

[gnana, 2014-02-03 06:35:06]

The CQ bit was unchecked by gnanasekar.s@samsung.com

[commit-bot: I haz the power, 2014-02-03 06:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/146683003/560020

[commit-bot: I haz the power, 2014-02-03 06:35:08]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 06:35:10]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 06:35:16]

CQ bit was unchecked on CL. Ignoring.

[gnana, 2014-02-03 06:35:24]

The CQ bit was checked by gnanasekar.s@samsung.com

[commit-bot: I haz the power, 2014-02-03 06:35:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/146683003/560020

[commit-bot: I haz the power, 2014-02-03 08:46:15]

Change committed as 166295

[commit-bot: I haz the power, 2014-02-03 08:46:15]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 08:46:15]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 08:46:17]

CQ bit was unchecked on CL. Ignoring.