[webkit-reviews] review granted: [Bug 192612] HTTPS Upgrade: Scripts / preprocessing necessary to create new database in future : [Attachment 357362] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Vivek Seth <v_seth at apple.com>'s
request for review:
Bug 192612: HTTPS Upgrade: Scripts / preprocessing necessary to create new
database in future
https://bugs.webkit.org/show_bug.cgi?id=192612

Attachment 357362: Patch

https://bugs.webkit.org/attachment.cgi?id=357362&action=review




--- Comment #13 from Andy Estes <aestes at apple.com> ---
Comment on attachment 357362
  --> https://bugs.webkit.org/attachment.cgi?id=357362
Patch

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

> Source/WebKit/DerivedSources.make:337
> +ifeq ($(USE_INTERNAL_SDK),YES)
> +	ifeq ($(ENABLE_HTTPS_UPGRADE),ENABLE_HTTPS_UPGRADE)
> +
> +		ifeq ($(WK_PLATFORM_NAME),macosx)
> +			WK_CREATE_HTTPS_UPGRADE_DATABASE = YES
> +		endif # WK_PLATFORM_NAME
> +
> +		ifeq ($(WK_PLATFORM_NAME),iphoneos)
> +			WK_CREATE_HTTPS_UPGRADE_DATABASE = YES
> +		endif # WK_PLATFORM_NAME
> +	endif # ENABLE_HTTPS_UPGRADE
> +endif # USE_INTERNAL_SDK
> +
> +ifeq ($(WK_CREATE_HTTPS_UPGRADE_DATABASE),YES)

This seems too complicated. Usually we'd use the values of WK_PLATFORM_NAME and
USE_INTERNAL_SDK to set the value of ENABLE_HTTPS_UPGRADE in
FeatureDefines.xcconfig and/or FeatureDefines.h. If we did that, then here
you'd only need to check if $(ENABLE_HTTPS_UPGRADE) equals
ENABLE_HTTPS_UPGRADE.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:1
> +# This script requires that HTTPSUpgradeList.txt has the following format:

You should include a copyright and license at the top of this file.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:18
> +# Return early if we don't have HTTPSUpgradeList.txt.

This comment seems unnecessary (we typically only write "why" comments, not
"what" comments).

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:24
> +# Delete database if it exists.

Ditto.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:29
> +# Create database.

Ditto.