Bug 7169 - Support exporting of MHTML web archives
Summary: Support exporting of MHTML web archives
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P3 Enhancement
Assignee: Jay Civelli
URL: http://www.faqs.org/rfcs/rfc2557.html
Keywords: InRadar
Depends on: 61856
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-09 19:39 PST by David Kilzer (:ddkilzer)
Modified: 2011-06-02 14:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (14.50 KB, patch)
2011-05-27 18:26 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (14.29 KB, patch)
2011-05-29 18:11 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (17.18 KB, patch)
2011-05-31 13:22 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (17.77 KB, patch)
2011-05-31 17:00 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (17.67 KB, patch)
2011-05-31 17:35 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (17.92 KB, patch)
2011-06-01 11:24 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (18.56 KB, patch)
2011-06-01 15:19 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (17.60 KB, patch)
2011-06-01 16:55 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2006-02-09 19:39:45 PST
MSIE uses the MHTML format (based on RFC 2557) to save "web archives" of web pages.  It would be nice if WebKit supported exporting complete web pages in this format.

Note that because the MHTML format is essentially a mail message with a content-type of "multipart/related", a MIME encoder which supports quoted-printable and base64 encoding will be needed.  Mail.app probably contains usable encoding methods, but until they are moved to system libraries, a third-party library would have to be used or that wheel would have to be re-invented.  Pantomime is an example of such a third-party library.

http://www.collaboration-world.com/cgi-bin/project/index.cgi?pid=3
Comment 1 David Kilzer (:ddkilzer) 2006-02-09 19:57:35 PST
Created a Radar bug for a system-level APIs for MIME encoding and decoding in Mac OS X.
<rdar://problem/4440559>
Comment 2 Marco Barisione 2008-09-02 10:44:05 PDT
My patch for bug #7168 handle both loading and exporting MHTML files.
Comment 3 Adam Roben (:aroben) 2011-05-24 12:52:22 PDT
<rdar://problem/9495321>
Comment 4 Jay Civelli 2011-05-27 18:26:23 PDT
Created attachment 95240 [details]
Patch
Comment 5 Jay Civelli 2011-05-27 18:26:56 PDT
Comment on attachment 95240 [details]
Patch

Just an initial patch, not ready for review quite yet.
Comment 6 Jay Civelli 2011-05-29 18:11:56 PDT
Created attachment 95313 [details]
Patch
Comment 7 WebKit Review Bot 2011-05-29 18:29:45 PDT
Comment on attachment 95313 [details]
Patch

Attachment 95313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8741960
Comment 8 Adam Barth 2011-05-29 18:37:31 PDT
Comment on attachment 95313 [details]
Patch

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

Looks reasonable, but R- for the misplaced date code!

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:63
> +    md5.addBytes(reinterpret_cast<const uint8_t*>(&number), sizeof(double));

Yuck.  Why not cryptographicallyRandomValues ?  That will give you nicely distributed bytes of whatever length you like.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:65
> +    md5.checksum(md5Bytes);

What's the point of using MD5?  Just generate however many random bytes you want.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:87
> +static String currentDate()

This code really doesn't belong in this file.  Doesn't WTF have notions of date and time?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:158
> +PassRefPtr<SharedBuffer> MHTMLArchive::generateMHTMLData(const Frame& frame)

We usually pass Frame by pointer, not reference.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:170
> +    CString asciiTitle = frame.document()->title().ascii();

What if the title contains \r\n ?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:176
> +    stringBuilder.append("\ttype=\"text/html\";\r\n");

What if the frame contains something besides text/html, such as image/svg?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:181
> +    // We use utf8() below instead of ascii() as ascii() replaces CRLFs with ?? (we still only have put ASCII characters in it).

I see.  ascii() will protect the title.  That seems fragile.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:208
> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(*frame);

Notice the goofy *frame here.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:210
> +    // FIXME: we are copying all the data here. Idealy we would have a WebSharedData().
> +    return WebCString(mhtml->data(), mhtml->size());

Ouch.  That seems expensive.
Comment 9 Alexey Proskuryakov 2011-05-29 22:15:11 PDT
Comment on attachment 95313 [details]
Patch

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

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:56
> +static const char* const quotedPrintable = "quoted-printable";
> +static const char* const base64 = "base64";
> +
> +static const char* const daysOfWeek[] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" };
> +static const char* const monthsOfYear[] = { "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" };

There is no need for the leftmost "static" in C++.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:79
> +static String numberToSize2String(int number)

