[webkit-reviews] review denied: [Bug 42716] Upstream Android support for web archive : [Attachment 62135] Patch for Android web archive support.

[bugzilla-daemon at webkit.org]

Steve Block <steveblock at google.com> has denied	review:
Bug 42716: Upstream Android support for web archive
https://bugs.webkit.org/show_bug.cgi?id=42716

Attachment 62135: Patch for Android web archive support.
https://bugs.webkit.org/attachment.cgi?id=62135&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
Thanks for the patch Elliott. I'm not familiar with WebArchive, but some
general comments ...

The bug title should be written from the perspective of WebKit, not Android, eg
'Implement WebArchive for Android'.

You should set r? on your patch if you wish it to be reviewed. This will also
trigger the EWS bots to check style and try to build it.

ChangeLog
 +	https://bugs.webkit.org/show_bug.cgi?id=42716
Indentation. Should always be spaces, not tabs.

WebCore/ChangeLog:6
 +	https://bugs.webkit.org/show_bug.cgi?id=42716
Indentation

WebCore/ChangeLog:13
 +	    Tests are not included as this feature is Android-specific.
The feature is not Android specific. You can just say something like 'platform
implementation, no new functionality, existing tests suffice'

WebCore/config.h:103
 +  #define ENABLE_ARCHIVE 1
ENABLE_ARCHIVE is an Android-specific flag added to Android WebKit to allow
WebArchive to be disabled. It does not exist upstream (other than an erroneous
mention here it seems), so we shouldn't use it. If you'd like to add such a
flag to WebKit, that should be in a separate patch.

WebCore/loader/archive/android/WebArchiveAndroid.cpp:54
 +	    Vector<PassRefPtr<Archive> >& subframeArchives)
WebKit style is to indent 4 spaces. Did you run check-webkit-style? See also
http://webkit.org/coding/coding-style.html.

WebCore/loader/archive/android/WebArchiveAndroid.cpp:84
 +	     subresourcesIterator++) {
WebKit has no maximum line length, so this would probably stay on one line.

WebCore/loader/archive/android/WebArchiveAndroid.cpp:86
 +	}
No braces on one-line control statements.

WebCore/loader/archive/android/WebArchiveAndroid.cpp:118
 +	    LOGD("loadWebArchive: Failed to load field.");
LOGD is an Android function, so probably shouldn't be used from WebCore.

WebCore/loader/archive/android/WebArchiveAndroid.cpp:273
 +	/* When an archive cannot be loaded, we return an empty archive
instead. */
Use C++ style comments

WebCore/loader/archive/android/WebArchiveAndroid.h:55
 +  #endif // WEBARCHIVEANDROID_H
Incorrect label