Created attachment 113529 [details] Patch

Comment on attachment 113529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113529&action=review > Source/WebCore/rendering/RenderBoxRegionInfo.cpp:59 > + // FIXME: assume for now that a box with region styling always has box decorations. > + (const_cast<RenderBox*>(box))->setHasBoxDecorations(true); You should be able to set this accurately. Do the same checks that were done for the real style. > Source/WebCore/rendering/RenderBoxRegionInfo.h:56 > + RenderStyle* regionStyle() const { return m_regionStyle.get(); } > + void setRegionStyle(PassRefPtr<RenderStyle>); > + void computeStyleInRegion(const RenderBox*, RenderRegion*); I think this should just be in RenderFlowThread in a separate map rather than part of RenderBoxRegionInfo.h. It has to work for all RenderObjects, so that's my mistake suggesting to put it here. > Source/WebCore/rendering/RenderFlowThread.cpp:751 > + view()->setCurrentRenderRegion(startRegion); Let's just remove this for now. I don't think it's the right place to be setting the current render region, and it's not relevant to getting background color working, so you can revisit this in a future patch. > Source/WebCore/rendering/RenderFlowThread.cpp:769 > + view()->setCurrentRenderRegion(startRegion); Same. Remove for now. > Source/WebCore/rendering/RenderLayer.cpp:2548 > + renderer()->view()->setCurrentRenderRegion(region); Who clears this when you're finished? It seems like this should be more like a stack... where you push the region and then pop back to the old one when finished. A little helper class might be good for this. I suspect this might be a problem with the current flow thread stuff in RenderView as well... It seems like you could eliminate all of the RenderRegion* arguments I added to RenderLayer by just using this current region instead. Then you could set this in RenderRegion's painting and hit testing instead. > Source/WebCore/rendering/RenderObject.cpp:1696 > + if (view() && view()->currentRenderRegion() && view()->currentRenderRegion()->hasCustomRegionStyle() && isBox() && !isAnonymous()) { Check inRenderFlowThread(). That is a really fast check you can do right up front. I would also bias towards the common case first so: if (!inRenderFlowThread() || ....) return m_style.get(); // Now run your code.

(In reply to comment #2) > > It seems like you could eliminate all of the RenderRegion* arguments I added to RenderLayer by just using this current region instead. Then you could set this in RenderRegion's painting and hit testing instead. > Actually, changed my mind here. I would just fix RenderLayer to set back to the old value when done painting or hit testing. That way you don't have to worry about implementing a stack or helper class etc. in RenderView. There is only nesting like this with RenderLayer and painting/hit testing (you don't have nesting when doing layout), so I think we should just fix this in RenderLayer.

Created attachment 114062 [details] Patch 2

Comment on attachment 114062 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=114062&action=review > Source/WebCore/rendering/RenderRegion.cpp:252 > +bool RenderRegion::renderObjectInRegion(const RenderObject* renderObject) const > +{ > + ASSERT(renderObject); > + return m_renderBoxRegionInfo.contains(renderObject->enclosingBox()); > +} I don't understand this bit (or why it's checked earlier). You can have a unique style without having a RenderBoxRegionInfo at all. Ultimately I plan to optimize uniform width regions to try to get rid of RenderBoxRegionInfos, so I don't think you should assume that one has to exist. > Source/WebCore/rendering/RenderRegion.cpp:276 > + } else { > + RenderObject* parent = object->parent(); > + RenderObjectRegionStyleMap::iterator it = m_renderObjectRegionStyle.find(parent); > + ASSERT(it != m_renderObjectRegionStyle.end()); > + RefPtr<RenderStyle> renderParentStyle = it->second; > + m_renderObjectRegionStyle.set(object, renderParentStyle); > + } This is right for RenderText but it's not right for anonymous elements. Anonymous elements have to take the parent style you found and create a new style that inherits from it. This is going to be one of the harder things to get right, since these custom anonymous styles tend to be re-created/handled over in the style change methods of the various renderers. For now I would probably just support elements and text and return the normal style for anonymous elements. We should try to handle that in a separate patch (and file a followup bug about it). Add an assert that you never enter this function without the node being an element or the object being text and then before even calling this make sure you bailed and just returned the original style for anonymous elements. We'll figure them out in a followup bug.

