Bug 74386 – Migrate createObjectURL & revokeObjectURL to static (Class) methods

• Component: DOM
• Status: RESOLVED FIXED

[Kaustubh Atrawalkar]

Need to move createObjectURL & revokeObjectURL from DOMURL implementation to static methods as per specs - http://www.w3.org/TR/FileAPI/#creating-revoking

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

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.

[Erik Arvidsson]

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

[Kaustubh Atrawalkar]

Created attachment 119180 [details]
Updated patch

[Kaustubh Atrawalkar]

Comment on attachment 119180 [details]
Updated patch

Changed the patch as per Erik's comments. Darin can you review once?

[Kaustubh Atrawalkar]

Created attachment 119183 [details]
Updated Patch

[Kaustubh Atrawalkar]

Created attachment 119207 [details]
Fixed ChangeLog

[Adam Barth]

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?

[Erik Arvidsson]

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

[Eric Seidel (no email)]

He's not a committer, so this might as well be an r- if we're not gonna set cq+ :)

[Erik Arvidsson]

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.

[Kaustubh Atrawalkar]

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.

[WebKit Review Bot]

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

[Erik Arvidsson]

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.

[Kaustubh Atrawalkar]

Created attachment 122880 [details]
Updated Patch

Fixed test case.

[Erik Arvidsson]

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.

[Kaustubh Atrawalkar]

Created attachment 123078 [details]
Updated Patch

[Erik Arvidsson]

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.

[Adam Barth]

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?

[Kaustubh Atrawalkar]

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

[Adam Barth]

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!

[WebKit Review Bot]

Comment on attachment 123078 [details]
Updated Patch

Clearing flags on attachment: 123078

Committed r105486: <http://trac.webkit.org/changeset/105486>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke these tests:
fast/dom/prototype-inheritance-2.html
fast/js/global-constructors.html

[Kaustubh Atrawalkar]

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

[Adam Barth]

http://build.webkit.org/waterfall can answer these sorts of questions.  It looks like Lion, at least.

[David Levin]

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

[David Levin]

I'll wait a day or two before rolling out to see if there a fix being done quickly.

[Adam Barth]

Thanks Dave.  I'm sorry I missed those issues in my review.  I wasn't thinking about the worker case.

[David Levin]

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

[Adam Barth]

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

[Kaustubh Atrawalkar]

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

[David Levin]

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

[Dmitry Lomov]

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.

[David Levin]

Re-opening due to threadsafety issue.

[David Levin]

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.

[David Levin]

(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

[Csaba Osztrogonác]

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

[Kaustubh Atrawalkar]

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.

[David Levin]

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.

[Dmitry Lomov]

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

[Dmitry Lomov]

(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

[Kaustubh Atrawalkar]

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?

[David Levin]

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

[Early Warning System Bot]

Comment on attachment 125273 [details]
WIP

Attachment 125273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11421177

[WebKit Review Bot]

Comment on attachment 125273 [details]
WIP

Attachment 125273 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11424112

[Gyuyoung Kim]

Comment on attachment 125273 [details]
WIP

Attachment 125273 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11422164

[Kaustubh Atrawalkar]

Created attachment 125279 [details]
WIP

Missed one file.

[WebKit Review Bot]

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.

[Kaustubh Atrawalkar]

Created attachment 125298 [details]
WIP

[WebKit Review Bot]

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

[Dmitry Lomov]

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()?

[Kaustubh Atrawalkar]

Created attachment 125599 [details]
Updated Patch

[Dmitry Lomov]

Comment on attachment 125599 [details]
Updated Patch

Looks good to me.

[David Levin]

Comment on attachment 125599 [details]
Updated Patch

Ditto.

[Erik Arvidsson]

LGTM. Thanks a lot.

[WebKit Review Bot]

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

[Kaustubh Atrawalkar]

Comment on attachment 125599 [details]
Updated Patch

Dont knw what it failed. worked at my end. Trying again.

[WebKit Review Bot]

Comment on attachment 125599 [details]
Updated Patch

Rejecting attachment 125599 [details] from commit-queue.

[e-mail] 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.

[David Levin]

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.

[Kaustubh Atrawalkar]

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

[WebKit Review Bot]

Comment on attachment 125599 [details]
Updated Patch

Clearing flags on attachment: 125599

Committed r106912: <http://trac.webkit.org/changeset/106912>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

I rolled out the patch, since it broke AppleWebKit build: https://bugs.webkit.org/show_bug.cgi?id=77947

[Kaustubh Atrawalkar]

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

[Kentaro Hara]

(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

[Kaustubh Atrawalkar]

Reopening to fix the xcodeproj files required for AppleWebkit build.

[Kaustubh Atrawalkar]

Created attachment 125803 [details]
Updated patch

[Kentaro Hara]

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.

[WebKit Review Bot]

Comment on attachment 125803 [details]
Updated patch

Attachment 125803 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11438242

[Kaustubh Atrawalkar]

Created attachment 125825 [details]
Patch with rebaseline

[Kentaro Hara]

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?

[Kaustubh Atrawalkar]

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.

[Kentaro Hara]

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.

[WebKit Review Bot]

Comment on attachment 126051 [details]
Fixed patch

Clearing flags on attachment: 126051

Committed r107082: <http://trac.webkit.org/changeset/107082>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

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

[Allan Sandfeld Jensen]

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