[Webkit-unassigned] [Bug 42716] Upstream Android support for web archive

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42716


Steve Block <steveblock at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62135|                            |review-
               Flag|                            |




--- Comment #2 from Steve Block <steveblock at google.com>  2010-07-21 03:01:52 PST ---
(From update of attachment 62135)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.