Bug 64220 – Scrollbar color heuristic needs to be hooked up in WebKit1

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tim Horton]

We don't currently set the color of the scrollbar based on the content for WebKit1 views.

<rdar://problem/9589140>

Patch forthcoming.

[Tim Horton]

Created attachment 100192 [details]
Patch

Not commit-worthy yet, I don't think that putting this in repaint is the right place, though it's not clear there's a better place.

[Darin Adler]

Comment on attachment 100192 [details]
Patch

Looking back at the implementation of this that is already checked in:

I was surprised to see that in ScrollbarThemeMac::paint, the knob style is recomputed each time we draw a scrollbar. That is not a good design. Even if setting the style on the painter each time was efficient enough, it’s not conceptually OK to simply draw the scrollbar in a different style without making sure we repaint existing visible scrollbars when the style changes. Generally speaking such things are set up as part of style computation and layout.

It’s also not good that we added a getDocumentBackgroundColor function to Frame. This is not the right class for a function like that. For one thing, we should not be adding new functions to the Frame class at this time since it is just the center of a set of related classes. It is probably reasonable to put this function in the FrameView class.

The name of the getDocumentBackgroundColor function, starting with get, is correct style inside WebKit2, but not correct WebCore. The get prefix should have been removed.

The getDocumentBackgroundColor function gets at renderers without triggering style recalculation first. This is not correct unless the caller somehow guarantees rendering is up to date. Normally a function like this needs to make sure style is calculated before looking at renderers or must only be called when style is guaranteed to be up to date.

After talking to Maciej we agree that the right design for this is to store the scrollbar overlay style in the ScrollableArea. If the scrollbar overlay style changes, the scrollable area then schedules a scrollbar repaint. In the case of a scrollable area that is implemented with ScrollView, it would call a function from the ScrollView class.

We’d also need code to schedule a scrollbar overlay style recalculation that works either immediately or based on a timer. That would be called by any code that could change the background color. This would need to be done any time the color of the document element or the body element changes, presumably in RenderBox::styleDidChange, or any time the view's base background color changes, which would be in FrameView::setBaseBackgroundColor.

[Tim Horton]

