Bug 42716 – Implement WebArchive for Android

• Component: Platform
• Status: RESOLVED WONTFIX

[Elliott Slaughter]

Submitting patch upstream for Android support of web archives.

[Elliott Slaughter]

Created attachment 62135 [details]
Patch for Android web archive support.

[Steve Block]

Comment on attachment 62135 [details]
Patch for Android web archive support.

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

[Elliott Slaughter]

Created attachment 62227 [details]
Android WebArchive patch v2.

[Steve Block]

Adding [e-mail] to CC list, as he seems to be the owner of the archive code. Brady, would you mind taking a look please?

[David Levin]

(In reply to comment #3)
> Created an attachment (id=62227) [details]
> Android WebArchive patch v2.

General comments:

1. Don't use PassRefPtr as a local variable. It should only be a parameter or a return value.

2. Don't include parameter names in declarations when they add no information (like these places "create(Frame* frame);" and "create(SharedBuffer* buffer);" and "saveWebArchive(xmlTextWriterPtr writer);").

3. WebArchiveAndroid.h looks like it is missing includes for several things that it uses like PassRefPtr and vector<>

4. Use c++ style casting "(const char*)xmlNodeGetContent(fieldNode->xmlChildrenNode)"

5. Why does "saveArchive(xmlTextWriterPtr writer, PassRefPtr<Archive> archive)" take a PassRefPtr when it doesn't appear to take ownership of the pointer?

6. In general, wrapped lines should align with the open (. (I think this is just like Google's C++ standard.), so this

     static PassRefPtr<WebArchiveAndroid> create(PassRefPtr<ArchiveResource> mainResource,
         Vector<PassRefPtr<ArchiveResource> >& subresources,
         Vector<PassRefPtr<Archive> >& subframeArchives);

should be

static PassRefPtr<WebArchiveAndroid> create(PassRefPtr<ArchiveResource> mainResource,
                                            Vector<PassRefPtr<ArchiveResource> >& subresources,
                                            Vector<PassRefPtr<Archive> >& subframeArchives);
(which is aligned when using a fixed width font).

[Elliott Slaughter]

Created attachment 63010 [details]
Android WebArchive patch v3.

[Adam Barth]

Comment on attachment 63010 [details]
Android WebArchive patch v3.

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

> WebCore/loader/archive/ArchiveFactory.cpp:68
> +#elif PLATFORM(ANDROID)
> +    mimeTypes.set("application/x-webarchive-xml", archiveFactoryCreate<WebArchiveAndroid>);
Why the wacky MIME type?  Something more sensical would be application/x-webarchive+xml.  Even more sensical would be to use the existing webarchive MIME type.

> WebCore/loader/archive/android/WebArchiveAndroid.cpp:26
> +#define LOG_TAG "webarchive"
What is this?

[Elliott Slaughter]

(In reply to comment #7)
> (From update of attachment 63010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=63010&action=prettypatch
> 
> > WebCore/loader/archive/ArchiveFactory.cpp:68
> > +#elif PLATFORM(ANDROID)
> > +    mimeTypes.set("application/x-webarchive-xml", archiveFactoryCreate<WebArchiveAndroid>);
> Why the wacky MIME type?  Something more sensical would be application/x-webarchive+xml.  Even more sensical would be to use the existing webarchive MIME type.

We chose a different mime type because the format we used doesn't make use of the xml plist format, and uses custom xml instead. If you still think that using the same mimetype is warranted (even though the formats really aren't compatible), then I can make that change as suggested.

> > WebCore/loader/archive/android/WebArchiveAndroid.cpp:26
> > +#define LOG_TAG "webarchive"
> What is this?

That's Android's logging, will remove.

Thanks for the comments!

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 63010 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=63010&action=prettypatch
> > 
> > > WebCore/loader/archive/ArchiveFactory.cpp:68
> > > +#elif PLATFORM(ANDROID)
> > > +    mimeTypes.set("application/x-webarchive-xml", archiveFactoryCreate<WebArchiveAndroid>);
> > Why the wacky MIME type?  Something more sensical would be application/x-webarchive+xml.  Even more sensical would be to use the existing webarchive MIME type.
> 
> We chose a different mime type because the format we used doesn't make use of the xml plist format, and uses custom xml instead. If you still think that using the same mimetype is warranted (even though the formats really aren't compatible), then I can make that change as suggested.

Did you consider using the MHTML format instead?  Creating a variant of Apple's WebArchive format is a rather interesting choice.

I suppose this format has been shipping for a while in Android?  Is it used for anything beyond debugging?

[Eric Seidel (no email)]

Wow, I'm surprised Android would want to add a new format.

[Adam Barth]

Comment on attachment 63010 [details]
Android WebArchive patch v3.

I'm marking this r- for now.  I'm not necessarily opposed to the patch, I'd like to to understand more of the why.

[Eric Seidel (no email)]

Android Chromium is the new future.