What' is a "Size2" string? I can't figure out what this function should be doing not only from the name, but even form the code.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:81
> +    number = abs(number);

Note that INT_MIN remains negative here.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:87
>> +static String currentDate()
> 
> This code really doesn't belong in this file.  Doesn't WTF have notions of date and time?

Getting the time and serializing it into a string (what's the standard specifying this particular serialization?) should be in separate functions (but why does MHTML saving take into account current time at all?)

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:158
>> +PassRefPtr<SharedBuffer> MHTMLArchive::generateMHTMLData(const Frame& frame)
> 
> We usually pass Frame by pointer, not reference.

And no const, ideally.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:170
>> +    CString asciiTitle = frame.document()->title().ascii();
> 
> What if the title contains \r\n ?

And what if it's not all ASCII??? But really, do we even need to write out From and Subject, and do we care what's in these header fields?
Comment 10 Jay Civelli 2011-05-31 13:22:48 PDT
Created attachment 95469 [details]
Patch
Comment 11 Jay Civelli 2011-05-31 13:30:42 PDT
Comment on attachment 95313 [details]
Patch

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

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:56
>> +static const char* const monthsOfYear[] = { "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" };
> 
> There is no need for the leftmost "static" in C++.

Removed.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:63
>> +    md5.addBytes(reinterpret_cast<const uint8_t*>(&number), sizeof(double));
> 
> Yuck.  Why not cryptographicallyRandomValues ?  That will give you nicely distributed bytes of whatever length you like.

Ah! That's what I was looking for.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:65
>> +    md5.checksum(md5Bytes);
> 
> What's the point of using MD5?  Just generate however many random bytes you want.

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:79
>> +static String numberToSize2String(int number)
> 
> What' is a "Size2" string? I can't figure out what this function should be doing not only from the name, but even form the code.

This function returns a string 2 characters long:
2 -> 02
10 -> 10
Renamed to something hopefully clearer.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:81
>> +    number = abs(number);
> 
> Note that INT_MIN remains negative here.

Input is expected to be positive. Added ASSERTs.

>>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:87
>>> +static String currentDate()
>> 
>> This code really doesn't belong in this file.  Doesn't WTF have notions of date and time?
> 
> Getting the time and serializing it into a string (what's the standard specifying this particular serialization?) should be in separate functions (but why does MHTML saving take into account current time at all?)

Added that with some non math related function to DateMath.h.
Made it take the time as input values and renamed the function to make clear it is RFC 2822 format.

>>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:158
>>> +PassRefPtr<SharedBuffer> MHTMLArchive::generateMHTMLData(const Frame& frame)
>> 
>> We usually pass Frame by pointer, not reference.
> 
> And no const, ideally.

Now Frame*.

>>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:170
>>> +    CString asciiTitle = frame.document()->title().ascii();
>> 
>> What if the title contains \r\n ?
> 
> And what if it's not all ASCII??? But really, do we even need to write out From and Subject, and do we care what's in these header fields?

Other browsers do specify a From, Subject and Date. I include it for consistency with their behavior.
In the case of non ASCII text in the title, I replace these characters with ?s (that's what IE does).

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:176
>> +    stringBuilder.append("\ttype=\"text/html\";\r\n");
> 
> What if the frame contains something besides text/html, such as image/svg?

This type here is not important (the one that matters is the one for each resource).
Added it to be the right type anyway for good measure.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:181
>> +    // We use utf8() below instead of ascii() as ascii() replaces CRLFs with ?? (we still only have put ASCII characters in it).
> 
> I see.  ascii() will protect the title.  That seems fragile.

OK, added a function to do that explicitly.

>> Source/WebKit/chromium/src/WebPageSerializer.cpp:210
>> +    return WebCString(mhtml->data(), mhtml->size());
> 
> Ouch.  That seems expensive.

I'll address that in a follow up patch more Chromium WebKit API specific if you don't mind.
Comment 12 WebKit Review Bot 2011-05-31 13:38:01 PDT
Comment on attachment 95469 [details]
Patch

Attachment 95469 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8753442
Comment 13 Adam Barth 2011-05-31 14:27:59 PDT
Comment on attachment 95469 [details]
Patch

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

This looks great.  A bunch of minor nits below.

> Source/JavaScriptCore/wtf/DateMath.cpp:1041
> +String toRFC2822DateString(int dayOfWeek, int day, int month, int year, int hours, int minutes, int seconds, int utcOffset)

Much nicer!  Thanks.

> Source/JavaScriptCore/wtf/DateMath.h:65
> +String toRFC2822DateString(int dayOfWeek, int day, int month, int year, int hours, int minutes, int seconds, int utcOffset);

Is it worth saying which of these are zero-based and which are one-based?  I remember that being fairly confusing in POSIX.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:59
> +    char randomValues[10];
> +    cryptographicallyRandomValues(&randomValues, 10);

I'd make the 10 a named constant (local to this function).

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:122
> +    pageSerializer.serialize(frame->page());

Should MHTMLArchive::generateMHTMLData take a Page rather than a Frame?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:133
> +          dateString = toRFC2822DateString(tm->tm_wday, tm->tm_mday, tm->tm_mon, 1900 + tm->tm_year, tm->tm_hour, tm->tm_min, tm->tm_sec, timeZone.tz_minuteswest);

bad indent

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:135
> +          LOG_ERROR("Failed to retrieve time.");

In what situations could these fail?  Should these have ASSERT_NOT_REACHED?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:156
> +    // We use utf8() below instead of ascii() as ascii() replaces CRLFs with ?? (we still only have put ASCII characters in it).

Should we assert that asciiString is all ASCII?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:169
> +        const char* contentEncoding = resource.mimeType.startsWith("text/") ? quotedPrintable : base64;

So application/xml will get base64, but text/html will get quotedPrintable.  Maybe that's ok...  There's probably a helper function somewhere that will tell you if a MIME type is XML.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:194
> +                size_t lineLength = std::min(encodedDataLength - index, static_cast<size_t>(76));

I'd store 76 in a named constant (which will also let you avoid the static_cast).

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:197
> +                index += 76;

... because 76 recurs here.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:208
> +    WebFrame* webFrame = static_cast<WebViewImpl*>(view)->mainFrame();
> +    Frame* frame = static_cast<WebFrameImpl*>(webFrame)->frame();
> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(frame);

Yeah, see you're using mainFrame here.  That's a sign that you should really be dealing with Pages.
Comment 14 Alexey Proskuryakov 2011-05-31 14:45:00 PDT
Comment on attachment 95469 [details]
Patch

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

> Source/JavaScriptCore/wtf/DateMath.cpp:183
> +static String numberTo2CharacterLongString(int number)

I'd suggest twoDigitStringFromNumber().

> Source/JavaScriptCore/wtf/DateMath.h:65
> +String toRFC2822DateString(int dayOfWeek, int day, int month, int year, int hours, int minutes, int seconds, int utcOffset);

I'd say "make", not "to".

Why are all the arguments int, not unsigned?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:129
> +    struct timeval timeValue;
> +    struct timezone timeZone;

No need for "struct" in C++.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:131
> +        struct tm* tm = gmtime(&(timeValue.tv_sec));

No need for "struct" in C++. Please use a full name for the variable itself.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:137
> +          LOG_ERROR("Failed to retrieve time.");
> +    } else
> +        LOG_ERROR("Failed to retrieve time and time zone.");

There is no early return, so the values should be initialized to avoid having random data in them.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:142
> +    stringBuilder.append(replaceNonPrintableCharacters(frame->document()->title()));

Please add a comment here, explaining that IE replaces these with question marks.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:161
> +    for (Vector<PageSerializer::Resource>::const_iterator iterator = resources.begin(); iterator != resources.end(); ++iterator) {

As a matter of coding style, we don't use iterators on vectors. Please just use an unsigned index.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:188
> +            ASSERT(contentEncoding == base64);

Although it's correct in this case, comparing C strings for pointer equality raises red flags, and is better to avoid.

> Source/WebCore/page/PageSerializer.cpp:212
> +        // FIXME: it seems iframe used as image trigger this. We should deal with them correctly.

If these (among) others trigger the assertion, there is no need to say that they "seem" to.
Comment 15 Alexey Proskuryakov 2011-05-31 14:52:56 PDT
When marking r-, I thought that I re-asserted Adam's r- that got clobbered, but now I see that he said r+. I still think that with so many comments, another quick review round is appropriate.
Comment 16 Adam Barth 2011-05-31 15:02:07 PDT
(In reply to comment #15)
> When marking r-, I thought that I re-asserted Adam's r- that got clobbered, but now I see that he said r+. I still think that with so many comments, another quick review round is appropriate.

Yeah, that's fine.
Comment 17 Jay Civelli 2011-05-31 16:58:55 PDT
Comment on attachment 95469 [details]
Patch

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

>> Source/JavaScriptCore/wtf/DateMath.cpp:183
>> +static String numberTo2CharacterLongString(int number)
> 
> I'd suggest twoDigitStringFromNumber().

Done.

>>> Source/JavaScriptCore/wtf/DateMath.h:65
>>> +String toRFC2822DateString(int dayOfWeek, int day, int month, int year, int hours, int minutes, int seconds, int utcOffset);
>> 
>> Is it worth saying which of these are zero-based and which are one-based?  I remember that being fairly confusing in POSIX.
> 
> I'd say "make", not "to".
> 
> Why are all the arguments int, not unsigned?

renamed to makeRFC2822DateString, added comment describing parameters' range, modified parameters to be unsigned (but for utfOffset which can be negative).

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:59
>> +    cryptographicallyRandomValues(&randomValues, 10);
> 
> I'd make the 10 a named constant (local to this function).

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:122
>> +    pageSerializer.serialize(frame->page());
> 
> Should MHTMLArchive::generateMHTMLData take a Page rather than a Frame?

Good idea, done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:129
>> +    struct timezone timeZone;
> 
> No need for "struct" in C++.

Removed.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:131
>> +        struct tm* tm = gmtime(&(timeValue.tv_sec));
> 
> No need for "struct" in C++. Please use a full name for the variable itself.

Removed struct, renamed variable.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:133
>> +          dateString = toRFC2822DateString(tm->tm_wday, tm->tm_mday, tm->tm_mon, 1900 + tm->tm_year, tm->tm_hour, tm->tm_min, tm->tm_sec, timeZone.tz_minuteswest);
> 
> bad indent

Fixed indent.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:135
>> +          LOG_ERROR("Failed to retrieve time.");
> 
> In what situations could these fail?  Should these have ASSERT_NOT_REACHED?

It's not exactly clear from the docs in which cases these would fail, but we should probably assert if they do.
Now asserting.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:137
>> +        LOG_ERROR("Failed to retrieve time and time zone.");
> 
> There is no early return, so the values should be initialized to avoid having random data in them.

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:142
>> +    stringBuilder.append(replaceNonPrintableCharacters(frame->document()->title()));
> 
> Please add a comment here, explaining that IE replaces these with question marks.

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:156
>> +    // We use utf8() below instead of ascii() as ascii() replaces CRLFs with ?? (we still only have put ASCII characters in it).
> 
> Should we assert that asciiString is all ASCII?

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:161
>> +    for (Vector<PageSerializer::Resource>::const_iterator iterator = resources.begin(); iterator != resources.end(); ++iterator) {
> 
> As a matter of coding style, we don't use iterators on vectors. Please just use an unsigned index.

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:169
>> +        const char* contentEncoding = resource.mimeType.startsWith("text/") ? quotedPrintable : base64;
> 
> So application/xml will get base64, but text/html will get quotedPrintable.  Maybe that's ok...  There's probably a helper function somewhere that will tell you if a MIME type is XML.

OK, now using MIMERegistry code. Scripts and non-image types now use QuotedPrintable.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:188
>> +            ASSERT(contentEncoding == base64);
> 
> Although it's correct in this case, comparing C strings for pointer equality raises red flags, and is better to avoid.

OK, replaced with strcmp.

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:194
>> +                size_t lineLength = std::min(encodedDataLength - index, static_cast<size_t>(76));
> 
> I'd store 76 in a named constant (which will also let you avoid the static_cast).

Done.

>> Source/WebCore/page/PageSerializer.cpp:212
>> +        // FIXME: it seems iframe used as image trigger this. We should deal with them correctly.
> 
> If these (among) others trigger the assertion, there is no need to say that they "seem" to.

Changed comment.
Comment 18 Jay Civelli 2011-05-31 17:00:37 PDT
Created attachment 95512 [details]
Patch
Comment 19 Adam Barth 2011-05-31 17:10:10 PDT
Comment on attachment 95512 [details]
Patch

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

Looks good to me, but give Alexey a chance to take another look.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:132
> +        tm* timeAndDate = gmtime(&(timeValue.tv_sec));

Sorry for the dumb question, but do we need to free timeAndDate?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:208
> +    WebFrame* webFrame = static_cast<WebViewImpl*>(view)->mainFrame();
> +    Frame* frame = static_cast<WebFrameImpl*>(webFrame)->frame();
> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(frame->page());

There's no way to get directly from a WebViewImpl to a Page?
Comment 20 Alexey Proskuryakov 2011-05-31 17:21:29 PDT
I don't have further comments.
Comment 21 Jay Civelli 2011-05-31 17:35:12 PDT
Comment on attachment 95512 [details]
Patch

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

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:132
>> +        tm* timeAndDate = gmtime(&(timeValue.tv_sec));
> 
> Sorry for the dumb question, but do we need to free timeAndDate?

Actually it is a pretty good question! :-)
I was using the non-reentrant version, which is bad.
Now using the reentrant version and the localtime instead of gmtime (as we want the local time, not the UTC time).

>> Source/WebKit/chromium/src/WebPageSerializer.cpp:208
>> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(frame->page());
> 
> There's no way to get directly from a WebViewImpl to a Page?

Ah, yes, there is. Thanks for pointing that out.
Comment 22 Jay Civelli 2011-05-31 17:35:26 PDT
Created attachment 95519 [details]
Patch
Comment 23 Adam Barth 2011-05-31 17:37:47 PDT
Comment on attachment 95519 [details]
Patch

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

> Source/WebKit/chromium/src/WebPageSerializer.cpp:206
> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(static_cast<WebViewImpl*>(view)->page());

Ah, that's much nicer.
Comment 24 WebKit Commit Bot 2011-06-01 02:18:07 PDT
Comment on attachment 95519 [details]
Patch

Clearing flags on attachment: 95519

Committed r87788: <http://trac.webkit.org/changeset/87788>
Comment 25 WebKit Commit Bot 2011-06-01 02:18:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Commit Bot 2011-06-01 03:47:31 PDT
The commit-queue encountered the following flaky tests while processing attachment 95519 [details]:

http/tests/inspector/console-websocket-error.html bug 57392 (authors: pfeldman@chromium.org and yutak@chromium.org)
The commit-queue is continuing to process your patch.
Comment 27 Jay Civelli 2011-06-01 11:24:56 PDT
Created attachment 95633 [details]
Patch
Comment 28 Jay Civelli 2011-06-01 11:26:09 PDT
Fixed the Windows bustage.
Comment 29 Adam Barth 2011-06-01 13:57:52 PDT
Comment on attachment 95633 [details]
Patch

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

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:144
> +#if OS(WINDOWS)
> +    __time64_t time;
> +    _time64(&time); 
> +    if (_localtime64_s(&timeAndDate, &time))
> +        ASSERT_NOT_REACHED();
> +#else
> +    timeval timeValue = { 0 };
> +    if (gettimeofday(&timeValue, 0) || !localtime_r(&(timeValue.tv_sec), &timeAndDate))
> +        ASSERT_NOT_REACHED();
> +#endif

We shouldn't have platform-specific code in this file.  Maybe move this stuff to WTF?  Are you sure there isn't already a way to do this in WTF?
Comment 30 Jay Civelli 2011-06-01 15:19:49 PDT
Created attachment 95673 [details]
Patch
Comment 31 Jay Civelli 2011-06-01 15:21:28 PDT
Comment on attachment 95633 [details]
Patch

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

>> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:144
>> +#endif
> 
> We shouldn't have platform-specific code in this file.  Maybe move this stuff to WTF?  Are you sure there isn't already a way to do this in WTF?

I could not find code that already does that in WTF.
I created a new method retrieveDateAndTime in DateMath.h to get that info.
Comment 32 Adam Barth 2011-06-01 15:26:58 PDT
Comment on attachment 95673 [details]
Patch

Great.  I'm surprised we don't have this already, but I believe you.
Comment 33 Alexey Proskuryakov 2011-06-01 15:34:04 PDT
Can't you just use functions from wtf/CurrentTime.h? Even if not, that's where the new function should go.
Comment 34 Jay Civelli 2011-06-01 16:47:44 PDT
(In reply to comment #33)
> Can't you just use functions from wtf/CurrentTime.h? Even if not, that's where the new function should go.

Thanks Alexey, I somehow missed CurrentTime.h (I was grepping for gettimeofday).
Everything I need is already in there.
New patch coming shortly.
Comment 35 Jay Civelli 2011-06-01 16:55:19 PDT
Created attachment 95687 [details]
Patch
Comment 36 WebKit Commit Bot 2011-06-02 14:59:42 PDT
Comment on attachment 95687 [details]
Patch

Clearing flags on attachment: 95687

Committed r87958: <http://trac.webkit.org/changeset/87958>
Comment 37 WebKit Commit Bot 2011-06-02 14:59:50 PDT
All reviewed patches have been landed.  Closing bug.