Bug 74386 - Migrate createObjectURL & revokeObjectURL to static (Class) methods
: Migrate createObjectURL & revokeObjectURL to static (Class) methods
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
http://www.w3.org/TR/FileAPI/#creatin...
:
Depends on: 77182 77947
Blocks: 74385
  Show dependency treegraph
 
Reported: 2011-12-13 00:31 PST by Kaustubh Atrawalkar
Modified: 2012-10-15 01:47 PDT (History)
15 users (show)

See Also:


Attachments
Patch (18.11 KB, patch)
2011-12-13 02:25 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated patch (18.30 KB, patch)
2011-12-14 02:05 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (18.74 KB, patch)
2011-12-14 02:20 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Fixed ChangeLog (17.72 KB, patch)
2011-12-14 05:15 PST, Kaustubh Atrawalkar
arv: review-
Details | Formatted Diff | Diff
Patch (17.91 KB, patch)
2012-01-12 03:47 PST, Kaustubh Atrawalkar
arv: review-
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Updated Patch (17.90 KB, patch)
2012-01-17 23:50 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (18.04 KB, patch)
2012-01-18 22:53 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Work in progress patch (19.03 KB, patch)
2012-02-01 02:37 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
WIP (13.30 KB, patch)
2012-02-02 23:30 PST, Kaustubh Atrawalkar
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
WIP (16.25 KB, patch)
2012-02-03 00:48 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
WIP (20.79 KB, patch)
2012-02-03 02:50 PST, Kaustubh Atrawalkar
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Updated Patch (21.13 KB, patch)
2012-02-06 01:52 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated patch (26.45 KB, patch)
2012-02-07 03:37 PST, Kaustubh Atrawalkar
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Patch with rebaseline (30.38 KB, patch)
2012-02-07 05:30 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Fixed patch (30.21 KB, patch)
2012-02-08 04:13 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaustubh Atrawalkar 2011-12-13 00:31:08 PST
Need to move createObjectURL & revokeObjectURL from DOMURL implementation to static methods as per specs - http://www.w3.org/TR/FileAPI/#creating-revoking
Comment 1 Kaustubh Atrawalkar 2011-12-13 02:25:09 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.
Comment 2 Kaustubh Atrawalkar 2011-12-13 02:26:56 PST
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 3 Erik Arvidsson 2011-12-13 10:11:45 PST
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
Comment 4 Kaustubh Atrawalkar 2011-12-14 02:05:30 PST
Created attachment 119180 [details]
Updated patch
Comment 5 Kaustubh Atrawalkar 2011-12-14 02:06:51 PST
Comment on attachment 119180 [details]
Updated patch

Changed the patch as per Erik's comments. Darin can you review once?
Comment 6 Kaustubh Atrawalkar 2011-12-14 02:20:11 PST
Created attachment 119183 [details]
Updated Patch
Comment 7 Kaustubh Atrawalkar 2011-12-14 05:15:44 PST
Created attachment 119207 [details]
Fixed ChangeLog
Comment 8 Adam Barth 2011-12-21 13:30:24 PST
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?
Comment 9 Erik Arvidsson 2011-12-21 14:02:17 PST
(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".
Comment 10 Eric Seidel 2011-12-21 15:01:55 PST
He's not a committer, so this might as well be an r- if we're not gonna set cq+ :)
Comment 11 Erik Arvidsson 2011-12-21 15:25:12 PST
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.
Comment 12 Kaustubh Atrawalkar 2012-01-12 03:47:37 PST
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 13 WebKit Review Bot 2012-01-12 04:37:42 PST
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 14 Erik Arvidsson 2012-01-17 16:32:32 PST
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.
Comment 15 Kaustubh Atrawalkar 2012-01-17 23:50:18 PST
Created attachment 122880 [details]
Updated Patch

