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


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


Note

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


Description From 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 From 2011-12-13 02:25:09 PST -------
Created an attachment (id=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 From 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 From 2011-12-13 10:11:45 PST -------
(From update of attachment 118981 [details])
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 From 2011-12-14 02:05:30 PST -------
Created an attachment (id=119180) [details]
Update
------- Comment #5 From 2011-12-14 02:06:51 PST -------
(From update of attachment 119180 [details])
Changed the patch as per Erik's comments. Darin can you review once?
------- Comment #6 From 2011-12-14 02:20:11 PST -------
Created an attachment (id=119183) [details]
Updated Patch
------- Comment #7 From 2011-12-14 05:15:44 PST -------
Created an attachment (id=119207) [details]
Fixed ChangeLog
------- Comment #8 From 2011-12-21 13:30:24 PST -------
(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?
------- Comment #9 From 2011-12-21 14:02:17 PST -------
(In reply to comment #8)
> (From update of attachment 119207 [details] [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 From 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 From 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 From 2012-01-12 03:47:37 PST -------
Created an attachment (id=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 From 2012-01-12 04:37:42 PST -------
(From update of attachment 122206 [details])
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 From 2012-01-17 16:32:32 PST -------
(From update of attachment 122206 [details])
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 From 2012-01-17 23:50:18 PST -------
Created an attachment (id=122880) [details]
Updated Patch

Fixed test case.
------- Comment #16 From 2012-01-18 11:40:17 PST -------
(From update of attachment 122880 [details])
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 From 2012-01-18 22:53:51 PST -------
Created an attachment (id=123078) [details]
Updated Patch
------- Comment #18 From 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 From 2012-01-19 14:28:49 PST -------
(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.

> 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 From 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] [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 From 2012-01-19 22:57:43 PST -------
(From update of attachment 123078 [details])
Ok.  We can add OVERRIDE next time we touch this file.  It's not worth respinning the patch.  Thanks!
------- Comment #22 From 2012-01-20 00:00:27 PST -------
(From update of attachment 123078 [details])
Clearing flags on attachment: 123078

Committed r105486: <http://trac.webkit.org/changeset/105486>
------- Comment #23 From 2012-01-20 00:00:33 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2012-01-20 11:50:55 PST -------
This broke these tests:
fast/dom/prototype-inheritance-2.html
fast/js/global-constructors.html
------- Comment #25 From 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 From 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 From 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 From 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 From 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 From 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 From 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 From 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 From 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 From 2012-01-25 09:42:19 PST -------
(From update of attachment 123078 [details])
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 From 2012-01-25 09:44:17 PST -------
Re-opening due to threadsafety issue.
------- Comment #36 From 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 From 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 From 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 From 2012-02-01 02:37:31 PST -------
Created an attachment (id=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 From 2012-02-01 02:43:56 PST -------
(From update of attachment 124918 [details])
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 From 2012-02-01 09:28:23 PST -------
(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.

> 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 From 2012-02-01 10:09:05 PST -------
(In reply to comment #41)
> (From update of attachment 124918 [details] [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 From 2012-02-02 23:30:49 PST -------
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?
------- Comment #44 From 2012-02-02 23:36:35 PST -------
(In reply to comment #43)
> Created an attachment (id=125273) [details] [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 From 2012-02-03 00:03:04 PST -------
(From update of attachment 125273 [details])
Attachment 125273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11421177
------- Comment #46 From 2012-02-03 00:34:05 PST -------
(From update of attachment 125273 [details])
Attachment 125273 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11424112
------- Comment #47 From 2012-02-03 00:44:43 PST -------
(From update of attachment 125273 [details])
Attachment 125273 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11422164
------- Comment #48 From 2012-02-03 00:48:32 PST -------
Created an attachment (id=125279) [details]
WIP

Missed one file.
------- Comment #49 From 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 From 2012-02-03 02:50:20 PST -------
Created an attachment (id=125298) [details]
WIP
------- Comment #51 From 2012-02-03 03:47:07 PST -------
(From update of attachment 125298 [details])
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 From 2012-02-03 11:31:54 PST -------
(From update of attachment 125298 [details])
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 From 2012-02-06 01:52:52 PST -------
Created an attachment (id=125599) [details]
Updated Patch
------- Comment #54 From 2012-02-06 09:47:20 PST -------
(From update of attachment 125599 [details])
Looks good to me.
------- Comment #55 From 2012-02-06 10:24:44 PST -------
(From update of attachment 125599 [details])
Ditto.
------- Comment #56 From 2012-02-06 10:26:53 PST -------
LGTM. Thanks a lot.
------- Comment #57 From 2012-02-06 10:26:54 PST -------
(From update of attachment 125599 [details])
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 From 2012-02-06 23:17:41 PST -------
(From update of attachment 125599 [details])
Dont knw what it failed. worked at my end. Trying again.
------- Comment #59 From 2012-02-06 23:18:24 PST -------
(From update of attachment 125599 [details])
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 From 2012-02-06 23:34:13 PST -------
(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.
------- Comment #61 From 2012-02-06 23:50:39 PST -------
(In reply to comment #60)
> (From update of attachment 125599 [details] [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 From 2012-02-07 00:18:54 PST -------
(From update of attachment 125599 [details])
Clearing flags on attachment: 125599

Committed r106912: <http://trac.webkit.org/changeset/106912>
------- Comment #63 From 2012-02-07 00:19:03 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #64 From 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 From 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 From 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 From 2012-02-07 03:21:09 PST -------
Reopening to fix the xcodeproj files required for AppleWebkit build.
------- Comment #68 From 2012-02-07 03:37:46 PST -------
Created an attachment (id=125803) [details]
Updated patch
------- Comment #69 From 2012-02-07 03:44:19 PST -------
(From update of attachment 125803 [details])
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 From 2012-02-07 04:07:02 PST -------
(From update of attachment 125803 [details])
Attachment 125803 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11438242
------- Comment #71 From 2012-02-07 05:30:04 PST -------
Created an attachment (id=125825) [details]
Patch with rebaseline
------- Comment #72 From 2012-02-07 06:35:10 PST -------
(From update of attachment 125825 [details])
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 From 2012-02-08 04:13:35 PST -------
Created an attachment (id=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 From 2012-02-08 04:55:00 PST -------
(From update of attachment 126051 [details])
r+ for the build script change. Based on the previous r+ from David Levin, let me r+ the patch.
------- Comment #75 From 2012-02-08 06:39:36 PST -------
(From update of attachment 126051 [details])
Clearing flags on attachment: 126051

Committed r107082: <http://trac.webkit.org/changeset/107082>
------- Comment #76 From 2012-02-08 06:39:46 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #77 From 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 From 2012-10-15 01:47:06 PST -------
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