Bug 203809

Summary: REGRESSION(r243947) Epson software updater fails to install new version
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alex Christensen 2019-11-04 06:37:04 PST
REGRESSION(r243947) Epson software updater fails to install new version
Comment 1 Alex Christensen 2019-11-04 06:38:35 PST
Created attachment 382735 [details]
Patch
Comment 2 Alex Christensen 2019-11-04 06:38:40 PST
<rdar://problem/56002179>
Comment 3 Alex Christensen 2019-11-04 08:48:38 PST
Created attachment 382742 [details]
Patch
Comment 4 Brady Eidson 2019-11-04 09:01:31 PST
Comment on attachment 382742 [details]
Patch

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

> Source/WebKitLegacy/mac/Misc/WebDownload.mm:122
> +    if (isDelegateThread())

Is there any reason we even have "isDelegateThread"?

Couldn't we just always use "callOnDelegateThread"?

Even the forms of this that need a sync return value should be able to work with that, right?
Comment 5 Alex Christensen 2019-11-04 09:19:02 PST
Comment on attachment 382742 [details]
Patch

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

>> Source/WebKitLegacy/mac/Misc/WebDownload.mm:122
>> +    if (isDelegateThread())
> 
> Is there any reason we even have "isDelegateThread"?
> 
> Couldn't we just always use "callOnDelegateThread"?
> 
> Even the forms of this that need a sync return value should be able to work with that, right?

We would need callOnDelegateThread and callOnDelegateThreadAndWait.  Do you think that would be better?
Comment 6 Brady Eidson 2019-11-04 09:23:27 PST
(In reply to Alex Christensen from comment #5)
> Comment on attachment 382742 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382742&action=review
> 
> >> Source/WebKitLegacy/mac/Misc/WebDownload.mm:122
> >> +    if (isDelegateThread())
> > 
> > Is there any reason we even have "isDelegateThread"?
> > 
> > Couldn't we just always use "callOnDelegateThread"?
> > 
> > Even the forms of this that need a sync return value should be able to work with that, right?
> 
> We would need callOnDelegateThread and callOnDelegateThreadAndWait.  Do you
> think that would be better?

Oh, I was (incorrectly) thinking the sync behavior was managed through a local variable of some sort.

I think it'd be better. It would clean up every single call site into two methods with the complexity.
Comment 7 Alex Christensen 2019-11-04 09:33:50 PST
Created attachment 382746 [details]
Patch
Comment 8 Brady Eidson 2019-11-04 09:34:49 PST
Comment on attachment 382746 [details]
Patch

I think this is much better.
Comment 9 WebKit Commit Bot 2019-11-04 10:21:03 PST
Comment on attachment 382746 [details]
Patch

Clearing flags on attachment: 382746

Committed r252000: <https://trac.webkit.org/changeset/252000>
Comment 10 WebKit Commit Bot 2019-11-04 10:21:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Geoffrey Garen 2019-11-04 10:59:54 PST
Comment on attachment 382746 [details]
Patch

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

> Source/WebKitLegacy/mac/Misc/WebDownload.mm:50
> +    static bool isOldEpsonSoftwareUpdater = WebCore::MacApplication::isEpsonSoftwareUpdater() && dyld_get_program_sdk_version() < DYLD_MACOSX_VERSION_10_15;
> +    return isOldEpsonSoftwareUpdater;

Even though this is LegacyWebKit code, which technically works with dyld_get_program_sdk_version(), it's better style to use applicationSDKVersion(), which works for LegacyWebKit and WebKit.