Bug 202715 – Partially undo r250811

• Status: NEW
• Component: WebCore Misc.

[Jiewen Tan]

Partially undo r250811.

[Jiewen Tan]

<rdar://problem/56084287>

[Jiewen Tan]

Created attachment 380489 [details]
Patch

[Chris Dumez]

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 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 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 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]

Created attachment 380493 [details]
Patch

[Chris Dumez]

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 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 on attachment 380493 [details]
Patch

Please fix changelog before landing.

[Jiewen Tan]

Created attachment 380498 [details]
Patch for landing

[Jiewen Tan]

(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 on attachment 380498 [details]
Patch for landing

Clearing flags on attachment: 380498

Committed r250887: <https://trac.webkit.org/changeset/250887>