Bug 203809 – REGRESSION(r243947) Epson software updater fails to install new version

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

REGRESSION(r243947) Epson software updater fails to install new version

[Alex Christensen]

Created attachment 382735 [details]
Patch

[Alex Christensen]

<rdar://problem/56002179>

[Alex Christensen]

Created attachment 382742 [details]
Patch

[Brady Eidson]

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?

[Alex Christensen]

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?

[Brady Eidson]

(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.

[Alex Christensen]

Created attachment 382746 [details]
Patch

[Brady Eidson]

Comment on attachment 382746 [details]
Patch

I think this is much better.

[WebKit Commit Bot]

Comment on attachment 382746 [details]
Patch

Clearing flags on attachment: 382746

Committed r252000: <https://trac.webkit.org/changeset/252000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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.