Switch to using accessibilityEnabled and inlineTextBoxAccessibilityEnabled from settings

Switch to using accessibilityEnabled and inlineTextBoxAccessibilityEnabled from settings

Depends on https://codereview.chromium.org/497263002/

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

[dmazzoni, 2014-08-22 18:16:10]

https://codereview.chromium.org/495763005/diff/60001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/495763005/diff/60001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.cpp:1400: if (document()->settings() &&
document()->settings()->inlineTextBoxAccessibilityEnabled())
This test got reversed

[dmazzoni, 2014-08-23 05:36:01]

https://codereview.chromium.org/495763005/diff/100001/Source/core/accessibili...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/accessibili...
Source/core/accessibility/AXNodeObject.cpp:1400: if (!document()->settings() ||
!document()->settings()->inlineTextBoxAccessibilityEnabled())
I think it might be cleaner to implement convenience functions for this within
accessibility:

* It eliminates the need to include core/frame/Settings.h from more files, which
will hopefully save some recompiles
* It makes negating the test easier and more readable

Either move it back to AXObjectCache, or maybe implement it as a protected
method in AXObject.

[abarth-chromium, 2014-08-23 05:59:11]

https://codereview.chromium.org/495763005/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2275: if (!settings() ||
!settings()->accessibilityEnabled())
Please don't call settings() twice.  It's not free.

https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1789: if (UNLIKELY(settings &&
settings->accessibilityEnabled()) && m_lineBoxes.firstLineBox() == rootBox) {
This is sad.  We're adding a ton more branches to this codepath.

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:581: }
These aren't needed.

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:199: }
This makes no sense.  A remote frame is in a different process.  It can't have
any an accessibility tree.

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebSettingsI...
File Source/web/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebSettingsI...
Source/web/WebSettingsImpl.cpp:175: }
These are correct.

https://codereview.chromium.org/495763005/diff/100001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/495763005/diff/100001/public/web/WebFrame.h#n...
public/web/WebFrame.h:157: virtual void
setInlineTextBoxAccessibilityEnabled(bool) = 0;
These aren't needed.  You should put these on WebSettings

[aboxhall, 2014-08-27 15:26:33]

https://codereview.chromium.org/495763005/diff/100001/Source/core/accessibili...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/accessibili...
Source/core/accessibility/AXNodeObject.cpp:1400: if (!document()->settings() ||
!document()->settings()->inlineTextBoxAccessibilityEnabled())
On 2014/08/23 05:36:01, dmazzoni wrote:
> I think it might be cleaner to implement convenience functions for this
within
> accessibility:
> 
> * It eliminates the need to include core/frame/Settings.h from more files,
which
> will hopefully save some recompiles
> * It makes negating the test easier and more readable
> 
> Either move it back to AXObjectCache, or maybe implement it as a protected
> method in AXObject.
> 
> 

Good idea, done.

https://codereview.chromium.org/495763005/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2275: if (!settings() ||
!settings()->accessibilityEnabled())
On 2014/08/23 05:59:11, abarth wrote:
> Please don't call settings() twice.  It's not free.

Done.

https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1789: if (UNLIKELY(settings &&
settings->accessibilityEnabled()) && m_lineBoxes.firstLineBox() == rootBox) {
On 2014/08/23 05:59:11, abarth wrote:
> This is sad.  We're adding a ton more branches to this codepath.

I don't follow.

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:581: }
On 2014/08/23 05:59:11, abarth wrote:
> These aren't needed.

Done.

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:199: }
On 2014/08/23 05:59:11, abarth wrote:
> This makes no sense.  A remote frame is in a different process.  It can't have
> any an accessibility tree.

Done.

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebSettingsI...
File Source/web/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/web/WebSettingsI...
Source/web/WebSettingsImpl.cpp:175: }
On 2014/08/23 05:59:11, abarth wrote:
> These are correct.

Acknowledged.

https://codereview.chromium.org/495763005/diff/100001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/495763005/diff/100001/public/web/WebFrame.h#n...
public/web/WebFrame.h:157: virtual void
setInlineTextBoxAccessibilityEnabled(bool) = 0;
On 2014/08/23 05:59:11, abarth wrote:
> These aren't needed.  You should put these on WebSettings

