IndexedDB layout test refactor: factor prefix-handling-code
Created attachment 130945 [details] Patch
Created attachment 131433 [details] Patch
Created attachment 131439 [details] Patch
Created attachment 131444 [details] Patch
Created attachment 131467 [details] Patch
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
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 && ...)"); ?
Created attachment 131701 [details] Patch
(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?
(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.
(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?
(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!
Tony, could you review this? Sorry about the huge patch size. I looked at every new baseline.
Comment on attachment 131701 [details] Patch rs=me
Comment on attachment 131701 [details] Patch Clearing flags on attachment: 131701 Committed r110750: <http://trac.webkit.org/changeset/110750>
All reviewed patches have been landed. Closing bug.