Bug 153323 - Modern IDB: Add -private.html variants of crypto/subtle IndexedDB tests
Summary: Modern IDB: Add -private.html variants of crypto/subtle IndexedDB tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117
  Show dependency treegraph
 
Reported: 2016-01-21 14:28 PST by Brady Eidson
Modified: 2016-06-04 13:58 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (11.70 KB, patch)
2016-02-02 23:00 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (20.71 KB, patch)
2016-06-04 10:10 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-01-21 14:28:09 PST
Modern IDB: Add -private.html variants of crypto/subtle IndexedDB tests

There seem to only be two such tests
Comment 1 Brady Eidson 2016-02-02 23:00:19 PST
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.
Comment 2 Alexey Proskuryakov 2016-02-02 23:18:59 PST
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.
Comment 3 Brady Eidson 2016-02-03 08:46:55 PST
(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?
Comment 4 Alexey Proskuryakov 2016-02-03 08:56:37 PST
> 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.
Comment 5 Brady Eidson 2016-02-03 10:17:06 PST
(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.
Comment 6 Brady Eidson 2016-02-03 10:17:25 PST
(In reply to comment #5)
> (In reply to comment #4)
> 
> > These copies test different things,
> 
> They copy identical JS APIs.
>

s/copy/test/
Comment 7 Alexey Proskuryakov 2016-02-03 14:15:41 PST
> 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.
Comment 8 Alexey Proskuryakov 2016-02-03 14:16:49 PST
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.
Comment 9 Brady Eidson 2016-02-03 14:57:15 PST
(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.
Comment 10 Brady Eidson 2016-06-04 10:10:44 PDT
Created attachment 280516 [details]
Patch
Comment 11 WebKit Commit Bot 2016-06-04 13:58:34 PDT
Comment on attachment 280516 [details]
Patch

Clearing flags on attachment: 280516

Committed r201685: <http://trac.webkit.org/changeset/201685>
Comment 12 WebKit Commit Bot 2016-06-04 13:58:38 PDT
All reviewed patches have been landed.  Closing bug.