Done.

[dmazzoni, 2014-08-27 16:17:15]

https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1789: if (UNLIKELY(settings &&
settings->accessibilityEnabled()) && m_lineBoxes.firstLineBox() == rootBox) {
On 2014/08/27 15:26:33, aboxhall wrote:
> On 2014/08/23 05:59:11, abarth wrote:
> > This is sad.  We're adding a ton more branches to this codepath.
> 
> I don't follow.

I think Adam is saying that this function (createAndAppendRootInlineBox) is
called thousands of times during layout, and testing whether accessibility is
enabled every time is expensive - and this change potentially makes it worse.

I think it might be possible to get rid of this test entirely. I filed
http://crbug.com/408159, let me see what the implications are if we get rid of
this code and the accessibility logic that depended on it.

[aboxhall, 2014-08-29 03:36:45]

On 2014/08/27 16:17:15, dmazzoni wrote:
>
https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
> File Source/core/rendering/RenderBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/495763005/diff/100001/Source/core/rendering/R...
> Source/core/rendering/RenderBlockFlow.cpp:1789: if (UNLIKELY(settings &&
> settings->accessibilityEnabled()) && m_lineBoxes.firstLineBox() == rootBox) {
> On 2014/08/27 15:26:33, aboxhall wrote:
> > On 2014/08/23 05:59:11, abarth wrote:
> > > This is sad.  We're adding a ton more branches to this codepath.
> > 
> > I don't follow.
> 
> I think Adam is saying that this function (createAndAppendRootInlineBox) is
> called thousands of times during layout, and testing whether accessibility is
> enabled every time is expensive - and this change potentially makes it worse.
> 
> I think it might be possible to get rid of this test entirely. I filed
> http://crbug.com/408159, let me see what the implications are if we get rid of
> this code and the accessibility logic that depended on it.

abarth@: 
Should this be addressed in this change? Or is this change good to go now?

[abarth-chromium, 2014-08-29 16:16:52]

On 2014/08/29 at 03:36:45, aboxhall wrote:
> Should this be addressed in this change? Or is this change good to go now?

Please address this issue before landing this CL.

[abarth-chromium, 2014-08-29 18:32:07]

LGTM

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
Source/core/accessibility/AXObjectCache.cpp:919: return m_settings;
Why store a pointer to m_settings?  You can just get it from m_document.

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
File Source/core/accessibility/AXObjectCache.h (right):

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
Source/core/accessibility/AXObjectCache.h:234: Settings* getSettings();
s/getSettings/settings/

We usually skip the "get" prefix because it has no meaning.

https://codereview.chromium.org/495763005/diff/200001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/495763005/diff/200001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2595: if (frame() && renderView() && settings &&
settings->accessibilityEnabled()) {
There's no need to null check settings here.  If you have a renderView() you'll
have a settings().

[aboxhall, 2014-08-29 18:38:39]

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
File Source/core/accessibility/AXObjectCache.h (right):

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
Source/core/accessibility/AXObjectCache.h:234: Settings* getSettings();
On 2014/08/29 18:32:07, abarth wrote:
> s/getSettings/settings/
> 
> We usually skip the "get" prefix because it has no meaning.

I deliberately used 'get' as it's not a trivial getter. Happy to remove it if it
LGTY without.

[aboxhall, 2014-08-29 20:35:16]

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/495763005/diff/200001/Source/core/accessibili...
Source/core/accessibility/AXObjectCache.cpp:919: return m_settings;
On 2014/08/29 18:32:07, abarth wrote:
> Why store a pointer to m_settings?  You can just get it from m_document.

Done.

https://codereview.chromium.org/495763005/diff/200001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/495763005/diff/200001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2595: if (frame() && renderView() && settings &&
settings->accessibilityEnabled()) {
On 2014/08/29 18:32:07, abarth wrote:
> There's no need to null check settings here.  If you have a renderView()
you'll
> have a settings().

Done.

[aboxhall, 2014-08-29 20:52:27]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-08-29 23:04:27]

Committed patchset #15 (id:280001) as 181120