Bug 138257 – Add page overlays that show regions with mouseWheel event handlers, and the non-fast-scrollable region, and code to toggle them in MiniBrowser WK2

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Add page overlays that show regions with mouseWheel event handlers, and the non-fast-scrollable region, and code to toggle them in MiniBrowser WK2

[Simon Fraser (smfr)]

Created attachment 240751 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:40:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 20 files


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

[Tim Horton]

Comment on attachment 240751 [details]
Patch

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

> Source/WebCore/page/DebugPageOverlays.cpp:56
> +    virtual void pageOverlayDestroyed(PageOverlay&) override;

private?

> Source/WebCore/page/DebugPageOverlays.cpp:66
> +    MainFrame& m_frame;

private?

> Source/WebCore/page/DebugPageOverlays.cpp:278
> +    if (visibleRegions & NonFastScrollableRegion)

We should figure out how to make this more generic at some point.

> Source/WebCore/page/DebugPageOverlays.h:46
> +    static const unsigned NumberOfRegionTypes = NonFastScrollableRegion + 1;

Don't we usually address this by having a third 'last' type after the others, so that this doesn't need to be changed later?

> Source/WebKit2/Shared/WebPreferencesDefinitions.h:240
> +    macro(VisibleDebugOverlayRegions, visibleDebugOverlayRegions, UInt32, uint32_t, 0)

Maybe we should learn from the past and make this more like the new Logging pref (stringified) than the old one?

[Simon Fraser (smfr)]

> > Source/WebCore/page/DebugPageOverlays.h:46
> > +    static const unsigned NumberOfRegionTypes = NonFastScrollableRegion + 1;
> 
> Don't we usually address this by having a third 'last' type after the
> others, so that this doesn't need to be changed later?

That resulted in "enum not handled in switch" warnings, so I did this.

> > Source/WebKit2/Shared/WebPreferencesDefinitions.h:240
> > +    macro(VisibleDebugOverlayRegions, visibleDebugOverlayRegions, UInt32, uint32_t, 0)
> 
> Maybe we should learn from the past and make this more like the new Logging
> pref (stringified) than the old one?

Maybe, but I can deal with 2 bits in my head.

[Tim Horton]

(In reply to comment #4)
> > > Source/WebKit2/Shared/WebPreferencesDefinitions.h:240
> > > +    macro(VisibleDebugOverlayRegions, visibleDebugOverlayRegions, UInt32, uint32_t, 0)
> > 
> > Maybe we should learn from the past and make this more like the new Logging
> > pref (stringified) than the old one?
> 
> Maybe, but I can deal with 2 bits in my head.

But the future!!

[Simon Fraser (smfr)]

Created attachment 240761 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:40:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 23 files


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

[Chris Dumez]

Comment on attachment 240761 [details]
Patch

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

> Source/WebCore/page/DebugPageOverlays.cpp:56
> +    virtual void pageOverlayDestroyed(PageOverlay&) override;

nit: It seems all of these virtual functions could be marked as final.

> Source/WebCore/page/DebugPageOverlays.cpp:81
> +    MouseWheelRegionOverlay(MainFrame& frame)

nit: explicit?

> Source/WebCore/page/DebugPageOverlays.cpp:114
> +    NonFastScrollableRegionOverlay(MainFrame& frame)

nit: explicit?

> Source/WebCore/page/DebugPageOverlays.cpp:188
> +    

nit: extra line?

> Source/WebCore/page/DebugPageOverlays.cpp:222
> +RegionOverlay* DebugPageOverlays::ensureRegionOverlayForFrame(MainFrame& frame, RegionType regionType)

nit: this could return a reference.

> Source/WebCore/page/DebugPageOverlays.cpp:239
> +    m_frameRegionOverlays.set(&frame, WTF::move(visualizers));

nit: using .add() would be a little bit more efficient as we already know the key is not in the HashMap.

> Source/WebCore/page/DebugPageOverlays.h:61
> +        return m_frameRegionOverlays.find(&frame) != m_frameRegionOverlays.end();

nit: return m_frameRegionOverlays.contains(&frame);

> Tools/MiniBrowser/mac/SettingsController.m:110
> +    NSMenu* debugOverlaysMenu = [[NSMenu alloc] initWithTitle:@"Debug Overlays"];

Nit: wrong star placement

[Simon Fraser (smfr)]

https://trac.webkit.org/r175494

[Simon Fraser (smfr)]

Comment on attachment 240761 [details]
Patch

Tim reviewed this.