Summary: | Add page overlays that show regions with mouseWheel event handlers, and the non-fast-scrollable region, and code to toggle them in MiniBrowser WK2 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, cdumez, commit-queue, gyuyoung.kim, joepeck, rakuco, ryuan.choi, sam, sergio, simon.fraser, thorton, timothy | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2014-10-31 13:48:19 PDT
Created attachment 240751 [details]
Patch
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 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? > > 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. (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!! Created attachment 240761 [details]
Patch
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 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 on attachment 240761 [details]
Patch
Tim reviewed this.
|