Created attachment 114470 [details] Patch 3

Attachment 114470 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Last 3072 characters of output: st_expectations.txt:3864: Path does not exist. storage/domstorage/events/basic-body-attribute.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3865: Path does not exist. fast/dom/Element/id-in-frame.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3866: Path does not exist. fast/frames/content-opacity-2.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3867: Path does not exist. fast/parser/close-while-stopping.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3868: Path does not exist. fast/frames/iframe-double-scale-contents.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3869: Path does not exist. fast/frames/set-parent-src-synchronously.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3872: Path does not exist. svg/custom/getBBox-path.svg [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3876: Path does not exist. svg/as-border-image/svg-as-border-image.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3877: Path does not exist. svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3878: Path does not exist. svg/as-background-image/svg-as-background-1.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3879: Path does not exist. svg/as-background-image/svg-as-background-3.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3881: Path does not exist. svg/zoom/page/zoom-svg-as-relative-image.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3882: Path does not exist. svg/zoom/page/relative-sized-document-scrollbars.svg [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3884: Path does not exist. storage/indexeddb/key-type-array.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3886: Path does not exist. fast/js/array-functions-non-arrays.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3887: Path does not exist. fast/js/eval-cross-window.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3888: Path does not exist. fast/js/regexp-caching.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3889: Path does not exist. fast/js/toString-overrides.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3890: Path does not exist. fast/js/toString-recursion.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3891: Path does not exist. http/tests/security/xss-eval.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3893: Path does not exist. fast/dom/rtl-scroll-to-leftmost-and-resize.html [test/expectations] [2] Total errors found: 2138 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #5) > (From update of attachment 114062 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114062&action=review > > > Source/WebCore/rendering/RenderRegion.cpp:252 > > +bool RenderRegion::renderObjectInRegion(const RenderObject* renderObject) const > > +{ > > + ASSERT(renderObject); > > + return m_renderBoxRegionInfo.contains(renderObject->enclosingBox()); > > +} > > I don't understand this bit (or why it's checked earlier). You can have a unique style without having a RenderBoxRegionInfo at all. Ultimately I plan to optimize uniform width regions to try to get rid of RenderBoxRegionInfos, so I don't think you should assume that one has to exist. > You are right, there is no need for this check in RenderObject::style() > > Source/WebCore/rendering/RenderRegion.cpp:276 > > + } else { > > + RenderObject* parent = object->parent(); > > + RenderObjectRegionStyleMap::iterator it = m_renderObjectRegionStyle.find(parent); > > + ASSERT(it != m_renderObjectRegionStyle.end()); > > + RefPtr<RenderStyle> renderParentStyle = it->second; > > + m_renderObjectRegionStyle.set(object, renderParentStyle); > > + } > > This is right for RenderText but it's not right for anonymous elements. Anonymous elements have to take the parent style you found and create a new style that inherits from it. This is going to be one of the harder things to get right, since these custom anonymous styles tend to be re-created/handled over in the style change methods of the various renderers. > > For now I would probably just support elements and text and return the normal style for anonymous elements. We should try to handle that in a separate patch (and file a followup bug about it). Add an assert that you never enter this function without the node being an element or the object being text and then before even calling this make sure you bailed and just returned the original style for anonymous elements. We'll figure them out in a followup bug. I have created another bug, 71888, for dealing with anonymous elements styling.

Comment on attachment 114470 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=114470&action=review Getting close! > Source/WebCore/rendering/RenderRegion.cpp:155 > + bool clearRegionStyle = true; > if (m_flowThread && isValid()) { > if (regionRect().width() != contentWidth() || regionRect().height() != contentHeight()) > m_flowThread->invalidateRegions(); > + else > + clearRegionStyle = false; > } > - > + > + if (clearRegionStyle) > + clearRegionStyleInRegion(); This isn't really right. You should not clear styles just because layout changed. I would expect styles to be cleared out when the start/end regions change for a given object, or when that object goes away. I think at a bare minimum you should work with RenderObject::destroy to clear out any custom region styles. This could possibly be done by clearing the start/end region and having that function just delete the styles. I'd also file a followup bug about keeping the styles up to date when something dynamically changes with style.

Right now we don't set the start/end region for anything but RenderBlocks. You *might* want to just limit the custom styling to RenderBlocks for now to avoid leaks, and we can expand it as we add support for other objects.

Created attachment 115366 [details] Patch 4

(In reply to comment #9) > (From update of attachment 114470 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114470&action=review > > Getting close! > > > Source/WebCore/rendering/RenderRegion.cpp:155 > > + bool clearRegionStyle = true; > > if (m_flowThread && isValid()) { > > if (regionRect().width() != contentWidth() || regionRect().height() != contentHeight()) > > m_flowThread->invalidateRegions(); > > + else > > + clearRegionStyle = false; > > } > > - > > + > > + if (clearRegionStyle) > > + clearRegionStyleInRegion(); > > This isn't really right. You should not clear styles just because layout changed. I would expect styles to be cleared out when the start/end regions change for a given object, or when that object goes away. I think at a bare minimum you should work with RenderObject::destroy to clear out any custom region styles. This could possibly be done by clearing the start/end region and having that function just delete the styles. > I have limited the styling to blocks for now. > I'd also file a followup bug about keeping the styles up to date when something dynamically changes with style. Done with https://bugs.webkit.org/show_bug.cgi?id=72505.

Created attachment 115785 [details] Patch 5 Add more tests and restrict styling to blocks.

Comment on attachment 115785 [details] Patch 5 r=me

Comment on attachment 115785 [details] Patch 5 Rejecting attachment 115785 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: h Hunk #2 succeeded at 658 (offset 2 lines). patching file Source/WebCore/rendering/RenderObjectChildList.cpp patching file Source/WebCore/rendering/RenderRegion.cpp patching file Source/WebCore/rendering/RenderRegion.h patching file Source/WebCore/rendering/RenderView.cpp patching file Source/WebCore/rendering/RenderView.h Hunk #3 succeeded at 277 (offset 2 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10736886

Created attachment 118200 [details] Patch for landing

Comment on attachment 118200 [details] Patch for landing Clearing flags on attachment: 118200 Committed r102234: <http://trac.webkit.org/changeset/102234>

All reviewed patches have been landed. Closing bug.

Reopened because of regression introduced in https://bugs.webkit.org/show_bug.cgi?id=74141. Bug https://bugs.webkit.org/show_bug.cgi?id=74315 reverts the change in RenderObject::style() to its original state.

Created attachment 123110 [details] Patch

Comment on attachment 123110 [details] Patch r=me

Comment on attachment 123110 [details] Patch Clearing flags on attachment: 123110 Committed r105426: <http://trac.webkit.org/changeset/105426>

All reviewed patches have been landed. Closing bug.

I reverted this patch because it caused a PTL regression: http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=150&rev=-1

Created attachment 124329 [details] Patch

Attachment 124329 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/ChangeLog:11: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Source/WebCore/ChangeLog:12: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Source/WebCore/ChangeLog:13: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Source/WebCore/ChangeLog:14: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Source/WebCore/ChangeLog:15: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 5 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124329 [details] Patch Overall I like this approach. It seems kind of silly to waste 4 bytes in the RenderBoxRegionInfo for the original style though. It seems like you could just use a HashMap from RenderBox to RefPtr<RenderStyle> for that instead. Since most of the time you're not going to have any region styling, it seems better to avoid bloating RenderBoxRegionInfo in the common case.

Also, fix the ChangeLog to not use * for bullets, since you're confusing the style checker. :)

Comment on attachment 124329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124329&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2266 > - bool styleDeclarationInsideRegionRule = m_regionForStyling ? isInsideRegionRule(styleDeclaration) : false; > + bool styleDeclarationInsideRegionRule = isInsideRegionRule(styleDeclaration); > + if (styleDeclarationInsideRegionRule && !hasRegionStyling()) { Invoking isInsideRegionRule() on every declaration, even when there are no region styles is not good. If it is unavoidable the the bit needs be cached, either to CSSMutableStyleDeclaration or to RuleData (you actually had this in an earlier patch).

Created attachment 124530 [details] Patch

Comment on attachment 124530 [details] Patch r=me

Comment on attachment 124530 [details] Patch Clearing flags on attachment: 124530 Committed r106281: <http://trac.webkit.org/changeset/106281>

All reviewed patches have been landed. Closing bug.

This appears to be causing a number of assertions. Can one of you take a look at this quickly? Otherwise I'm going to roll it out until the issue is addressed. Here are some failing tests after this change: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fregions%2Fpositioned-objects-block-static-spanning-regions.html%2Cfast%2Fregions%2Fpositioned-objects-block-static-in-regions.html%2Cfast%2Fregions%2Fcontent-flowed-into-regions.html%2Cfast%2Fregions%2Fpositioned-objects-block-static-in-rtl-regions.html%2Cfast%2Fregions%2Fcontent-flowed-into-regions-dynamically-removed.html%2Cfast%2Fregions%2Fpositioned-objects-block-static-spanning-regions-rtl.html%2Cfast%2Fregions%2Fcontent-flowed-into-regions-dynamically-inserted.html%2Cfast%2Fregions%2Fcontent-flowed-into-regions-dynamically-added.html And here's an example backtrace: ASSERTION FAILED: !m_insideRegionPaint && "RenderBoxRegionInfo should not be modified inside region paint." /b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderRegion.cpp(251) : WebCore::RenderBoxRegionInfo *WebCore::RenderRegion::setRenderBoxRegionInfo(const WebCore::RenderBox *, LayoutUnit, LayoutUnit, bool) 1 0x1e1078f9 WebCore::RenderRegion::setRenderBoxRegionInfo(WebCore::RenderBox const*, int, int, bool) 2 0x1dfd82d6 WebCore::RenderBox::renderBoxRegionInfo(WebCore::RenderRegion*, int, WebCore::RenderBox::RenderBoxRegionInfoFlags) const 3 0x1dfded8c WebCore::RenderBox::containingBlockLogicalWidthForContentInRegion(WebCore::RenderRegion*, int) const 4 0x1dfe14e5 WebCore::RenderBox::computeLogicalWidthInRegion(WebCore::RenderRegion*, int) 5 0x1dfd7ed3 WebCore::RenderBox::renderBoxRegionInfo(WebCore::RenderRegion*, int, WebCore::RenderBox::RenderBoxRegionInfoFlags) const 6 0x1dfd7a08 WebCore::RenderBox::borderBoxRectInRegion(WebCore::RenderRegion*, int, WebCore::RenderBox::RenderBoxRegionInfoFlags) const 7 0x1dfdc92a WebCore::RenderBox::paintBoxDecorations(WebCore::PaintInfo&, WebCore::IntPoint const&) 8 0x1df579f4 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 9 0x1df559e1 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 10 0x1df57540 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::IntPoint const&) 11 0x1df57164 WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::IntPoint const&) 12 0x1df57b92 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 13 0x1df559e1 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 14 0x1df57540 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::IntPoint const&) 15 0x1df57164 WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::IntPoint const&) 16 0x1df57b92 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 17 0x1df559e1 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 18 0x1e08b858 WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) 19 0x1e08afda WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) 20 0x1e08a754 WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) 21 0x1e08a036 WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, unsigned int) 22 0x1e0483bd WebCore::RenderFlowThread::paintIntoRegion(WebCore::PaintInfo&, WebCore::RenderRegion*, WebCore::IntPoint const&) 23 0x1e106fa4 WebCore::RenderRegion::paintReplaced(WebCore::PaintInfo&, WebCore::IntPoint const&) 24 0x1e111b6a WebCore::RenderReplaced::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 25 0x1df57004 WebCore::RenderBlock::paintFloats(WebCore::PaintInfo&, WebCore::IntPoint const&, bool) 26 0x1df57cc3 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 27 0x1df559e1 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 28 0x1e08b89e WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) 29 0x1e08afda WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) 30 0x1e08a754 WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) 31 0x1e08ca8d WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int)

FYI: I removed these assertions with Hyatt's blessing in http://trac.webkit.org/changeset/106291