Bug 211389 - Remove HAVE(IOSURFACE) checks in Cocoa-platform-specific code
Summary: Remove HAVE(IOSURFACE) checks in Cocoa-platform-specific code
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: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-04 10:32 PDT by Darin Adler
Modified: 2020-05-04 18:01 PDT (History)
16 users (show)

See Also:


Attachments
Patch (46.15 KB, patch)
2020-05-04 12:44 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (46.28 KB, patch)
2020-05-04 12:54 PDT, Darin Adler
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-05-04 10:32:42 PDT
Remove HAVE(IOSURFACE) checks in Cocoa-platform-specific code
Comment 1 Darin Adler 2020-05-04 12:44:50 PDT
Created attachment 398401 [details]
Patch
Comment 2 Darin Adler 2020-05-04 12:54:57 PDT
Created attachment 398403 [details]
Patch
Comment 3 Anders Carlsson 2020-05-04 15:10:40 PDT
(In reply to Darin Adler from comment #0)
> Remove HAVE(IOSURFACE) checks in Cocoa-platform-specific code

Is this true even for the simulator nowadays? (I'm assuming yes since ios-sim is green).
Comment 4 Darin Adler 2020-05-04 15:54:12 PDT
The relevant quote from PlatformHave.h is:

#if PLATFORM(COCOA)
#define HAVE_IOSURFACE 1
#endif

I was going based on that. Not changing anything.
Comment 5 Alexey Proskuryakov 2020-05-04 16:24:06 PDT
Comment on attachment 398403 [details]
Patch

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

> Source/WebKit/ChangeLog:35
> +        Merged #if expressions into a single one.

I went back to ChangeLog to understand why. Since this is in Cocoa directory, when is HAVE(IOSURFACE) false here?

> Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:-58
> -    // TODO: <rdar://problem/6558366> DumpRenderTree: Investigate testRepaintSweepHorizontally and dumpSelectionRect
> -    BEGIN_BLOCK_OBJC_EXCEPTIONS;

I'm not sure if/how the TODO is related to BEGIN_BLOCK_OBJC_EXCEPTIONS. In fact, they were added at different times. Perhaps it should stay?
Comment 6 Tim Horton 2020-05-04 16:29:02 PDT
(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 398403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398403&action=review
> 
> > Source/WebKit/ChangeLog:35
> > +        Merged #if expressions into a single one.
> 
> I went back to ChangeLog to understand why. Since this is in Cocoa
> directory, when is HAVE(IOSURFACE) false here?

Used to be false in the Sim, right? But no longer.
Comment 7 Tim Horton 2020-05-04 16:29:44 PDT
(In reply to Tim Horton from comment #6)
> (In reply to Alexey Proskuryakov from comment #5)
> > Comment on attachment 398403 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=398403&action=review
> > 
> > > Source/WebKit/ChangeLog:35
> > > +        Merged #if expressions into a single one.
> > 
> > I went back to ChangeLog to understand why. Since this is in Cocoa
> > directory, when is HAVE(IOSURFACE) false here?
> 
> Used to be false in the Sim, right? But no longer.

Oh, undersea already said that.
Comment 8 Darin Adler 2020-05-04 17:37:32 PDT
Comment on attachment 398403 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:35
>>>> +        Merged #if expressions into a single one.
>>> 
>>> I went back to ChangeLog to understand why. Since this is in Cocoa directory, when is HAVE(IOSURFACE) false here?
>> 
>> Used to be false in the Sim, right? But no longer.
> 
> Oh, undersea already said that.

OK, will go further on this.

>> Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:-58
>> -    BEGIN_BLOCK_OBJC_EXCEPTIONS;
> 
> I'm not sure if/how the TODO is related to BEGIN_BLOCK_OBJC_EXCEPTIONS. In fact, they were added at different times. Perhaps it should stay?

Yes, they are not related.

I removed the two for two different reasons, and forgot to mention one in the change log.

I removed the "TODO" comment because there is no real reason to have a comment in the code. It’s OK to have a bug about perhaps implementing this some day, but I don’t think the comment in the code helps enough to be worth it. The real driver is the skipped tests, not the comment in the code. Also, these are features of pixel tests. I’m thinking that pixel tests of legacy WebKit are not a very high priority for future enhancement.

If you don’t agree, I would be happy to bring the TODO back (although I will say FIXME instead).
Comment 9 Darin Adler 2020-05-04 17:37:53 PDT
(In reply to Tim Horton from comment #7)
> Oh, undersea already said that.

Thanks, autocorrect!
Comment 10 Darin Adler 2020-05-04 18:00:43 PDT
Committed r261133: <https://trac.webkit.org/changeset/261133>
Comment 11 Radar WebKit Bug Importer 2020-05-04 18:01:22 PDT
<rdar://problem/62870016>