Bug 202715

Summary:

Partially undo r250811

Product:

WebKit

Reporter:

Jiewen Tan <jiewen_tan>

Component:

WebCore Misc.

Assignee:

Jiewen Tan <jiewen_tan>

Status:

NEW

Severity:

Normal

CC:

alecflett, beidson, bfulgham, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, jiewen_tan, jsbell, kangil.han, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Attachments:

Description	Flags
Patch	none
Patch	cdumez: review+
Patch for landing	none

Jiewen Tan

Reported 2019-10-08 18:12:22 PDT

Partially undo r250811.

Attachments
Patch (18.56 KB, patch) 2019-10-08 18:27 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (17.34 KB, patch) 2019-10-08 19:22 PDT, Jiewen Tan	cdumez: review+	Details Formatted Diff Diff
Patch for landing (17.32 KB, patch) 2019-10-08 20:30 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Jiewen Tan

Comment 1 2019-10-08 18:12:36 PDT

<rdar://problem/56084287>

Jiewen Tan

Comment 2 2019-10-08 18:27:12 PDT

Created attachment 380489 [details] Patch

Chris Dumez

Comment 3 2019-10-08 18:37:42 PDT

Comment on attachment 380489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380489&action=review > Source/WebCore/ChangeLog:12 > + the flag. Therefore, there is no ways to separate the serialization process into two and ways -> way > Source/WebCore/ChangeLog:13 > + this patch restores the old and crappy behaviour. However, the hardening part of r250811 crappy? Tell us what you really think. > Source/WebCore/ChangeLog:16 > + Covered by existing tests. Does not look like it since this test adds no new test. You are reverting part of your change because it caused a regression, yet, you fail to introduce any regression test. As a result, we may make the same mistake again in the future without noticing. Given how bad the regression was, I think a regression test is important.

Jiewen Tan

Comment 4 2019-10-08 19:04:11 PDT

Comment on attachment 380489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380489&action=review >> Source/WebCore/ChangeLog:12 >> + the flag. Therefore, there is no ways to separate the serialization process into two and > > ways -> way Fixed. >> Source/WebCore/ChangeLog:13 >> + this patch restores the old and crappy behaviour. However, the hardening part of r250811 > > crappy? Tell us what you really think. Certainly not something can be typed into ChangeLog. >> Source/WebCore/ChangeLog:16 >> + Covered by existing tests. > > Does not look like it since this test adds no new test. You are reverting part of your change because it caused a regression, yet, you fail to introduce any regression test. As a result, we may make the same mistake again in the future without noticing. > Given how bad the regression was, I think a regression test is important. Well, I change an existing test to put an { key: key } object into idb. Will enhance the test a bit to make it more clear.

Chris Dumez

Comment 5 2019-10-08 19:07:51 PDT

Comment on attachment 380489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380489&action=review >>> Source/WebCore/ChangeLog:16 >>> + Covered by existing tests. >> >> Does not look like it since this test adds no new test. You are reverting part of your change because it caused a regression, yet, you fail to introduce any regression test. As a result, we may make the same mistake again in the future without noticing. >> Given how bad the regression was, I think a regression test is important. > > Well, I change an existing test to put an { key: key } object into idb. Will enhance the test a bit to make it more clear. Oh, I missed that you updated a test. In this case, this should say “No new test, updated existing test”.

Jiewen Tan

Comment 6 2019-10-08 19:15:24 PDT

Comment on attachment 380489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380489&action=review >>>> Source/WebCore/ChangeLog:16 >>>> + Covered by existing tests. >>> >>> Does not look like it since this test adds no new test. You are reverting part of your change because it caused a regression, yet, you fail to introduce any regression test. As a result, we may make the same mistake again in the future without noticing. >>> Given how bad the regression was, I think a regression test is important. >> >> Well, I change an existing test to put an { key: key } object into idb. Will enhance the test a bit to make it more clear. > > Oh, I missed that you updated a test. In this case, this should say “No new test, updated existing test”. Fixed.

Jiewen Tan

Comment 7 2019-10-08 19:22:05 PDT

Created attachment 380493 [details] Patch

Chris Dumez

Comment 8 2019-10-08 19:29:27 PDT

Comment on attachment 380493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380493&action=review > Source/WebCore/ChangeLog:13 > + this patch restores the old and crappy behaviour. However, the hardening part of r250811 Well, this still says crappy :(

Jiewen Tan

Comment 9 2019-10-08 19:37:15 PDT

Comment on attachment 380493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380493&action=review >> Source/WebCore/ChangeLog:13 >> + this patch restores the old and crappy behaviour. However, the hardening part of r250811 > > Well, this still says crappy :( I guess just old then.

Chris Dumez

Comment 10 2019-10-08 19:45:00 PDT

Comment on attachment 380493 [details] Patch Please fix changelog before landing.

Jiewen Tan

Comment 11 2019-10-08 20:30:13 PDT

Created attachment 380498 [details] Patch for landing

Jiewen Tan

Comment 12 2019-10-08 20:30:29 PDT

(In reply to Chris Dumez from comment #10) > Comment on attachment 380493 [details] > Patch > > Please fix changelog before landing. Thanks, Chris.

WebKit Commit Bot

Comment 13 2019-10-08 21:15:36 PDT

Comment on attachment 380498 [details] Patch for landing Clearing flags on attachment: 380498 Committed r250887: <https://trac.webkit.org/changeset/250887>

Note You need to log in before you can comment on or make changes to this bug.