(In reply to comment #2)
Ok, I'm a lot more familiar with the scrolling code after the last two days, so I'll take a crack at this and see what happens. Thanks, Darin!

[Tim Horton]

Created attachment 101212 [details]
totally new patch, utilizing Darin's suggestions

[Tim Horton]

Created attachment 101213 [details]
same as last + radar URL

[Gyuyoung Kim]

Comment on attachment 101213 [details]
same as last + radar URL

Attachment 101213 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9108002

[Early Warning System Bot]

Comment on attachment 101213 [details]
same as last + radar URL

Attachment 101213 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9117373

[Gustavo Noronha (kov)]

Comment on attachment 101213 [details]
same as last + radar URL

Attachment 101213 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9100022

[Tim Horton]

Created attachment 101224 [details]
this time, without breaking all of the other platforms!

[Tim Horton]

Interesting. Works great for WK1, but for WK2, somewhere between setting the correct overlay style (I've added an updateScrollbarOverlayStyle call to registerScrollbar and setNewPainterForScrollbar) and going to paint, the NSScrollerImp is losing its style (getting reset to default). Will continue investigating (if all else fails, we could just set the cached style during every paint(); we still get 99% of the benefit).

[Darin Adler]

Comment on attachment 101224 [details]
this time, without breaking all of the other platforms!

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

> Source/WebCore/page/FrameView.cpp:333
> +    Color bgColor = documentBackgroundColor();

I’d suggest not abbreviated background to “bg”.

> Source/WebCore/page/FrameView.cpp:2390
> +    // start as invalid colors

I’d suggest changing this comment to sentence style:

    // Start with invalid colors.

> Source/WebCore/platform/ScrollView.cpp:790
> +    if (horizontalScrollbar())
> +        ScrollbarTheme::nativeTheme()->updateScrollbarOverlayStyle(horizontalScrollbar());
> +    if (verticalScrollbar())
> +        ScrollbarTheme::nativeTheme()->updateScrollbarOverlayStyle(verticalScrollbar());

Why is this code here instead of in the ScrollableArea base class?

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:239
> +    switch (style) {
> +        case ScrollbarOverlayStyleDark:
> +            return wkScrollerKnobStyleDark;
> +        case ScrollbarOverlayStyleLight:
> +            return wkScrollerKnobStyleLight;
> +        default:
> +            return wkScrollerKnobStyleDefault;
> +    }

The old indenting style is the correct style for WebKit, believe it or not.

[Tim Horton]

Comment on attachment 101224 [details]
this time, without breaking all of the other platforms!

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

>> Source/WebCore/page/FrameView.cpp:333
>> +    Color bgColor = documentBackgroundColor();
> 
> I’d suggest not abbreviated background to “bg”.

Ok.

>> Source/WebCore/page/FrameView.cpp:2390
>> +    // start as invalid colors
> 
> I’d suggest changing this comment to sentence style:
> 
>     // Start with invalid colors.

Ok, sure!

>> Source/WebCore/platform/ScrollView.cpp:790
>> +        ScrollbarTheme::nativeTheme()->updateScrollbarOverlayStyle(verticalScrollbar());
> 
> Why is this code here instead of in the ScrollableArea base class?

Fixed.

>> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:239
>> +    }
> 
> The old indenting style is the correct style for WebKit, believe it or not.

That's odd; does Xcode reindent code when you copy/paste it around?

[Tim Horton]

Created attachment 101242 [details]
work around <rdar://problem/9797253>

[WebKit Review Bot]

Comment on attachment 101242 [details]
work around <rdar://problem/9797253>

Attachment 101242 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9112503

New failing tests:
fast/css/stylesheet-candidate-nodes-crash.xhtml

[Darin Adler]

Comment on attachment 101242 [details]
work around <rdar://problem/9797253>

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

I think the EWS failure was a false positive.

> Source/WebCore/platform/ScrollableArea.cpp:227
> +    if (horizontalScrollbar())
> +        ScrollbarTheme::nativeTheme()->updateScrollbarOverlayStyle(horizontalScrollbar());
> +    if (verticalScrollbar())
> +        ScrollbarTheme::nativeTheme()->updateScrollbarOverlayStyle(verticalScrollbar());
> +
> +    if (horizontalScrollbar())
> +        horizontalScrollbar()->invalidate();
> +
> +    if (verticalScrollbar())
> +        verticalScrollbar()->invalidate();

Could we merge these if statements for clarity?

> Source/WebCore/platform/mac/ScrollViewMac.mm:215
> +    switch (style) {
> +        case ScrollbarOverlayStyleDark:
> +            return NSScrollerKnobStyleDark;
> +        case ScrollbarOverlayStyleLight:
> +            return NSScrollerKnobStyleLight;
> +        default:
> +            return NSScrollerKnobStyleDefault;
> +    }

Same switch statement indentation problem.

[WebKit Review Bot]

Comment on attachment 101242 [details]
work around <rdar://problem/9797253>

Rejecting attachment 101242 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:

  perf/object-keys.html = TEXT PASS


Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  fast/css/stylesheet-candidate-nodes-crash.xhtml = CRASH



Full output: http://queues.webkit.org/results/9136008

[Tim Horton]

(In reply to comment #16)

While the rest of the failures seem to be happening to everyone, this one:

fast/css/stylesheet-candidate-nodes-crash.xhtml = CRASH

seems to be legitimate (can reproduce with patch, cannot without). But the backtrace certainly doesn't make it obvious how this patch is causing this failure. I'll take a look.

[Tim Horton]

(In reply to comment #17)
> (In reply to comment #16)
> 
> While the rest of the failures seem to be happening to everyone, this one:
> 
> fast/css/stylesheet-candidate-nodes-crash.xhtml = CRASH
> 
> seems to be legitimate (can reproduce with patch, cannot without). But the backtrace certainly doesn't make it obvious how this patch is causing this failure. I'll take a look.

It was the updateStyleIfNeeded() added to FrameView::documentBackgroundColor.

[Darin Adler]

(In reply to comment #18)
> It was the updateStyleIfNeeded() added to FrameView::documentBackgroundColor.

Very interesting. Maybe we need to make that change all alone in a separate patch and figure out what callers have difficulty.

I guess that could be done *after* this fix if the fix works without it. Or *before* if it really doesn't.

[Tim Horton]

Created attachment 101341 [details]
remove updateStyleIfNeeded()

> I guess that could be done *after* this fix if the fix works without it. Or *before* if it really doesn't.

Everything works fine without it as far as my manual tests go (I added it solely because of your comment, though it certainly makes sense), so my vote is for solve that mystery after this patch is landed.

[WebKit Review Bot]

Comment on attachment 101341 [details]
remove updateStyleIfNeeded()

Clearing flags on attachment: 101341

Committed r91435: <http://trac.webkit.org/changeset/91435>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It seems like this broke Snow Leopard build:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/32163

[Ryosuke Niwa]

This patch appears to have caused valgrind error:

19:02:10 memcheck_analyze.py [ERROR] FAIL! There were 1 errors: 
19:02:10 memcheck_analyze.py [ERROR] Command: src/sconsbuild/Release/test_shell_tests --gtest_filter=-BMPImageDecoderTest.FLAKY_ChunkedDecodingSlow:MediaLeakTest.VideoBear:MediaLeakTest.FAILS_VideoBear:WebFrameTest.GetFullHtmlOfPage:BMPImageDecoderTest.FLAKY_DecodingSlow:BMPImageDecoderTest.FAILS_ChunkedDecodingSlow:MediaLeakTest.FLAKY_VideoBear:MediaLeakTest.ManyVideoBear:BMPImageDecoderTest.DecodingSlow:MediaLeakTest.FAILS_ManyVideoBear:WebFrameTest.FAILS_GetFullHtmlOfPage:MediaLeakTest.FLAKY_ManyVideoBear:BMPImageDecoderTest.ChunkedDecodingSlow:BMPImageDecoderTest.FAILS_DecodingSlow:WebFrameTest.FLAKY_GetFullHtmlOfPage --gtest_print_time
UninitCondition
Conditional jump or move depends on uninitialised value(s)
  WebCore::FrameView::recalculateScrollbarOverlayStyle() (third_party/WebKit/Source/WebCore/page/FrameView.cpp:344)
  WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) (third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp:341)
  WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) (third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:233)
  WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>) (third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:1631)
  WebCore::RenderObject::setAnimatableStyle(WTF::PassRefPtr<WebCore::RenderStyle>) (third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:1544)
  WebCore::NodeRendererFactory::createRendererAndStyle() (third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:287)
  WebCore::NodeRendererFactory::createRendererIfNeeded() (third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:316)
  WebCore::Node::createRendererIfNeeded() (third_party/WebKit/Source/WebCore/dom/Node.cpp:1454)
  WebCore::Element::attach() (third_party/WebKit/Source/WebCore/dom/Element.cpp:1015)
  WTF::PassRefPtr<WebCore::Element> WebCore::HTMLConstructionSite::attach<WebCore::Element>(WebCore::ContainerNode*, WTF::PassRefPtr<WebCore::Element>) (third_party/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:111)
  WebCore::HTMLConstructionSite::insertHTMLHtmlStartTagBeforeHTML(WebCore::AtomicHTMLToken&) (third_party/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:186)
  WebCore::HTMLTreeBuilder::defaultForBeforeHTML() (third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2685)
  WebCore::HTMLTreeBuilder::processEndOfFile(WebCore::AtomicHTMLToken&) (third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2594)
  WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken&) (third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:493)
  WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&) (third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:462)
  WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) (third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:452)
  WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) (third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:276)
  WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) (third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:175)
  WebCore::HTMLDocumentParser::prepareToStopParsing() (third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:140)
  WebCore::HTMLDocumentParser::attemptToEnd() (third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:399)
  WebCore::HTMLDocumentParser::finish() (third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:426)
  WebCore::DocumentWriter::endIfNotLoadingMainResource() (third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:226)
  WebCore::DocumentWriter::end() (third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:209)
  WebCore::FrameLoader::init() (third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:233)
  WebCore::Frame::init() (third_party/WebKit/Source/WebCore/page/Frame.h:286)
  WebKit::WebFrameImpl::initializeAsMainFrame(WebKit::WebViewImpl*) (third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1883)
  WebKit::WebViewImpl::initializeMainFrame(WebKit::WebFrameClient*) (third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:271)
  WebViewHost::Create(_GtkWidget*, TestWebViewDelegate*, WebKit::WebDevToolsAgentClient*, WebPreferences const&) (webkit/tools/test_shell/webview_host_gtk.cc:34)
  TestShell::Initialize(GURL const&) (webkit/tools/test_shell/test_shell_gtk.cc:349)
  TestShell::CreateNewWindow(GURL const&, TestShell**) (webkit/tools/test_shell/test_shell.cc:203)
