WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/9495321
>
Jay Civelli
Comment 4
2011-05-27 18:26:23 PDT
Created
attachment 95240
[details]
Patch
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
Created
attachment 95313
[details]
Patch
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
Created
attachment 95469
[details]
Patch
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
Created
attachment 95512
[details]
Patch
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
Created
attachment 95519
[details]
Patch
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
Created
attachment 95633
[details]
Patch
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
Created
attachment 95673
[details]
Patch
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
Created
attachment 95687
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug