Add text autosizing override in the inspector.

Add text autosizing override in the inspector.

This patch adds a checkbox to the inspector for enabling text
autosizing. Screenshot: http://pr.gg/as2.png

This gives developers the ability to test mobile text layout/rendering
on a desktop (and vice-versa).

This patch has two components (Inspector changes and Blink changes):
1) A checkbox for text autosizing has been added to the inspector
under the Device Metrics section. This checkbox has been wired
up to the metrics.textAutosizing property.

Default text autosizing settings have been added for the various
pre-provided user agent strings.

I explored implementing the checkbox outside the device metrics
section (https://codereview.chromium.org/25735003) but the feature
doesn't make a lot of sense unless a device scale value is set.
A downside to the approach taken in the current patch is it is not
possible to disable text autosizing on a mobile device without
overriding device metrics; I think this is a reasonable tradeoff.

I also explored implementing this checkbox as a tri-state
dropdown (https://codereview.chromium.org/27119002) so users
could test overriding device metrics but use the default setting for
text autosizing. I think the resulting UI is overly complex for the
average developer.

2) This patch also wires up Blink's Settings to the text
autosizing feature. A ternary enum has been added to track
whether autosizing is overridden in Settings.

Because the device metrics overrides do not actually scale the
window (to maintain crisp rendering), I used the text autosizing
window size override.

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

[pdr., 2013-10-14 04:51:26]

Ready for review!

[pdr., 2013-10-15 23:55:27]

@pfeldman, @dgozman, ping?

[pfeldman, 2013-10-16 05:34:14]

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.cpp
File Source/core/page/Settings.cpp (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.cpp...
Source/core/page/Settings.cpp:252: void
Settings::setTextAutosizingEnabledOverride(TextAutosizingOverride
textAutosizingOverride)
Instead of introducing a new setting clients might call, I'd suggest turning
::textAutosizingEnabled above into:

bool enableAutosize;
InspectorInstrumentation::overrideTextAutosizing(m_textAutosizing,
&enableAutosize);
return enableAutosize;

See interface InspectorOverrides in InspectorInstrumentation.idl.

[skobes, 2013-10-16 19:38:09]

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorPageAgent.cpp:411:
m_state->setBoolean(PageAgentState::pageAgentFitWindow, false);
Do you also need
m_state->setBoolean(PageAgentState::pageAgentTextAutosizingOverride, false)
here?

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorPageAgent.cpp:1091: // we need to explicity
override the text autosizing window size.
I'm surprised this is needed since screen.width / screen.height respect the
device metrics override.  Does this mean the autosizer is getting the window
size in the wrong way?

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorPageAgent.h (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorPageAgent.h:184: void updateViewMetrics(int, int,
double, bool, bool);
The C++ style guide forbids unnamed params (and I see the Blink style guide
makes an exception, but only when the name is "meaningless" / redundant).  Do
you mind adding the param names here?

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.h
File Source/core/page/Settings.h (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.h#n...
Source/core/page/Settings.h:183: unsigned m_textAutosizingOverride : 2; //
TextAutosizingOverride
Why is the type unsigned int instead of TextAutosizingOverride?

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesSupport.js (right):

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesSupport.js:82: return new
WebInspector.OverridesSupport.DeviceMetrics(parseInt(splitMetrics[0], 10),
parseInt(splitMetrics[1], 10), parseFloat(splitMetrics[2]), splitMetrics[3] ==
1);
This would be a lot more readable with each ctor arg on its own line.

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesView.js (right):

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesView.js:139: // When present, the third
element lists device metrics separated by 'x':
'x' sort of makes sense as a separator for width, height, and density, but less
so for autosizing.  Maybe use ',' instead?

[pfeldman, 2013-10-17 16:31:15]

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesView.js (right):

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesView.js:139: // When present, the third
element lists device metrics separated by 'x':
It should all be converted into JSON. Parsing strings is no good. I'll follow up
on that so that it did not block you.

[pdr., 2013-10-18 01:28:44]

Updated!

@pfeldman, can you take another look?

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorPageAgent.cpp:411:
m_state->setBoolean(PageAgentState::pageAgentFitWindow, false);
On 2013/10/16 19:38:09, skobes wrote:
> Do you also need
> m_state->setBoolean(PageAgentState::pageAgentTextAutosizingOverride, false)
> here?

Nice catch. Fixed.

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorPageAgent.cpp:1091: // we need to explicity
override the text autosizing window size.
On 2013/10/16 19:38:09, skobes wrote:
> I'm surprised this is needed since screen.width / screen.height respect the
> device metrics override.  Does this mean the autosizer is getting the window
> size in the wrong way?

You are so right. http://crbug.com/308800 filed. We can leave this in as a
workaround in the interim.

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorPageAgent.h (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorPageAgent.h:184: void updateViewMetrics(int, int,
double, bool, bool);
On 2013/10/16 19:38:09, skobes wrote:
> The C++ style guide forbids unnamed params (and I see the Blink style guide
> makes an exception, but only when the name is "meaningless" / redundant).  Do
> you mind adding the param names here?

Sure thing, done.

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.cpp
File Source/core/page/Settings.cpp (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.cpp...
Source/core/page/Settings.cpp:252: void
Settings::setTextAutosizingEnabledOverride(TextAutosizingOverride
textAutosizingOverride)
On 2013/10/16 05:34:14, pfeldman wrote:
> Instead of introducing a new setting clients might call, I'd suggest turning
> ::textAutosizingEnabled above into:
> 
> bool enableAutosize;
> InspectorInstrumentation::overrideTextAutosizing(m_textAutosizing,
> &enableAutosize);
> return enableAutosize;
> 
> See interface InspectorOverrides in InspectorInstrumentation.idl.

Great catch, done.

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.h
File Source/core/page/Settings.h (right):

https://codereview.chromium.org/26929003/diff/1/Source/core/page/Settings.h#n...
Source/core/page/Settings.h:183: unsigned m_textAutosizingOverride : 2; //
TextAutosizingOverride
On 2013/10/16 19:38:09, skobes wrote:
> Why is the type unsigned int instead of TextAutosizingOverride?

This is a c++ issue where enum types can't be in bitfields but the casts work
out as an unsigned.

I've removed this line as pfeldman pointed out we can use the inspector
instrumentation instead of modifying settings.

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesSupport.js (right):

https://codereview.chromium.org/26929003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesSupport.js:82: return new
WebInspector.OverridesSupport.DeviceMetrics(parseInt(splitMetrics[0], 10),
parseInt(splitMetrics[1], 10), parseFloat(splitMetrics[2]), splitMetrics[3] ==
1);
On 2013/10/16 19:38:09, skobes wrote:
> This would be a lot more readable with each ctor arg on its own line.

Agreed, I refactored this a bit to clean it up.

[pfeldman, 2013-10-18 12:01:26]

lgtm

https://codereview.chromium.org/26929003/diff/27001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/26929003/diff/27001/Source/devtools/protocol....
Source/devtools/protocol.json:344: { "name": "textAutosizing", "type":
"boolean", "description": "Whether to override the text autosizing feature." }
We'll have to do that "optional": true, but that can be done later.

[skobes, 2013-10-18 17:17:06]

https://codereview.chromium.org/26929003/diff/27001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/26929003/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorPageAgent.cpp:1211: return
m_state->getBoolean(PageAgentState::pageAgentTextAutosizingOverride);
Does this handle the case where the inspector is enabled but the device metrics
override is not checked?  It seems like you need a tri-state field
(true/false/no-override) but I may be confused.

[pfeldman, 2013-10-18 17:26:51]

https://codereview.chromium.org/26929003/diff/27001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/26929003/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorPageAgent.cpp:1211: return
m_state->getBoolean(PageAgentState::pageAgentTextAutosizingOverride);
On 2013/10/18 17:17:07, skobes wrote:
> Does this handle the case where the inspector is enabled but the device
metrics
> override is not checked?  It seems like you need a tri-state field
> (true/false/no-override) but I may be confused.

Yeah, I guess you are right. It should be
if (m_enabled &&
m_state->getBoolean(PageAgentState::pageAgentTextAutosizingOverride))
    return true;

[pdr., 2013-10-18 18:46:09]

https://codereview.chromium.org/26929003/diff/27001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/26929003/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorPageAgent.cpp:1211: return
m_state->getBoolean(PageAgentState::pageAgentTextAutosizingOverride);
On 2013/10/18 17:26:52, pfeldman wrote:
> On 2013/10/18 17:17:07, skobes wrote:
> > Does this handle the case where the inspector is enabled but the device
> metrics
> > override is not checked?  It seems like you need a tri-state field
> > (true/false/no-override) but I may be confused.
> 
> Yeah, I guess you are right. It should be
> if (m_enabled &&
> m_state->getBoolean(PageAgentState::pageAgentTextAutosizingOverride))
>     return true;

Done.

https://codereview.chromium.org/26929003/diff/27001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/26929003/diff/27001/Source/devtools/protocol....
Source/devtools/protocol.json:344: { "name": "textAutosizing", "type":
"boolean", "description": "Whether to override the text autosizing feature." }
On 2013/10/18 12:01:26, pfeldman wrote:
> We'll have to do that "optional": true, but that can be done later.

Filed crbug.com/309094 and assigned it to myself.

[commit-bot: I haz the power, 2013-10-18 18:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/26929003/147001

[skobes, 2013-10-18 18:47:40]

lgtm

[commit-bot: I haz the power, 2013-10-18 21:07:37]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[pdr., 2013-10-20 01:35:55]

On 2013/10/18 21:07:37, I haz the power (commit-bot) wrote:
> Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

This failure was real. I didn't understand what [DefaultReturn] meant in
InspectorInstrumentation.idl and incorrectly removed it.

[commit-bot: I haz the power, 2013-10-20 01:36:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/26929003/337001

[commit-bot: I haz the power, 2013-10-20 02:53:31]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-20 03:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/26929003/547001

[commit-bot: I haz the power, 2013-10-20 04:26:03]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-20 04:33:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/26929003/547001

[commit-bot: I haz the power, 2013-10-20 05:10:06]

Change committed as 160027