Summary: | Migrate createObjectURL & revokeObjectURL to static (Class) methods | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kaustubh Atrawalkar <kaustubh.ra> | ||||||||||||||||||||||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, allan.jensen, ap, arv, darin, dglazkov, dslomov, eric, haraken, jianli, kaustubh.ra, levin, ojan, ossy, webkit.review.bot | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
URL: | http://www.w3.org/TR/FileAPI/#creating-revoking | ||||||||||||||||||||||||||||||||||
Bug Depends on: | 77182, 77947 | ||||||||||||||||||||||||||||||||||
Bug Blocks: | 74385 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Kaustubh Atrawalkar
2011-12-13 00:31:08 PST
Created attachment 118981 [details]
Patch
Added DOMURL constructor as per new specs. Moved createObjectURL & revokeObjectURL to static methods.
Added test case for checking static property of above methods.
IDL attributes for DOMURL and test cases for constructor & attributes to be followed up in next patch. This is with reference to spec mentioned in the above url. Comment on attachment 118981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118981&action=review > Source/WebCore/ChangeLog:8 > + Test: fast/dom/DOMURL/check-instanceof-domurl-functions.html It is probably worth pointing out that there are existing tests that cover webkitURL. > Source/WebCore/html/DOMURL.cpp:179 > +DOMURL::DOMURL(const String& url, const String& baseURL) > +{ > + m_href = KURL(KURL(), url); > + // Resolve the URL relative to baseURL if is provide. > + if (!baseURL.isEmpty()) > + m_href = KURL(m_href, baseURL); > +} Can this be taken out of this patch? In this patch we don't want to be able to instantiate new URL objects. > Source/WebCore/html/DOMURL.h:59 > + KURL m_href; Not needed in this patch. > Source/WebCore/html/DOMURL.idl:-28 > - Conditional=BLOB, Keep conditional in this patch? > Source/WebCore/html/DOMURL.idl:32 > + Constructor(in DOMString url, in [Optional=CallWithNullValue] DOMString baseURL) For this patch I would remove the Constructor since at this point there is no need to be able to instantiate this. > Source/WebCore/page/DOMWindow.idl:817 > + attribute DOMURLConstructor webkitURL; Given this patch is only about moving these methods from instance methods to class methods it still makes sense to keep the conditional here and in WorkerContext.idl Created attachment 119180 [details]
Updated patch
Comment on attachment 119180 [details]
Updated patch
Changed the patch as per Erik's comments. Darin can you review once?
Created attachment 119183 [details]
Updated Patch
Created attachment 119207 [details]
Fixed ChangeLog
Comment on attachment 119207 [details] Fixed ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=119207&action=review I'm going to mark this R+, but there is one subtle issue (mentioned below) that you might want to consider before landing this patch. > Source/WebCore/html/DOMURL.cpp:88 > +static PublicURLManagerMap& publicURLManagerMap() Interesting design choice. Another option is to store the information in the ScriptExecutionContext as some sort of a mix-in. We don't have a facility for doing that at the moment, but there are a bunch of cases like this where it would be useful. > Source/WebCore/html/DOMURL.idl:34 > - [ConvertNullStringTo=Undefined] DOMString createObjectURL(in MediaStream stream); > + static [ConvertNullStringTo=Undefined,CallWith=ScriptExecutionContext] DOMString createObjectURL(in MediaStream stream); So, this is actually slightly different because you'll get a different ScriptExecution context. After this change, you get the script execution context associated with the currently executing script rather than the one associated with the window object that contains this object. Is that an intentional change? If so, should we add a test that demonstrates it? (In reply to comment #8) > (From update of attachment 119207 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119207&action=review > > I'm going to mark this R+, but there is one subtle issue (mentioned below) that you might want to consider before landing this patch. > > > Source/WebCore/html/DOMURL.cpp:88 > > +static PublicURLManagerMap& publicURLManagerMap() > > Interesting design choice. Another option is to store the information in the ScriptExecutionContext as some sort of a mix-in. We don't have a facility for doing that at the moment, but there are a bunch of cases like this where it would be useful. > > > Source/WebCore/html/DOMURL.idl:34 > > - [ConvertNullStringTo=Undefined] DOMString createObjectURL(in MediaStream stream); > > + static [ConvertNullStringTo=Undefined,CallWith=ScriptExecutionContext] DOMString createObjectURL(in MediaStream stream); > > So, this is actually slightly different because you'll get a different ScriptExecution context. After this change, you get the script execution context associated with the currently executing script rather than the one associated with the window object that contains this object. Is that an intentional change? If so, should we add a test that demonstrates it? Good catch. That is the wrong behavior. If you do "someWindow.URL.createObjectURL(...)" that URL should be associated with the ScriptExecution context of "someWindow". He's not a committer, so this might as well be an r- if we're not gonna set cq+ :) For V8 this looks like it it using the right context. The V8 bindings call getScriptExecutionContext which in turn calls V8Proxy::retrieveFrameForCurrentContext() which according to the comments is the frame of the context of the function. http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/V8Proxy.h&type=cs&l=184 I'm not sure I'm reading this correctly and a test would be very helpful. Created attachment 122206 [details]
Patch
First of all, apology for long delay in updating the patch. Fixed Test cases. Updated to current revision. added check for somewindow.webkitURL.createObjectURL & somewindow.webkitURL.revokeObjectURL.
Comment on attachment 122206 [details] Patch Attachment 122206 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11211380 New failing tests: fast/dom/DOMURL/check-instanceof-domurl-functions.html Comment on attachment 122206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122206&action=review > LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions-expected.txt:7 > +FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined). Can you fix the test so that it does not fail? > LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions.html:10 > +var url = new webkitURL(); At this point we do not care if webkitURL is constructable or not. This also throws so none of the other tests are reached. > LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions.html:16 > +shouldBeUndefined("url.createObjectURL"); > +shouldBeUndefined("url.revokeObjectURL"); Remove or comment out since webkitURL is not yet constructable. Created attachment 122880 [details]
Updated Patch
Fixed test case.
Comment on attachment 122880 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122880&action=review > LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions.html:13 > +//shouldBeTrue("'createObjectURL' in webkitURL"); > +//shouldBeTrue("'revokeObjectURL' in webkitURL"); This is confusing. I thought "new webkitURL" was throwing before and if it isn't then these 2 tests should be OK. Created attachment 123078 [details]
Updated Patch
I have no more issues with this. I'm not a reviewer and I did not pay attention to style issues. Maybe Adam or Darin can give you a real review. Comment on attachment 123078 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123078&action=review This looks good. A few minor questions/nits below. Thanks! > Source/WebCore/html/DOMURL.cpp:-28 > -#if ENABLE(BLOB) I'm curious why you moved this ifdef around. It looks like almost everything is ifdefed anyway. > Source/WebCore/html/DOMURL.cpp:59 > + virtual void contextDestroyed() Please add the OVERRIDE keyword. We're just starting to use that in WebKit. > Source/WebCore/html/DOMURL.h:41 > +class DOMURL : public RefCounted<DOMURL> { It looks like DOMURL is now declared outside of ENABLE(BLOB). Is that what you intend? (In reply to comment #19) Thanks for the review Adam. Please find my comments inline. > (From update of attachment 123078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123078&action=review > > This looks good. A few minor questions/nits below. Thanks! > > > Source/WebCore/html/DOMURL.cpp:-28 > > -#if ENABLE(BLOB) > > I'm curious why you moved this ifdef around. It looks like almost everything is ifdefed anyway. > This is partial implementation of the Meta bug - Implement URL API. Going forward other implementation wont be in #if ENABLE(BLOB) so tried to keep it that way. > > Source/WebCore/html/DOMURL.cpp:59 > > + virtual void contextDestroyed() > > Please add the OVERRIDE keyword. We're just starting to use that in WebKit. > Sure, will add it. > > Source/WebCore/html/DOMURL.h:41 > > +class DOMURL : public RefCounted<DOMURL> { > > It looks like DOMURL is now declared outside of ENABLE(BLOB). Is that what you intend? Yes, I suppose that's what new URL implementation is expecting. Correct me if I am wrong. Comment on attachment 123078 [details]
Updated Patch
Ok. We can add OVERRIDE next time we touch this file. It's not worth respinning the patch. Thanks!
Comment on attachment 123078 [details] Updated Patch Clearing flags on attachment: 123078 Committed r105486: <http://trac.webkit.org/changeset/105486> All reviewed patches have been landed. Closing bug. This broke these tests: fast/dom/prototype-inheritance-2.html fast/js/global-constructors.html (In reply to comment #24) > This broke these tests: > fast/dom/prototype-inheritance-2.html > fast/js/global-constructors.html Which platform did the tests failed? Working on chromium port + gtk port here on my machine. http://build.webkit.org/waterfall can answer these sorts of questions. It looks like Lion, at least. This patch looks like this patch will cause problems for usage from workers. Here's why: 1. It removed "NoStaticTables" from DOMURL.idl. Isn't that needed to allow this to be used with workers? 2. The methods publicURLManagerMap and publicURLManager aren't threadsafe so they has potential race conditions since it can be called on Web Worker threads as well as the main thread. On the plus side, I noticed this because it fixes a leak that existed in the code before this patch. (The leak would happen because ~DOMURL would get called before DOM::contextDestroyed got called and now it appears that contextDestroyed always gets called before ~PublicURLManager.) It may be good to roll this out while these are being addressed rather than leaves this in the tree and have potential random crashes due to it (unless they are addressed quickly). PS It also feels odd that DOMURL is the one calling registerBlobURL but PublicURLManager is the one responsible for calling unregisterBlobURL and how PublicURLManager basically just exposes m_blobURLs. (I think that PublicURLManager should hide the implementation detail that it has HashSets and just expose a method to register*URL which would address this PS). I'll wait a day or two before rolling out to see if there a fix being done quickly. Thanks Dave. I'm sorry I missed those issues in my review. I wasn't thinking about the worker case. (In reply to comment #29) > Thanks Dave. I'm sorry I missed those issues in my review. I wasn't thinking about the worker case. It might be that the NoStaticTables thing isn't needed due to all methods becoming static. I don't know. It feels like it may still be needed. (It would be nice to know it was done on purpose and why :) .) > It might be that the NoStaticTables thing isn't needed due to all methods becoming static. I don't know. It feels like it may still be needed. It's going to be needed once we let web site construct these objects. > (It would be nice to know it was done on purpose and why :) .) I just missed it. (In reply to comment #28) > I'll wait a day or two before rolling out to see if there a fix being done quickly. Thanks David, I am sorry I could not think of these issues. I saw your comment on Meta bug though about these functions not being thread safe. I will try to fix both NoStaticTables and threading issues ASAP. (In reply to comment #32) > (In reply to comment #28) > > I'll wait a day or two before rolling out to see if there a fix being done quickly. > > Thanks David, I am sorry I could not think of these issues. I saw your comment on Meta bug though about these functions not being thread safe. I will try to fix both NoStaticTables and threading issues ASAP. No worries. It is hard to see it every angle. We all make mistakes. (I just would rather them no be lingering in the code and making things flaky now that we know about them :) ). Anyway, I don't want you too feel too pressured. There is no problem with rolling out the change and putting up a fixed patch. Comment on attachment 123078 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123078&action=review > Source/WebCore/html/DOMURL.cpp:52 > +static PublicURLManagerMap& publicURLManagerMap(); I do not think having a global PubliURLManagerMap is a good idea. This map will be accessed from different thread, so access to it must made thread safe, and this introduces complexity. Better to make PublicURLManager a part of ScriptExecutionContext to ensure single-threaded access. Re-opening due to threadsafety issue. Actually the thread safety issues have been broken out in a separate bug (https://bugs.webkit.org/show_bug.cgi?id=77003), so re-resolving. (In reply to comment #28) > I'll wait a day or two before rolling out to see if there a fix being done quickly. Rolled out here: http://trac.webkit.org/changeset/106087 (In reply to comment #37) > (In reply to comment #28) > > I'll wait a day or two before rolling out to see if there a fix being done quickly. > > Rolled out here: http://trac.webkit.org/changeset/106087 I rolled out the Qt expected files update too - http://trac.webkit.org/changeset/106105 I think GTK update should be roll out too. Please land the Qt and GTK expected file changes with the proper fix. Thanks. Created attachment 124918 [details]
Work in progress patch
Trying to make it work as per Dimitry's suggestion. Can you check if I am going right? Please provide comments/suggestions.
Comment on attachment 124918 [details] Work in progress patch View in context: https://bugs.webkit.org/attachment.cgi?id=124918&action=review This appears to be on the right track now and should work with web workers. > Source/WebCore/dom/ScriptExecutionContext.cpp:130 > + if (m_publicURLManager) #if ENABLE(BLOB) > Source/WebCore/dom/ScriptExecutionContext.cpp:395 > + m_publicURLManager = adoptPtr(new PublicURLManager(this)); Should be PublicURLManager::create (which is a static method that does "return adoptPtr(new PublicURLManager(context));" > Source/WebCore/html/DOMURL.cpp:3 > + * Copyright (C) 2011 Motorola Mobility Inc. 2012 > Source/WebCore/html/DOMURL.cpp:-28 > -#if ENABLE(BLOB) Why not leave this if as is? > Source/WebCore/html/DOMURL.h:59 > +class PublicURLManager : public RefCounted<PublicURLManager> { I would create a separate header file for this. Comment on attachment 124918 [details] Work in progress patch View in context: https://bugs.webkit.org/attachment.cgi?id=124918&action=review Some lifetime commnents > Source/WebCore/dom/ScriptExecutionContext.cpp:131 > + m_publicURLManager->contextDestroyed(); If you end up not ref-counting PublicURLManager (see my comments below) maybe not have a separate PublicURLManager::contextDestroyed() method, and just do cleanup in PublicURLManager's destructor? > Source/WebCore/dom/ScriptExecutionContext.cpp:392 > +PublicURLManager* ScriptExecutionContext::publicURLManager() (see below comment on header file). > Source/WebCore/dom/ScriptExecutionContext.h:110 > + PublicURLManager* publicURLManager(); Since PublicURLManager is a RefCounted, this should return a RefPtr - never leak a naked pointer to ref-counted class. Alternatively, I suggest *not* making PublicURLManager ref-counted - looks like it is only used in conjunction with owning ScriptExecutionContext and never stored anywhere else. > Source/WebCore/dom/ScriptExecutionContext.h:215 > + OwnPtr<PublicURLManager> m_publicURLManager; Ditto, choose if ref-counted or not. If ref-counted, use RefPtr, not OwnPtr (In reply to comment #41) > (From update of attachment 124918 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124918&action=review > > Some lifetime commnents > > > Source/WebCore/dom/ScriptExecutionContext.cpp:131 > > + m_publicURLManager->contextDestroyed(); > > If you end up not ref-counting PublicURLManager (see my comments below) maybe not have a separate PublicURLManager::contextDestroyed() method, and just do cleanup in PublicURLManager's destructor? > > > Source/WebCore/dom/ScriptExecutionContext.cpp:392 > > +PublicURLManager* ScriptExecutionContext::publicURLManager() > > (see below comment on header file). > > > Source/WebCore/dom/ScriptExecutionContext.h:110 > > + PublicURLManager* publicURLManager(); > > Since PublicURLManager is a RefCounted, this should return a RefPtr - never leak a naked pointer to ref-counted class. > Alternatively, I suggest *not* making PublicURLManager ref-counted - looks like it is only used in conjunction with owning ScriptExecutionContext and never stored anywhere else. David suggested offline to return PublicURLManager& instead of any pointers. This is guaranteed to be non-null. > > > Source/WebCore/dom/ScriptExecutionContext.h:215 > > + OwnPtr<PublicURLManager> m_publicURLManager; > > Ditto, choose if ref-counted or not. If ref-counted, use RefPtr, not OwnPtr Created attachment 125273 [details]
WIP
Update patch considering comments & suggestions from David & Dmitry. But there is some issue, adding PublicURLManager as a part of ScriptExecutionContext is changing the rendering position of the test cases by 2 pixels. Can you assist in that?
(In reply to comment #43) > Created an attachment (id=125273) [details] > WIP > > Update patch considering comments & suggestions from David & Dmitry. But there is some issue, adding PublicURLManager as a part of ScriptExecutionContext is changing the rendering position of the test cases by 2 pixels. Can you assist in that? This doesn't make sense. I'd retry the failing tests without the change to check this. Comment on attachment 125273 [details] WIP Attachment 125273 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11421177 Comment on attachment 125273 [details] WIP Attachment 125273 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11424112 Comment on attachment 125273 [details] WIP Attachment 125273 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11422164 Created attachment 125279 [details]
WIP
Missed one file.
Attachment 125279 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/dom/ScriptExecutionContext...." exit_code: 1
Source/WebCore/html/PublicURLManager.h:26: #ifndef header guard has wrong style, please use: PublicURLManager_h [build/header_guard] [5]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 125298 [details]
WIP
Comment on attachment 125298 [details] WIP Attachment 125298 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11421251 New failing tests: fast/dom/window-domurl-crash.html fast/files/workers/inline-worker-via-blob-url.html editing/pasteboard/data-transfer-items-image-png.html fast/files/apply-blob-url-to-img.html fast/dom/HTMLAnchorElement/anchor-download.html Comment on attachment 125298 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=125298&action=review > Source/WebCore/dom/ScriptExecutionContext.cpp:393 > + m_publicURLManager = PublicURLManager::create(); Once m_publicURLManager is allocated, who deallocates it? To avoid this issue entirely, use OwnPtr<PublicURLManager> as a type for m_publicURLManager. > Source/WebCore/html/PublicURLManager.h:48 > + void contextDestroyed() Who calls contextDestroyed()? Created attachment 125599 [details]
Updated Patch
Comment on attachment 125599 [details]
Updated Patch
Looks good to me.
Comment on attachment 125599 [details]
Updated Patch
Ditto.
LGTM. Thanks a lot. Comment on attachment 125599 [details] Updated Patch Rejecting attachment 125599 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: urce/WebCore/workers/WorkerContext.h patching file Source/WebCore/workers/WorkerContext.idl patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions-expected.txt patching file LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Levin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11428488 Comment on attachment 125599 [details]
Updated Patch
Dont knw what it failed. worked at my end. Trying again.
Comment on attachment 125599 [details] Updated Patch Rejecting attachment 125599 [details] from commit-queue. kaustubh@motorola.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Comment on attachment 125599 [details]
Updated Patch
I bet the patch is failing to apply.
You should sync your tree and verify that the patch applies (and/or fix up any conflicts and then reupload).
but I'll cq+ one more time for you.
(In reply to comment #60) > (From update of attachment 125599 [details]) > I bet the patch is failing to apply. > > You should sync your tree and verify that the patch applies (and/or fix up any conflicts and then reupload). > > but I'll cq+ one more time for you. Thanks David, I will try to fix the patch if it again fails to apply. Comment on attachment 125599 [details] Updated Patch Clearing flags on attachment: 125599 Committed r106912: <http://trac.webkit.org/changeset/106912> All reviewed patches have been landed. Closing bug. I rolled out the patch, since it broke AppleWebKit build: https://bugs.webkit.org/show_bug.cgi?id=77947 (In reply to comment #64) > I rolled out the patch, since it broke AppleWebKit build: https://bugs.webkit.org/show_bug.cgi?id=77947 Can you give me logs for build break? (In reply to comment #65) > (In reply to comment #64) > > I rolled out the patch, since it broke AppleWebKit build: https://bugs.webkit.org/show_bug.cgi?id=77947 > > Can you give me logs for build break? This one: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/38567/steps/compile-webkit/logs/stdio Reopening to fix the xcodeproj files required for AppleWebkit build. Created attachment 125803 [details]
Updated patch
Comment on attachment 125803 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=125803&action=review > Source/WebCore/ChangeLog:8 > + Rollout fixes for xcodeproj, vcproj & gypi files. This is not so descriptive. Maybe you can add more explanation about what this patch is doing. > Source/WebCore/GNUmakefile.list.am:2259 > + Source/WebCore/html/PublicURLManager.h \ Nit: One tab instead of four spaces. Comment on attachment 125803 [details] Updated patch Attachment 125803 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11438242 Created attachment 125825 [details]
Patch with rebaseline
Comment on attachment 125825 [details] Patch with rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=125825&action=review > Source/WebCore/dom/ScriptExecutionContext.h:33 > +#include "PublicURLManager.h" By the way, do you need this #include? The below "class PublicURLManager;" would be enough, right? Created attachment 126051 [details]
Fixed patch
I hope this is the final patch and apple webkit build does not fails. Have done changes as suggested by Haraken.
Comment on attachment 126051 [details]
Fixed patch
r+ for the build script change. Based on the previous r+ from David Levin, let me r+ the patch.
Comment on attachment 126051 [details] Fixed patch Clearing flags on attachment: 126051 Committed r107082: <http://trac.webkit.org/changeset/107082> All reviewed patches have been landed. Closing bug. Looks like it included qt and gtk rebaselines but was missing other platforms. I suspect this caused Chromium and OS X Webkit builds to fail tests. Something to consider for future patches. :) This patch seems to cause some problems atleast for JSC, since the static method table for DOMURLConstructor end up shared between threads which doesn't really work. See bug #99178 |