Fixed test case.
Comment 16 Erik Arvidsson 2012-01-18 11:40:17 PST
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.
Comment 17 Kaustubh Atrawalkar 2012-01-18 22:53:51 PST
Created attachment 123078 [details]
Updated Patch
Comment 18 Erik Arvidsson 2012-01-19 11:39:38 PST
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 19 Adam Barth 2012-01-19 14:28:49 PST
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?
Comment 20 Kaustubh Atrawalkar 2012-01-19 22:43:13 PST
(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 21 Adam Barth 2012-01-19 22:57:43 PST
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 22 WebKit Review Bot 2012-01-20 00:00:27 PST
Comment on attachment 123078 [details]
Updated Patch

Clearing flags on attachment: 123078

Committed r105486: <http://trac.webkit.org/changeset/105486>
Comment 23 WebKit Review Bot 2012-01-20 00:00:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Simon Fraser (smfr) 2012-01-20 11:50:55 PST
This broke these tests:
fast/dom/prototype-inheritance-2.html
fast/js/global-constructors.html
Comment 25 Kaustubh Atrawalkar 2012-01-22 22:42:35 PST
(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.
Comment 26 Adam Barth 2012-01-22 23:38:53 PST
http://build.webkit.org/waterfall can answer these sorts of questions.  It looks like Lion, at least.
Comment 27 David Levin 2012-01-24 15:06:08 PST
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).
Comment 28 David Levin 2012-01-24 15:10:00 PST
I'll wait a day or two before rolling out to see if there a fix being done quickly.
Comment 29 Adam Barth 2012-01-24 16:12:57 PST
Thanks Dave.  I'm sorry I missed those issues in my review.  I wasn't thinking about the worker case.
Comment 30 David Levin 2012-01-24 16:18:35 PST
(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 :) .)
Comment 31 Adam Barth 2012-01-24 16:45:05 PST
> 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.
Comment 32 Kaustubh Atrawalkar 2012-01-24 19:36:13 PST
(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.
Comment 33 David Levin 2012-01-24 23:15:48 PST
(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 34 Dmitry Lomov 2012-01-25 09:42:19 PST
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.
Comment 35 David Levin 2012-01-25 09:44:17 PST
Re-opening due to threadsafety issue.
Comment 36 David Levin 2012-01-25 10:06:58 PST
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.
Comment 37 David Levin 2012-01-26 21:10:45 PST
(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
Comment 38 Csaba Osztrogonác 2012-01-27 03:58:14 PST
(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.
Comment 39 Kaustubh Atrawalkar 2012-02-01 02:37:31 PST
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 40 David Levin 2012-02-01 02:43:56 PST
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 41 Dmitry Lomov 2012-02-01 09:28:23 PST
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
Comment 42 Dmitry Lomov 2012-02-01 10:09:05 PST
(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
Comment 43 Kaustubh Atrawalkar 2012-02-02 23:30:49 PST
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?
Comment 44 David Levin 2012-02-02 23:36:35 PST
(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 45 Early Warning System Bot 2012-02-03 00:03:04 PST
Comment on attachment 125273 [details]
WIP

Attachment 125273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11421177
Comment 46 WebKit Review Bot 2012-02-03 00:34:05 PST
Comment on attachment 125273 [details]
WIP

Attachment 125273 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11424112
Comment 47 Gyuyoung Kim 2012-02-03 00:44:43 PST
Comment on attachment 125273 [details]
WIP

Attachment 125273 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11422164
Comment 48 Kaustubh Atrawalkar 2012-02-03 00:48:32 PST
Created attachment 125279 [details]
WIP

Missed one file.
Comment 49 WebKit Review Bot 2012-02-03 00:50:45 PST
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.
Comment 50 Kaustubh Atrawalkar 2012-02-03 02:50:20 PST
Created attachment 125298 [details]
WIP
Comment 51 WebKit Review Bot 2012-02-03 03:47:07 PST
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 52 Dmitry Lomov 2012-02-03 11:31:54 PST
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()?
Comment 53 Kaustubh Atrawalkar 2012-02-06 01:52:52 PST
Created attachment 125599 [details]
Updated Patch
Comment 54 Dmitry Lomov 2012-02-06 09:47:20 PST
Comment on attachment 125599 [details]
Updated Patch

Looks good to me.
Comment 55 David Levin 2012-02-06 10:24:44 PST
Comment on attachment 125599 [details]
Updated Patch

Ditto.
Comment 56 Erik Arvidsson 2012-02-06 10:26:53 PST
LGTM. Thanks a lot.
Comment 57 WebKit Review Bot 2012-02-06 10:26:54 PST
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 58 Kaustubh Atrawalkar 2012-02-06 23:17:41 PST
Comment on attachment 125599 [details]
Updated Patch

Dont knw what it failed. worked at my end. Trying again.
Comment 59 WebKit Review Bot 2012-02-06 23:18:24 PST
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 60 David Levin 2012-02-06 23:34:13 PST
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.
Comment 61 Kaustubh Atrawalkar 2012-02-06 23:50:39 PST
(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 62 WebKit Review Bot 2012-02-07 00:18:54 PST
Comment on attachment 125599 [details]
Updated Patch

Clearing flags on attachment: 125599

Committed r106912: <http://trac.webkit.org/changeset/106912>
Comment 63 WebKit Review Bot 2012-02-07 00:19:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 64 Kentaro Hara 2012-02-07 01:46:42 PST
I rolled out the patch, since it broke AppleWebKit build: https://bugs.webkit.org/show_bug.cgi?id=77947
Comment 65 Kaustubh Atrawalkar 2012-02-07 01:48:47 PST
(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?
Comment 66 Kentaro Hara 2012-02-07 01:52:14 PST
(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
Comment 67 Kaustubh Atrawalkar 2012-02-07 03:21:09 PST
Reopening to fix the xcodeproj files required for AppleWebkit build.
Comment 68 Kaustubh Atrawalkar 2012-02-07 03:37:46 PST
Created attachment 125803 [details]
Updated patch
Comment 69 Kentaro Hara 2012-02-07 03:44:19 PST
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 70 WebKit Review Bot 2012-02-07 04:07:02 PST
Comment on attachment 125803 [details]
Updated patch

Attachment 125803 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11438242
Comment 71 Kaustubh Atrawalkar 2012-02-07 05:30:04 PST
Created attachment 125825 [details]
Patch with rebaseline
Comment 72 Kentaro Hara 2012-02-07 06:35:10 PST
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?
Comment 73 Kaustubh Atrawalkar 2012-02-08 04:13:35 PST
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 74 Kentaro Hara 2012-02-08 04:55:00 PST
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 75 WebKit Review Bot 2012-02-08 06:39:36 PST
Comment on attachment 126051 [details]
Fixed patch

Clearing flags on attachment: 126051

Committed r107082: <http://trac.webkit.org/changeset/107082>
Comment 76 WebKit Review Bot 2012-02-08 06:39:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 77 David Levin 2012-02-08 07:31:42 PST
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. :)
Comment 78 Allan Sandfeld Jensen 2012-10-15 01:47:06 PDT
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