Bug 39117

Summary:

Finish up IndexedDB events

Product:

WebKit

Reporter:

Jeremy Orlow <jorlow>

Component:

New Bugs

Assignee:

Jeremy Orlow <jorlow>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

japhet

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	japhet: review+

Jeremy Orlow

Reported 2010-05-14 08:18:53 PDT

Finish up IndexedDB events

Attachments
Patch (89.69 KB, patch) 2010-05-14 08:31 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Patch (90.20 KB, patch) 2010-05-14 08:55 PDT, Jeremy Orlow	japhet: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Jeremy Orlow

Comment 1 2010-05-14 08:20:21 PDT

Nate, I'm not sure, but I think you might be a good reviewer for this.

Jeremy Orlow

Comment 2 2010-05-14 08:31:29 PDT

Created attachment 56072 [details] Patch

Jeremy Orlow

Comment 3 2010-05-14 08:55:41 PDT

Created attachment 56074 [details] Patch

Nate Chapin

Comment 4 2010-05-14 09:38:00 PDT

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?

Jeremy Orlow

Comment 5 2010-05-14 09:43:04 PDT

(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. :-)

Jeremy Orlow

Comment 6 2010-05-14 09:45:38 PDT

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. :-)

Nate Chapin

Comment 7 2010-05-14 09:51:07 PDT

(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.

Nate Chapin

Comment 8 2010-05-14 09:51:29 PDT

Comment on attachment 56074 [details] Patch Switching to r+ based on previous comments.

Jeremy Orlow

Comment 9 2010-05-17 04:21:05 PDT

Committed r59602: <http://trac.webkit.org/changeset/59602>

Note You need to log in before you can comment on or make changes to this bug.