Bug 203809 - REGRESSION(r243947) Epson software updater fails to install new version
Summary: REGRESSION(r243947) Epson software updater fails to install new version
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: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-04 06:37 PST by Alex Christensen
Modified: 2019-11-04 10:59 PST (History)
4 users (show)

See Also:

Attachments
Patch (9.23 KB, patch)
2019-11-04 06:38 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (9.30 KB, patch)
2019-11-04 08:48 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (9.48 KB, patch)
2019-11-04 09:33 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

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