Bug 209706 – Add a ScrollLatching log channel and improve some logging functionality

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Add a ScrollLatching log channel and improve some logging functionality

[Simon Fraser (smfr)]

Created attachment 394837 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 394840 [details]
Patch

[Darin Adler]

Comment on attachment 394840 [details]
Patch

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

> Source/WTF/wtf/text/TextStream.h:31
> +#include <wtf/WeakPtr.h>

Would be nice to find a way to make the weak pointers easy to stream, but not have TextStream.h need to include WeakPtr.h.

> Source/WebCore/page/mac/EventHandlerMac.mm:1058
> +    const ScrollLatchingState* latchingState = m_frame.page() ? m_frame.page()->latchingState() : nullptr;

How about auto or auto* instead?

> Source/WebCore/page/mac/EventHandlerMac.mm:1102
> +    const ScrollLatchingState* latchingState = m_frame.page() ? m_frame.page()->latchingState() : nullptr;

Ditto.

> Source/WebCore/page/scrolling/ScrollLatchingState.h:40
> -class ScrollLatchingState final {
> +class ScrollLatchingState {

Why this change?

[David Kilzer (:ddkilzer)]

Comment on attachment 394840 [details]
Patch

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

r=me, but please change `int` to `size_t`.

> Source/WebCore/dom/Node.cpp:2619
> +    const int FormatBufferSize = 512;

const size_t please.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #3)
> Comment on attachment 394840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394840&action=review
> 
> > Source/WTF/wtf/text/TextStream.h:31
> > +#include <wtf/WeakPtr.h>
> 
> Would be nice to find a way to make the weak pointers easy to stream, but
> not have TextStream.h need to include WeakPtr.h.

Could move this to WeakPtr, but would have to add a .cpp to avoid pulling TextStream.h into the header.

> > Source/WebCore/page/mac/EventHandlerMac.mm:1058
> > +    const ScrollLatchingState* latchingState = m_frame.page() ? m_frame.page()->latchingState() : nullptr;
> 
> How about auto or auto* instead?

Sure.

> > Source/WebCore/page/scrolling/ScrollLatchingState.h:40
> > -class ScrollLatchingState final {
> > +class ScrollLatchingState {
> 
> Why this change?

ScrollLatchingState doesn't have any virtual member functions, so there doesn't seem to be any expectation that anyone will inherit from it. Is there value in keeping 'final' here?

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/259165/webkit

[Radar WebKit Bug Importer]

<rdar://problem/61018328>

[Darin Adler]

Comment on attachment 394840 [details]
Patch

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

>>> Source/WebCore/page/scrolling/ScrollLatchingState.h:40
>>> +class ScrollLatchingState {
>> 
>> Why this change?
> 
> ScrollLatchingState doesn't have any virtual member functions, so there doesn't seem to be any expectation that anyone will inherit from it. Is there value in keeping 'final' here?

There are various classes that are entirely non-polymorphic with nothing virtual. I would never *add* final to one of those, but I also was thinking I wouldn’t remove it either. It’s kind of like the "const" some people want to put on the many, many local variables. There is possibly some minimal value in clarifying that there are no classes derived from this one.

Anyway, OK to remove it, but I was wondering what your thinking on this way.

[alan]

> I wouldn’t remove it either. It’s kind of like the "const" some people want
> to put on the many, many local variables.
It's really not the right place to discuss it, but I've been meaning to ask your take on this.

[Darin Adler]

(In reply to zalan from comment #9)
> > I wouldn’t remove it either. It’s kind of like the "const" some people want
> > to put on the many, many local variables.
> It's really not the right place to discuss it, but I've been meaning to ask
> your take on this.

I wish this was a language where const was the default and variables you want to modify are the ones with an extra keyword or one like Swift which has both let and var that are equally economical to write and in which you even get a warning if you use a var and never modify it.

But given that this is C++ I think it’s not so great to add constexpr or const to every local variable that we happen not to be modifying after it’s created. I’d rather see lots of auto rather than lots of const auto; the extra keyword just doesn’t seem,worth it. That’s my current take.

If we were putting const on all those locals I guess we could do the same with function arguments in function definitions with the same minimal benefit.