Suppression (error hash=#1E5287B6D215189B#):
  For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-valgrind#TOC-Suppressing-Errors
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:_ZN7WebCore9FrameView32recalculateScrollbarOverlayStyleEv
   fun:_ZN7WebCore9RenderBox14styleDidChangeENS_15StyleDifferenceEPKNS_11RenderStyleE
   fun:_ZN7WebCore11RenderBlock14styleDidChangeENS_15StyleDifferenceEPKNS_11RenderStyleE
   fun:_ZN7WebCore12RenderObject8setStyleEN3WTF10PassRefPtrINS_11RenderStyleEEE
   fun:_ZN7WebCore12RenderObject18setAnimatableStyleEN3WTF10PassRefPtrINS_11RenderStyleEEE
   fun:_ZN7WebCore19NodeRendererFactory22createRendererAndStyleEv
   fun:_ZN7WebCore19NodeRendererFactory22createRendererIfNeededEv
   fun:_ZN7WebCore4Node22createRendererIfNeededEv
   fun:_ZN7WebCore7Element6attachEv
   fun:_ZN7WebCore20HTMLConstructionSite6attachINS_7ElementEEEN3WTF10PassRefPtrIT_EEPNS_13ContainerNodeES6_
   fun:_ZN7WebCore20HTMLConstructionSite32insertHTMLHtmlStartTagBeforeHTMLERNS_15AtomicHTMLTokenE
   fun:_ZN7WebCore15HTMLTreeBuilder20defaultForBeforeHTMLEv
   fun:_ZN7WebCore15HTMLTreeBuilder16processEndOfFileERNS_15AtomicHTMLTokenE
   fun:_ZN7WebCore15HTMLTreeBuilder12processTokenERNS_15AtomicHTMLTokenE
   fun:_ZN7WebCore15HTMLTreeBuilder28constructTreeFromAtomicTokenERNS_15AtomicHTMLTokenE
   fun:_ZN7WebCore15HTMLTreeBuilder22constructTreeFromTokenERNS_9HTMLTokenE
   fun:_ZN7WebCore18HTMLDocumentParser13pumpTokenizerENS0_15SynchronousModeE
   fun:_ZN7WebCore18HTMLDocumentParser23pumpTokenizerIfPossibleENS0_15SynchronousModeE
   fun:_ZN7WebCore18HTMLDocumentParser20prepareToStopParsingEv
   fun:_ZN7WebCore18HTMLDocumentParser12attemptToEndEv
   fun:_ZN7WebCore18HTMLDocumentParser6finishEv
   fun:_ZN7WebCore14DocumentWriter27endIfNotLoadingMainResourceEv
}

