Bug 198615

Summary: Remove Dashboard support
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, darin, ews-watchlist, mitz, rniwa, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=198624
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Patch
rniwa: review+, rniwa: commit-queue-
For landing none

Description Sam Weinig 2019-06-06 11:33:26 PDT 
Remove Dashboard support
Comment 1 Sam Weinig 2019-06-06 11:39:14 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2019-06-06 11:44:37 PDT Comment hidden (obsolete)
Comment 3 Tim Horton 2019-06-06 15:42:29 PDT 
Comment on attachment 371516 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:114
> -        CSS_UNICODE_RANGE = 102,
> +        CSS_UNICODE_RANGE = 101,

Odd, are these all explicit numbers for a reason?

> Source/WebCore/css/StyleResolver.cpp:-121
> -#if ENABLE(DASHBOARD_SUPPORT)
> -#endif
> -
> -#if ENABLE(VIDEO_TRACK)
> -#endif

WTF

> Tools/DumpRenderTree/mac/Configurations/Base.xcconfig:47
>  GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) U_DISABLE_RENAMING=1 U_SHOW_CPLUSPLUS_API=0 $(GCC_PREPROCESSOR_DEFINITIONS_$(PLATFORM_NAME));

Do we need this anymore?

> Tools/ImageDiff/cg/Configurations/Base.xcconfig:38
>  GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) $(GCC_PREPROCESSOR_DEFINITIONS_$(PLATFORM_NAME));

Do we need this anymore?
Comment 4 Sam Weinig 2019-06-06 15:55:32 PDT 
(In reply to Tim Horton from comment #3)
> Comment on attachment 371516 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371516&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.h:114
> > -        CSS_UNICODE_RANGE = 102,
> > +        CSS_UNICODE_RANGE = 101,
> 
> Odd, are these all explicit numbers for a reason?

I was wondering that too. They also aren't IDL constants or anything, so they can probably not shout at us. I'll modernize it as a follow up.

> 
> > Source/WebCore/css/StyleResolver.cpp:-121
> > -#if ENABLE(DASHBOARD_SUPPORT)
> > -#endif
> > -
> > -#if ENABLE(VIDEO_TRACK)
> > -#endif
> 
> WTF
> 
> > Tools/DumpRenderTree/mac/Configurations/Base.xcconfig:47
> >  GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) U_DISABLE_RENAMING=1 U_SHOW_CPLUSPLUS_API=0 $(GCC_PREPROCESSOR_DEFINITIONS_$(PLATFORM_NAME));
> 
> Do we need this anymore?

Nope, will fix.

> 
> > Tools/ImageDiff/cg/Configurations/Base.xcconfig:38
> >  GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) $(GCC_PREPROCESSOR_DEFINITIONS_$(PLATFORM_NAME));
> 
> Do we need this anymore?

Nope, will fix.
Comment 5 Sam Weinig 2019-06-06 16:21:03 PDT 
*** Bug 198624 has been marked as a duplicate of this bug. ***
Comment 6 Sam Weinig 2019-06-06 16:37:27 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2019-06-06 16:56:01 PDT 
Anyone know what's going on with the inability to apply the change?

