RESOLVED FIXED 7169
Support exporting of MHTML web archives
https://bugs.webkit.org/show_bug.cgi?id=7169
Summary Support exporting of MHTML web archives
David Kilzer (:ddkilzer)
Reported 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
Attachments
Patch (14.50 KB, patch)
2011-05-27 18:26 PDT, Jay Civelli
no flags
Patch (14.29 KB, patch)
2011-05-29 18:11 PDT, Jay Civelli
no flags
Patch (17.18 KB, patch)
2011-05-31 13:22 PDT, Jay Civelli
no flags
Patch (17.77 KB, patch)
2011-05-31 17:00 PDT, Jay Civelli
no flags
Patch (17.67 KB, patch)
2011-05-31 17:35 PDT, Jay Civelli
no flags
Patch (17.92 KB, patch)
2011-06-01 11:24 PDT, Jay Civelli
no flags
Patch (18.56 KB, patch)
2011-06-01 15:19 PDT, Jay Civelli
no flags
Patch (17.60 KB, patch)
2011-06-01 16:55 PDT, Jay Civelli
no flags
David Kilzer (:ddkilzer)
Comment 1 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>
Marco Barisione
Comment 2 2008-09-02 10:44:05 PDT
My patch for bug #7168 handle both loading and exporting MHTML files.
Adam Roben (:aroben)
Comment 3 2011-05-24 12:52:22 PDT
Jay Civelli
Comment 4 2011-05-27 18:26:23 PDT
Jay Civelli
Comment 5 2011-05-27 18:26:56 PDT
Comment on attachment 95240 [details] Patch Just an initial patch, not ready for review quite yet.
Jay Civelli
Comment 6 2011-05-29 18:11:56 PDT
WebKit Review Bot
Comment 7 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
Adam Barth
Comment 8 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.
Alexey Proskuryakov
Comment 9 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?
Jay Civelli
Comment 10 2011-05-31 13:22:48 PDT
Jay Civelli
Comment 11 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.
WebKit Review Bot
Comment 12 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
Adam Barth
Comment 13 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.
Alexey Proskuryakov
Comment 14 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.
Alexey Proskuryakov
Comment 15 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.
Adam Barth
Comment 16 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.
Jay Civelli
Comment 17 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.
Jay Civelli
Comment 18 2011-05-31 17:00:37 PDT
Adam Barth
Comment 19 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?
Alexey Proskuryakov
Comment 20 2011-05-31 17:21:29 PDT
I don't have further comments.
Jay Civelli
Comment 21 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.
Jay Civelli
Comment 22 2011-05-31 17:35:26 PDT
Adam Barth
Comment 23 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.
WebKit Commit Bot
Comment 24 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>
WebKit Commit Bot
Comment 25 2011-06-01 02:18:14 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 26 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.
Jay Civelli
Comment 27 2011-06-01 11:24:56 PDT
Jay Civelli
Comment 28 2011-06-01 11:26:09 PDT
Fixed the Windows bustage.
Adam Barth
Comment 29 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?
Jay Civelli
Comment 30 2011-06-01 15:19:49 PDT
Jay Civelli
Comment 31 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.
Adam Barth
Comment 32 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.
Alexey Proskuryakov
Comment 33 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.
Jay Civelli
Comment 34 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.
Jay Civelli
Comment 35 2011-06-01 16:55:19 PDT
WebKit Commit Bot
Comment 36 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>
WebKit Commit Bot
Comment 37 2011-06-02 14:59:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.