[Ryosuke Niwa]

It appears that m_scrollbarOverlayStyle added to ScrollableArea is never initialized :(

[Tim Horton]

(In reply to comment #25)
> It appears that m_scrollbarOverlayStyle added to ScrollableArea is never initialized :(

Eek! It needs to be initialized to ScrollbarOverlayStyleDefault. I'll do that tomorrow when I get back to my machine.

Is there a way to run valgrind tests on my machine? (though this one should have been caught simply by not making the mistake in the first place)

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #25)
> > It appears that m_scrollbarOverlayStyle added to ScrollableArea is never initialized :(
> 
> Eek! It needs to be initialized to ScrollbarOverlayStyleDefault. I'll do that tomorrow when I get back to my machine.

Indeed!  Fixed in http://trac.webkit.org/changeset/91447.

> Is there a way to run valgrind tests on my machine? (though this one should have been caught simply by not making the mistake in the first place)

There's a documentation on chromium.org: http://www.chromium.org/developers/how-tos/using-valgrind

But I've never used it myself so I'm not sure how much of that is applicable to Mac port.

If anything, Chromium Linux has Valgrind bot:
http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux%20Valgrind

We also have a shiny Snow Leopard Clang bot:
http://build.chromium.org/p/chromium.webkit/waterfall?builder=Mac%20Clang%20Builder%20%28dbg%29

[Tim Horton]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > It appears that m_scrollbarOverlayStyle added to ScrollableArea is never initialized :(
> > 
> > Eek! It needs to be initialized to ScrollbarOverlayStyleDefault. I'll do that tomorrow when I get back to my machine.
> 
> Indeed!  Fixed in http://trac.webkit.org/changeset/91447.

Ah, you've been great today, cleaning up after all my mistakes. Thanks!

> > Is there a way to run valgrind tests on my machine? (though this one should have been caught simply by not making the mistake in the first place)
> 
> There's a documentation on chromium.org: http://www.chromium.org/developers/how-tos/using-valgrind
> 
> But I've never used it myself so I'm not sure how much of that is applicable to Mac port.
> 
> If anything, Chromium Linux has Valgrind bot:
> http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux%20Valgrind
> 
> We also have a shiny Snow Leopard Clang bot:
> http://build.chromium.org/p/chromium.webkit/waterfall?builder=Mac%20Clang%20Builder%20%28dbg%29

Excellent, I'll take a look!

[Simon Fraser (smfr)]

Comment on attachment 101341 [details]
remove updateStyleIfNeeded()

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

> Source/WebCore/ChangeLog:14
> +        No new tests, since this code is only enabled on future versions of Mac OS X.

No longer future!