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
Summary: Add page overlays that show regions with mouseWheel event handlers, and the n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-31 13:48 PDT by Simon Fraser (smfr)
Modified: 2014-11-03 16:13 PST (History)
12 users (show)

See Also:


Attachments
Patch (41.35 KB, patch)
2014-10-31 13:56 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (44.06 KB, patch)
2014-10-31 15:29 PDT, Simon Fraser (smfr)
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-10-31 13:48:19 PDT
Add page overlays that show regions with mouseWheel event handlers, and the non-fast-scrollable region, and code to toggle them in MiniBrowser WK2
Comment 1 Simon Fraser (smfr) 2014-10-31 13:56:14 PDT
Created attachment 240751 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-31 13:58:07 PDT
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.
Comment 3 Tim Horton 2014-10-31 14:08:36 PDT
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?
Comment 4 Simon Fraser (smfr) 2014-10-31 14:13:46 PDT
> > 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.
Comment 5 Tim Horton 2014-10-31 14:20:52 PDT
(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!!
Comment 6 Simon Fraser (smfr) 2014-10-31 15:29:31 PDT
Created attachment 240761 [details]
Patch
Comment 7 WebKit Commit Bot 2014-10-31 15:32:09 PDT
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.
Comment 8 Chris Dumez 2014-10-31 16:22:55 PDT
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
Comment 9 Simon Fraser (smfr) 2014-11-03 16:13:01 PST
https://trac.webkit.org/r175494
Comment 10 Simon Fraser (smfr) 2014-11-03 16:13:12 PST
Comment on attachment 240761 [details]
Patch

Tim reviewed this.