Bug 202715 - Partially undo r250811
Summary: Partially undo r250811
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-08 18:12 PDT by Jiewen Tan
Modified: 2019-10-08 21:15 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-10-08 18:12:22 PDT
Partially undo r250811.
Comment 1 Jiewen Tan 2019-10-08 18:12:36 PDT
<rdar://problem/56084287>
Comment 2 Jiewen Tan 2019-10-08 18:27:12 PDT
Created attachment 380489 [details]
Patch
Comment 3 Chris Dumez 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.
Comment 4 Jiewen Tan 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.
Comment 5 Chris Dumez 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”.
Comment 6 Jiewen Tan 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.
Comment 7 Jiewen Tan 2019-10-08 19:22:05 PDT
Created attachment 380493 [details]
Patch
Comment 8 Chris Dumez 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 :(
Comment 9 Jiewen Tan 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.
Comment 10 Chris Dumez 2019-10-08 19:45:00 PDT
Comment on attachment 380493 [details]
Patch

Please fix changelog before landing.
Comment 11 Jiewen Tan 2019-10-08 20:30:13 PDT
Created attachment 380498 [details]
Patch for landing
Comment 12 Jiewen Tan 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.
Comment 13 WebKit Commit Bot 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>