Bug 153323 – Modern IDB: Add -private.html variants of crypto/subtle IndexedDB tests

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: Add -private.html variants of crypto/subtle IndexedDB tests

There seem to only be two such tests

[Brady Eidson]

Created attachment 270555 [details]
Patch v1

No additional changes needed. Confirmed via testing that the -private.html versions are using the in-memory backing store, and are also passing.

[Alexey Proskuryakov]

Comment on attachment 270555 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=270555&action=review

> LayoutTests/resources/js-test-pre.js:5
> +    // If the test file URL ends in "-private.html", enable private browsing.

I would really really prefer if we didn't add more magic paths and filenames.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 270555 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270555&action=review
> 
> > LayoutTests/resources/js-test-pre.js:5
> > +    // If the test file URL ends in "-private.html", enable private browsing.
> 
> I would really really prefer if we didn't add more magic paths and filenames.

Already established practice at this point; js-test.js already does this.

Instead of making the change to js-test-pre.js, I could just make these tests include js-test.js also.

It seems more undesirable to me to have the source of the two copies of the test *not* be identical.

Do you have any other suggestions?

[Alexey Proskuryakov]

> Already established practice at this point

Yes, and I'd like to reduce that, not add more of such abuse.

> It seems more undesirable to me to have the source of the two copies of the test *not* be identical.

Why? These copies test different things, so the more apparent the difference is the better. Putting it directly into .html is vastly preferable to hiding it inside a helper script.

[Brady Eidson]

(In reply to comment #4)
> > Already established practice at this point
> 
> Yes, and I'd like to reduce that, not add more of such abuse.

I don't even mean "magic filenames."  I mean *this specific* magic filename is *already* in wide use.

> > It seems more undesirable to me to have the source of the two copies of the test *not* be identical.
> 
> Why? 

Because the tests are expected to be identical.

> These copies test different things,

They copy identical JS APIs.

The fact that the storage backend is different is irrelevant to the text of the test.

> Putting it directly into .html is vastly preferable to hiding
> it inside a helper script.

This is a statement of opinion presented as fact. I happen to disagree.

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #4)
> 
> > These copies test different things,
> 
> They copy identical JS APIs.
>

s/copy/test/

[Alexey Proskuryakov]

> I don't even mean "magic filenames."  I mean *this specific* magic filename is *already* in wide use.

That is unfortunate, and this patch makes it even more unfortunate. Let's undo what has been done, instead of continuing the trend.

> This is a statement of opinion presented as fact. I happen to disagree.

Please feel free to take the discussion to webkit-dev. I strongly believe that this is the wrong approach.

[Alexey Proskuryakov]

The logic seems super straightforward: anyone who will want to find what the difference between these tests is will do a diff, and will be super confused. We should avoid confusing WebKit engineers.

[Brady Eidson]

(In reply to comment #7)
> > This is a statement of opinion presented as fact. I happen to disagree.
> 
> Please feel free to take the discussion to webkit-dev. I strongly believe
> that this is the wrong approach.

I strongly believe that it is less wrong than the suggested alternative, but I also don't need to resolve this right now, so I'll punt until a later date.

[Brady Eidson]

Created attachment 280516 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 280516 [details]
Patch

Clearing flags on attachment: 280516

Committed r201685: <http://trac.webkit.org/changeset/201685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.