Bug 198615 - Remove Dashboard support
Summary: Remove Dashboard support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
: 198624 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-06-06 11:33 PDT by Sam Weinig
Modified: 2019-06-10 18:13 PDT (History)
9 users (show)

See Also:


Attachments
Patch (242.96 KB, patch)
2019-06-06 11:39 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (242.95 KB, patch)
2019-06-06 11:44 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (242.96 KB, patch)
2019-06-06 16:37 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (242.81 KB, patch)
2019-06-06 16:58 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (243.37 KB, patch)
2019-06-06 17:27 PDT, Sam Weinig
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.24 MB, application/zip)
2019-06-06 18:53 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.13 MB, application/zip)
2019-06-06 19:05 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (3.04 MB, application/zip)
2019-06-06 19:32 PDT, EWS Watchlist
no flags Details
Patch (243.46 KB, patch)
2019-06-09 11:26 PDT, Sam Weinig
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.22 MB, application/zip)
2019-06-09 12:57 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (3.07 MB, application/zip)
2019-06-09 13:37 PDT, EWS Watchlist
no flags Details
Patch (243.36 KB, patch)
2019-06-09 16:36 PDT, Sam Weinig
rniwa: review+
rniwa: commit-queue-
Details | Formatted Diff | Diff
For landing (243.38 KB, patch)
2019-06-10 14:32 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>