RESOLVED FIXED Bug 27180
Cleanup DOM Storage dependencies. 
https://bugs.webkit.org/show_bug.cgi?id=27180
Summary Cleanup DOM Storage dependencies.
Jeremy Orlow
Reported 2009-07-11 13:23:49 PDT
DOM Storage has some really bad dependencies. Most of these are simply because of destructors or constructors. There's also a lot of file included that could otherwise just be declared. Clean this stuff up. This will allow things like StorageAreaSync to take a StorageAreaImpl* (as it should) rather than a StorageArea* which previously weren't possible because the dependencies were such a tangled mess.
Attachments
patch 1 (13.68 KB, patch)
2009-07-11 13:47 PDT, Jeremy Orlow 		bfulgham: review-
DetailsFormatted DiffDiff
rev 2 (13.73 KB, patch)
2009-07-12 07:22 PDT, Jeremy Orlow 		bfulgham: review-
DetailsFormatted DiffDiff
Windows build failure output. (22.46 KB, application/x-webarchive)
2009-07-14 13:42 PDT, Brent Fulgham 		no flags
Details
rev 3 (13.77 KB, patch)
2009-07-14 19:34 PDT, Jeremy Orlow 		fishd: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jeremy Orlow
Comment 1 2009-07-11 13:47:08 PDT
Created attachment 32617 [details] patch 1
Darin Adler
Comment 2 2009-07-11 15:11:42 PDT
Comment on attachment 32617 [details] patch 1 r=me Having the reference counter be a friend class and a private destructor does seem slightly tighter than our usual reference counting idiom, which is good. If it was me I wouldn't call these "really bad dependencies" or say "such a tangled mess" -- I would intend say something factual about how many extra includes there are and let the reader decide how bad it was.
Jeremy Orlow
Comment 3 2009-07-12 07:22:29 PDT
Created attachment 32625 [details] rev 2 Cleaned up the ChangeLog wording. Ready to be landed, I think.
David Kilzer (:ddkilzer)
Comment 4 2009-07-14 09:25:31 PDT
Comment on attachment 32625 [details] rev 2 Yes, this may be landed. Technically, the lander should use Darin Adler's r+, not mine. I'm just confirming the ChangeLog update. :)
Darin Adler
Comment 5 2009-07-14 09:39:05 PDT
Comment on attachment 32625 [details] rev 2 I'll set the review flag again just for clarity.
Brent Fulgham
Comment 6 2009-07-14 12:37:21 PDT
Landed in http://trac.webkit.org/changeset/45864.
Brent Fulgham
Comment 7 2009-07-14 13:41:30 PDT
This patch broke the Windows build due to Visual Studio complaints about the template friend declaration. Reverted in http://trac.webkit.org/changeset/45866. I'm attaching the error log for review.
Brent Fulgham
Comment 8 2009-07-14 13:42:07 PDT
Created attachment 32732 [details] Windows build failure output.
David Kilzer (:ddkilzer)
Comment 9 2009-07-14 15:14:45 PDT
Comment on attachment 32732 [details] Windows build failure output. See also: http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/2762/steps/compile-webkit/logs/stdio/text
Brent Fulgham
Comment 10 2009-07-14 16:10:12 PDT
Marking patches r- to remove from the application queue.
Jeremy Orlow
Comment 11 2009-07-14 19:34:09 PDT
Created attachment 32759 [details] rev 3 Removed the fancy private destructor stuff. It's not that important.
Jeremy Orlow
Comment 12 2009-07-14 20:35:07 PDT
I guess my original comment was overly terse. I'm very sorry the first patch had issues building on visual studio. This time I built it on windows and on mac, and everything seemed good. The only thing that changed is that I moved all the destructors back to being public and took out the friend statements. This patch should be ready to land. (Yes, famous last words. :-)
Darin Fisher (:fishd, Google)
Comment 13 2009-07-15 15:25:02 PDT
Comment on attachment 32759 [details] rev 3 r=me
Darin Fisher (:fishd, Google)
Comment 14 2009-07-15 15:56:42 PDT
Landed as http://trac.webkit.org/changeset/45954
Note You need to log in before you can comment on or make changes to this bug.