Finish up IndexedDB events
Nate, I'm not sure, but I think you might be a good reviewer for this.
Created attachment 56072 [details] Patch
Created attachment 56074 [details] Patch
Comment on attachment 56074 [details] Patch No Webkit/chromium/ChangeLog entry. Rest of comments inline. On the whole, looks good. > + Disable it all all !Chromium platforms since none of them compile it Typo. > diff --git a/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html b/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html > new file mode 100644 > index 0000000000000000000000000000000000000000..239a7940debd7857873574f0a09a35880735ac19 > --- /dev/null > +++ b/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html > @@ -0,0 +1,12 @@ > +<html> > +<head> > +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css"> > +<script src="../../fast/js/resources/js-test-pre.js"></script> > +<script src="../../fast/js/resources/js-test-post-function.js"></script> > +</head> > +<body> > +<p id="description"></p> > +<div id="console"></div> > +<script src="YOUR_JS_FILE_HERE"></script> > +</body> > +</html> Do we need to propagate the TEMPLATE? > +protected: > + // Do not directly instantiate. > + IDBEvent(const AtomicString& type, PassRefPtr<IDBAny> source); Nit: this comment is redundant with the fact that the constructor is protected. > - PassRefPtr<IndexedDatabase> m_indexedDatabase; > + RefPtr<IndexedDatabase> m_indexedDatabase; > + RefPtr<IDBAny> m_this; A variable name of m_this rubs me the wrong way. m_source or m_request or something?
(In reply to comment #4) > (From update of attachment 56074 [details]) > No Webkit/chromium/ChangeLog entry. > > > Rest of comments inline. On the whole, looks good. > > > > + Disable it all all !Chromium platforms since none of them compile it > > Typo. Will fix. > > diff --git a/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html b/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html > > new file mode 100644 > > index 0000000000000000000000000000000000000000..239a7940debd7857873574f0a09a35880735ac19 > > --- /dev/null > > +++ b/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html > > @@ -0,0 +1,12 @@ > > +<html> > > +<head> > > +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css"> > > +<script src="../../fast/js/resources/js-test-pre.js"></script> > > +<script src="../../fast/js/resources/js-test-post-function.js"></script> > > +</head> > > +<body> > > +<p id="description"></p> > > +<div id="console"></div> > > +<script src="YOUR_JS_FILE_HERE"></script> > > +</body> > > +</html> > > Do we need to propagate the TEMPLATE? This is how script-tests work. > > +protected: > > + // Do not directly instantiate. > > + IDBEvent(const AtomicString& type, PassRefPtr<IDBAny> source); > > Nit: this comment is redundant with the fact that the constructor is protected. Will do. > > - PassRefPtr<IndexedDatabase> m_indexedDatabase; > > + RefPtr<IndexedDatabase> m_indexedDatabase; > > + RefPtr<IDBAny> m_this; > > A variable name of m_this rubs me the wrong way. m_source or m_request or something? Neither of those names are correct tho. It actually is a wrapper to "this" object. I do this rather than allocating an object on every request. (Thus letting refPtrs do the stuff they're good at. :-)
As for the WebKit/chromium change (with no ChangeLog): that wasn't supposed to be there. Will make sure it's not present when committing. Promise. :-)
(In reply to comment #5) > (In reply to comment #4) > > > diff --git a/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html b/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..239a7940debd7857873574f0a09a35880735ac19 > > > --- /dev/null > > > +++ b/LayoutTests/storage/indexeddb/script-tests/TEMPLATE.html > > > @@ -0,0 +1,12 @@ > > > +<html> > > > +<head> > > > +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css"> > > > +<script src="../../fast/js/resources/js-test-pre.js"></script> > > > +<script src="../../fast/js/resources/js-test-post-function.js"></script> > > > +</head> > > > +<body> > > > +<p id="description"></p> > > > +<div id="console"></div> > > > +<script src="YOUR_JS_FILE_HERE"></script> > > > +</body> > > > +</html> > > > > Do we need to propagate the TEMPLATE? > > This is how script-tests work. Ok, I did some more looking, and I hadn't realized that that file is in every script-tests directory. It just seemed strange at first. > > > - PassRefPtr<IndexedDatabase> m_indexedDatabase; > > > + RefPtr<IndexedDatabase> m_indexedDatabase; > > > + RefPtr<IDBAny> m_this; > > > > A variable name of m_this rubs me the wrong way. m_source or m_request or something? > > Neither of those names are correct tho. It actually is a wrapper to "this" object. I do this rather than allocating an object on every request. (Thus letting refPtrs do the stuff they're good at. :-) Yeah, I totally misread the constructor. On second reading, I'm much more ok with it, though it is a bit counter-intuitive. (In reply to comment #6) > As for the WebKit/chromium change (with no ChangeLog): that wasn't supposed to be there. Will make sure it's not present when committing. Promise. :-) This was the primary reason I didn't r+ with nits. If this is getting reverted, I think I'm ok with approving.
Comment on attachment 56074 [details] Patch Switching to r+ based on previous comments.
Committed r59602: <http://trac.webkit.org/changeset/59602>