IndexedDB layout tests: factor out prefix-handling-code

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB layout test refactor: factor prefix-handling-code

[David Grogan]

Created attachment 130945 [details]
Patch

[David Grogan]

Created attachment 131433 [details]
Patch

[David Grogan]

Created attachment 131439 [details]
Patch

[David Grogan]

Created attachment 131444 [details]
Patch

[David Grogan]

Created attachment 131467 [details]
Patch

[David Grogan]

Josh, could you look at this?

The prefix lines were moved to a function, dealWithPrefixes, in shared.js that each test now calls.

With apologies to any non-googlers who might be reading this, it might be easier to look at the expectation changes here:
http://emmy/chrome/layout-test-results/results.html

[Joshua Bell]

Comment on attachment 131467 [details]
Patch

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

> LayoutTests/ChangeLog:15
> +        * storage/indexeddb/basics-workers-expected.txt:

Was basics-shared-workers-expected.txt missed?

> LayoutTests/ChangeLog:54
> +        * storage/indexeddb/database-quota.html:

Looks like database-quota-expected.txt wasn't updated?

> LayoutTests/storage/indexeddb/resources/shared.js:27
> +function dealWithPrefixes()

I'm not thrilled with the name... how about "removeVendorPrefixes" ?

> LayoutTests/storage/indexeddb/resources/shared.js:41
> +    shouldBeFalse("indexedDB == null");

Some of the tests had shouldBeFalse("IDBTransaction == null") removed; maybe add shouldBeTrue("Boolean(indexedDB && IDBCursor && IDBDatabase && ...)"); ?

[David Grogan]

Created attachment 131701 [details]
Patch

[David Grogan]

(In reply to comment #7)
> (From update of attachment 131467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131467&action=review
> 
> > LayoutTests/ChangeLog:15
> > +        * storage/indexeddb/basics-workers-expected.txt:
> 
> Was basics-shared-workers-expected.txt missed?
> 
> > LayoutTests/ChangeLog:54
> > +        * storage/indexeddb/database-quota.html:
> 
> Looks like database-quota-expected.txt wasn't updated?

Ah yes, these two were missed because new-run-webkit-tests doesn't run them.  Updated.

> 
> > LayoutTests/storage/indexeddb/resources/shared.js:27
> > +function dealWithPrefixes()
> 
> I'm not thrilled with the name... how about "removeVendorPrefixes" ?

Thanks, I meant to revisit that.  removeVendorPrefixes works.

> 
> > LayoutTests/storage/indexeddb/resources/shared.js:41
> > +    shouldBeFalse("indexedDB == null");
> 
> Some of the tests had shouldBeFalse("IDBTransaction == null") removed; maybe add shouldBeTrue("Boolean(indexedDB && IDBCursor && IDBDatabase && ...)"); ?

Changed.

But why does the single Boolean function call work?  I expected to have to add that function call to each object that was incompatible with &&.  It's as if && accepts more types if it knows it's going to be passed to Boolean.  What's going on here that I'm misunderstanding?

[Joshua Bell]

(In reply to comment #9)
> > 
> > Some of the tests had shouldBeFalse("IDBTransaction == null") removed; maybe add shouldBeTrue("Boolean(indexedDB && IDBCursor && IDBDatabase && ...)"); ?
> 
> Changed.
> 
> But why does the single Boolean function call work?  I expected to have to add that function call to each object that was incompatible with &&.  It's as if && accepts more types if it knows it's going to be passed to Boolean.  What's going on here that I'm misunderstanding?

(x && y) will return x (if x is falsy) or y. It doesn't coerce the result to an actual boolean like the comparison operators do.

The Boolean(expr) is just so that what pops out is actually a JS boolean value (true or false), rather than just the last item in the (a && b && c) chain. Boolean(expr) does the same thing as !!expr. !!(a && b && c ...) would work just as well.

[David Grogan]

(In reply to comment #10)
> (In reply to comment #9)
Thanks for the explanation, I get it now.

Do you have any more comments on this patch or should I send it off to tony?

[Joshua Bell]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> Thanks for the explanation, I get it now.
> 
> Do you have any more comments on this patch or should I send it off to tony?

Nope, lgtm!

[David Grogan]

Tony, could you review this?  Sorry about the huge patch size.

I looked at every new baseline.

[Tony Chang]

Comment on attachment 131701 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 131701 [details]
Patch

Clearing flags on attachment: 131701

Committed r110750: <http://trac.webkit.org/changeset/110750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.