Failed to find the property value for the SVN property "svn:mergeinfo": "Deleted: svn:mime-type
". at /home/ews-gtk/WebKit/Tools/Scripts/VCSUtils.pm line 1546, <ARGV> line 4056.
Comment 8 Sam Weinig 2019-06-06 16:58:15 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-06-06 17:01:39 PDT Comment hidden (obsolete)
Comment 10 Sam Weinig 2019-06-06 17:27:18 PDT 
Created attachment 371541 [details]
Patch
Comment 11 Sam Weinig 2019-06-06 17:31:39 PDT 
(In reply to Sam Weinig from comment #7)
> Anyone know what's going on with the inability to apply the change?
> 
> Failed to find the property value for the SVN property "svn:mergeinfo":
> "Deleted: svn:mime-type
> ". at /home/ews-gtk/WebKit/Tools/Scripts/VCSUtils.pm line 1546, <ARGV> line
> 4056.

I just manually removed the svn:mergeinfo lines from the patch and it seems to work. Fun.
Comment 12 EWS Watchlist 2019-06-06 17:31:56 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-06-06 18:53:45 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-06-06 18:53:47 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2019-06-06 19:05:52 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-06-06 19:05:54 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-06-06 19:32:23 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-06-06 19:32:25 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-06-06 20:03:25 PDT Comment hidden (obsolete)
Comment 20 Sam Weinig 2019-06-09 11:26:58 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2019-06-09 12:57:51 PDT Comment hidden (obsolete)
Comment 22 EWS Watchlist 2019-06-09 12:57:52 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2019-06-09 13:37:38 PDT Comment hidden (obsolete)
Comment 24 EWS Watchlist 2019-06-09 13:37:39 PDT Comment hidden (obsolete)
Comment 25 Sam Weinig 2019-06-09 16:36:08 PDT 
Created attachment 371714 [details]
Patch
Comment 26 Ryosuke Niwa 2019-06-10 12:51:00 PDT 
Windows failure:
C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/TestNetscapePlugIn.exp
c:\cygwin\home\buildbot\webkit\tools\dumprendertree\win\testrunnerwin.cpp(695): error C2039: 'setUseDashboardCompatibilityMode': is not a member of 'TestRunner'
Comment 27 Darin Adler 2019-06-10 13:39:27 PDT 
Comment on attachment 371714 [details]
Patch

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

Exciting patch. Those "FIXME: Remove" comments are calling my name!

> Source/WebKitLegacy/mac/WebView/WebClipView.mm:-187
> -- (void)scrollWheel:(NSEvent *)event
> -{
> -    NSView *docView = [self documentView];
> -    if ([docView respondsToSelector:@selector(_webView)]) {
> -#if ENABLE(DASHBOARD_SUPPORT)
> -        WebView *wv = [docView _webView];
> -        if ([wv _dashboardBehavior:WebDashboardBehaviorAllowWheelScrolling]) {
> -            [super scrollWheel:event];
> -        }
> -#endif
> -        return;
> -    }
> -    [super scrollWheel:event];
> -}

This is a behavior change. Before, this would just return and not call super if the documentView responds to the selector _webView.

Is the behavior change an accident, or intentional?
Comment 28 Sam Weinig 2019-06-10 14:30:56 PDT 
(In reply to Darin Adler from comment #27)
> Comment on attachment 371714 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371714&action=review
> 
> Exciting patch. Those "FIXME: Remove" comments are calling my name!
> 
> > Source/WebKitLegacy/mac/WebView/WebClipView.mm:-187
> > -- (void)scrollWheel:(NSEvent *)event
> > -{
> > -    NSView *docView = [self documentView];
> > -    if ([docView respondsToSelector:@selector(_webView)]) {
> > -#if ENABLE(DASHBOARD_SUPPORT)
> > -        WebView *wv = [docView _webView];
> > -        if ([wv _dashboardBehavior:WebDashboardBehaviorAllowWheelScrolling]) {
> > -            [super scrollWheel:event];
> > -        }
> > -#endif
> > -        return;
> > -    }
> > -    [super scrollWheel:event];
> > -}
> 
> This is a behavior change. Before, this would just return and not call super
> if the documentView responds to the selector _webView.
> 
> Is the behavior change an accident, or intentional?

I actually don't think this is a behavior change since this method is only ever called in mac Legacy WebKit, where ENABLE(DASHBOARD_SUPPORT) was true, and [wv _dashboardBehavior:WebDashboardBehaviorAllowWheelScrolling] always returned true by default (this is due to the underlying value, dashboardBehaviorAllowWheelScrolling in WebViewData.mm being initialized to YES, unlike it's peers, that default initialize to NO).
Comment 29 Sam Weinig 2019-06-10 14:32:07 PDT 
Created attachment 371776 [details]
For landing
Comment 30 WebKit Commit Bot 2019-06-10 15:16:45 PDT 
Comment on attachment 371776 [details]
For landing

Clearing flags on attachment: 371776

Committed r246285: <https://trac.webkit.org/changeset/246285>
Comment 31 Darin Adler 2019-06-10 15:53:19 PDT 
Comment on attachment 371714 [details]
Patch

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

>>> Source/WebKitLegacy/mac/WebView/WebClipView.mm:-187
>>> -}
>> 
>> This is a behavior change. Before, this would just return and not call super if the documentView responds to the selector _webView.
>> 
>> Is the behavior change an accident, or intentional?
> 
> I actually don't think this is a behavior change since this method is only ever called in mac Legacy WebKit, where ENABLE(DASHBOARD_SUPPORT) was true, and [wv _dashboardBehavior:WebDashboardBehaviorAllowWheelScrolling] always returned true by default (this is due to the underlying value, dashboardBehaviorAllowWheelScrolling in WebViewData.mm being initialized to YES, unlike it's peers, that default initialize to NO).

Great! I’m relieved to hear that.
Comment 32 Radar WebKit Bug Importer 2019-06-10 18:13:25 PDT 
<rdar://problem/51606093>