Bug 119903 – Consider making WebCore::Settings ref-counted.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andreas Kling]

If we were to make WebCore::Settings a ref-counted object, it could allow detached Frames (and perhaps also Documents) to access Settings despite not currently having a way to grab at the Page.

[Andreas Kling]

Created attachment 208935 [details]
Snack for EWS

This is the basic idea. By making Settings ref-counted, Frame can cache a pointer to the Page::settings() instead of always going through the Page.
This means that Settings continue to apply to detached Frames, and we can avoid a ton of null checking.

[Andreas Kling]

Created attachment 208941 [details]
Snack for EWS

[WebKit Commit Bot]

Attachment 208941 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/bindings/ScriptControllerBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowCustom.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/css/CSSFontSelector.cpp', u'Source/WebCore/css/MediaQueryEvaluator.cpp', u'Source/WebCore/dom/DOMImplementation.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/EditorCommand.cpp', u'Source/WebCore/editing/FrameSelection.cpp', u'Source/WebCore/editing/SpellChecker.cpp', u'Source/WebCore/editing/TextCheckingHelper.cpp', u'Source/WebCore/html/HTMLEmbedElement.cpp', u'Source/WebCore/html/HTMLSelectElement.cpp', u'Source/WebCore/html/ImageDocument.cpp', u'Source/WebCore/html/PluginDocument.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/parser/XSSAuditor.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/loader/DocumentLoader.cpp', u'Source/WebCore/loader/DocumentWriter.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/HistoryController.cpp', u'Source/WebCore/loader/MixedContentChecker.cpp', u'Source/WebCore/loader/SubframeLoader.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheHost.cpp', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/loader/cache/CachedResourceLoader.cpp', u'Source/WebCore/loader/icon/IconController.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/Frame.h', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/Navigator.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/page/SpatialNavigation.cpp', u'Source/WebCore/page/mac/EventHandlerMac.mm', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderFrameSet.cpp', u'Source/WebCore/rendering/RenderIFrame.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebFrameNetworkingContext.mm', u'Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm']" exit_code: 1
Source/WebCore/css/CSSFontSelector.cpp:379:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSFontSelector.cpp:381:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSFontSelector.cpp:383:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSFontSelector.cpp:385:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSFontSelector.cpp:387:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSFontSelector.cpp:389:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSFontSelector.cpp:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 208941 [details]
Snack for EWS

Attachment 208941 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1464854

[Early Warning System Bot]

Comment on attachment 208941 [details]
Snack for EWS

Attachment 208941 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1466693

[EFL EWS Bot]

Comment on attachment 208941 [details]
Snack for EWS

Attachment 208941 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1469607

[kov's GTK+ EWS bot]

Comment on attachment 208941 [details]
Snack for EWS

Attachment 208941 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1477446

[Build Bot]

Comment on attachment 208941 [details]
Snack for EWS

Attachment 208941 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1465777

New failing tests:
editing/selection/verify-editing-behavior-for-line-granularity.html
editing/pasteboard/emacs-ctrl-a-k-y.html
fast/forms/textarea-arrow-navigation.html
editing/selection/caret-at-end-of-text-line-followed-by-empty-block-in-vertical-mode.html
editing/pasteboard/emacs-ctrl-k-with-move.html
editing/selection/4947387.html
editing/pasteboard/paste-sanitize-crash-1.html
editing/pasteboard/paste-sanitize-crash-2.html
editing/deleting/delete-to-end-of-paragraph.html
fast/events/selectstart-by-arrow-keys-prevent-default.html
editing/input/editable-container-with-word-wrap-normal.html
http/tests/inspector/inspect-element.html
editing/selection/move-begin-end.html

[Build Bot]

Created attachment 208948 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Andreas Kling]

Created attachment 208958 [details]
Coconut for EWS

[kov's GTK+ EWS bot]

Comment on attachment 208958 [details]
Coconut for EWS

Attachment 208958 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1467698

[Andreas Kling]

Created attachment 208965 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 208965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp', u'Source/WebCore/bindings/ScriptControllerBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSDOMWindowCustom.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/css/CSSFontSelector.cpp', u'Source/WebCore/css/MediaQueryEvaluator.cpp', u'Source/WebCore/dom/DOMImplementation.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/EditorCommand.cpp', u'Source/WebCore/editing/FrameSelection.cpp', u'Source/WebCore/editing/SpellChecker.cpp', u'Source/WebCore/editing/TextCheckingHelper.cpp', u'Source/WebCore/html/HTMLEmbedElement.cpp', u'Source/WebCore/html/HTMLSelectElement.cpp', u'Source/WebCore/html/ImageDocument.cpp', u'Source/WebCore/html/PluginDocument.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/parser/XSSAuditor.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/loader/DocumentLoader.cpp', u'Source/WebCore/loader/DocumentWriter.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/HistoryController.cpp', u'Source/WebCore/loader/MixedContentChecker.cpp', u'Source/WebCore/loader/SubframeLoader.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheHost.cpp', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/loader/cache/CachedResourceLoader.cpp', u'Source/WebCore/loader/icon/IconController.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/Frame.h', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/Navigator.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/page/SpatialNavigation.cpp', u'Source/WebCore/page/mac/EventHandlerMac.mm', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderFrameSet.cpp', u'Source/WebCore/rendering/RenderIFrame.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebFrameNetworkingContext.mm', u'Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'Source/WebKit/win/WebCoreSupport/WebFrameNetworkingContext.cpp', u'Source/WebKit/wince/WebCoreSupport/EditorClientWinCE.cpp', u'Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp']" exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 208965 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208965&action=review

r=me

> Source/WebCore/html/HTMLEmbedElement.cpp:196
> +    // Workaround for <rdar://problem/6642221>.
> +    if (frame->settings().usesDashboardBackwardCompatibilityMode())
> +        return true;

My favorite of all the settings.

> Source/WebCore/page/Frame.h:144
> +        Settings& settings() const { return *m_settings; }

Should this be const Settings&? I don't think any clients want the ability to change the settings inside WebCore.

[EFL EWS Bot]

Comment on attachment 208965 [details]
Proposed patch

Attachment 208965 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1393035

[Andreas Kling]

(In reply to comment #14)
> (From update of attachment 208965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208965&action=review
> 
> r=me

Thanks!

> > Source/WebCore/page/Frame.h:144
> > +        Settings& settings() const { return *m_settings; }
> 
> Should this be const Settings&? I don't think any clients want the ability to change the settings inside WebCore.

Inspector changes some stuff. :|

[Andreas Kling]

This was http://trac.webkit.org/r154219