August 2010 - webkit-dev - lists.webkit.org

Misplaced files
by Adam Barth 28 Aug '10

28 Aug '10

Looking through WebCore/html I noticed the files below. I'm not sure they belong in WebCore/html because they don't appear to be HTML-specific. Rather, they seem like generic web platform APIs (e.g., they could be exposed to SVG or whatever other markup languages we choose to support in the future). Should we move these to WebCore/page, WebCore/dom, or should we make a new location for these sorts of things? Adam == Blob-related == Blob.cpp Blob.h Blob.idl BlobBuilder.cpp BlobBuilder.h BlobBuilder.idl BlobURL.cpp BlobURL.h ThreadableBlobRegistry.cpp ThreadableBlobRegistry.h == File-related == File.cpp File.h File.idl FileError.h FileError.idl FileList.cpp FileList.h FileList.idl FileReader.cpp FileReader.h FileReader.idl FileStreamProxy.cpp FileStreamProxy.h FileThread.cpp FileThread.h FileThreadTask.h FileWriter.cpp FileWriter.h FileWriter.idl

4 5

URL and text/uri-list handling in event.DataTransfer
by Daniel Cheng 27 Aug '10

27 Aug '10

The section on http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#datatr… claims that URL is simply a synonym for text/uri-list. Why does WebKit treat URL differently from text/uri-list? Should we fix it so that both are treated identically? It makes the code simpler and makes the behavior consistent with the treatment of Text and text/plain. Daniel

1 0

Naked new considered harmful
by Adam Barth 27 Aug '10

27 Aug '10

One thing Darin and I discussed at WWDC (yes, this email has been a long time coming) is better programming patterns to prevent memory leaks. As I'm sure you know, whenever you allocate a RefCounted object, you need to call adoptRef to prevent a memory leak by adopting the object's initial reference: adoptRef(new FancyRefCountedObject()) We now have an ASSERT that enforces this programming pattern, so we should be able to catch errors pretty easily. Recently, Darin also introduced an analogous function for OwnPtr: adoptPtr(new NiftyNonRefCountedObject()) What adoptPtr does is shove the newly allocated object into a PassOwnPtr, which together with OwnPtr, makes sure we don't leak the object. By using one of these two functions every time we call new, it's easy to see that we don't have any memory leaks. In the cases where we have an intentional memory leak (e.g., for a static), please use the leakPtr() member function to document the leak. In writing new code, please avoid using "naked" calls to new. If you're making an object that you expect to be heap-allocated, please add a "create" method, similar to the create method we use for RefCounted objects: static PassOwnPtr<NiftyNonRefCountedObject> create(ParamClass* param) { return adoptPtr(new NiftyNonRefCountedObject(param)); } You should also make the constructor non-public so folks are forced to call the create method. (Of course, stack-allocated objects should have public constructors.) I'm going through WebCore and removing as many naked news and I can. At some point, we'll add a stylebot rule to flag violations for new code. If you'd like to help out, pick a directory and change all the calls to new to use adoptRef or adoptPtr, as appropriate. Adam

6 11

style question for empty for loops
by Chris Fleizach 26 Aug '10

26 Aug '10

Which is preferred? for (...; ...; ...) { } or for (...; ...; ...) { }

5 8

What is XBL?
by Adam Barth 25 Aug '10

25 Aug '10

What is XBL? As far as I can tell, there's no way this code can even build. It's incredibly stale. Can we remove it? Adam

9 11

Bad Qt API?
by Eric Seidel 25 Aug '10

25 Aug '10

/*! Encloses the contents of this element with the result of parsing \a markup. This element becomes the child of the deepest descendant within \a markup. \sa encloseWith() */ void QWebElement::encloseContentsWith(const QString &markup) http://trac.webkit.org/browser/trunk/WebKit/qt/Api/qwebelement.cpp#L1248 These enclose methods use at least 2 deprecated parts of parser code (HTMLElement::endTagRequirement() and HTMLElement::deprecatedCreateContextualFragment()). They're clear layering violations, and make little sense to me. Who wants to call this API? Can it be removed from Qt? -eric

2 2

Re: [webkit-dev] Bad Qt API?
by Eric Seidel 25 Aug '10

25 Aug '10

Furthermore, any loops like this: for (RefPtr<Node> child = m_element->firstChild(); child;) { which allow synchronous javascript execution (i.e. take an ExceptionCode parameter) are vulnerable to crashes/security holes. :( All of those enclose* functions use such loops. :( -eric On Wed, Aug 25, 2010 at 11:47 AM, Eric Seidel <eric(a)webkit.org> wrote: > My comments apply to all of the enclose* APIs in that file. > > On Wed, Aug 25, 2010 at 11:46 AM, Eric Seidel <eric(a)webkit.org> wrote: >> /*! >> Encloses the contents of this element with the result of parsing \a markup. >> This element becomes the child of the deepest descendant within \a markup. >> >> \sa encloseWith() >> */ >> void QWebElement::encloseContentsWith(const QString &markup) >> >> >> http://trac.webkit.org/browser/trunk/WebKit/qt/Api/qwebelement.cpp#L1248 >> >> These enclose methods use at least 2 deprecated parts of parser code >> (HTMLElement::endTagRequirement() and >> HTMLElement::deprecatedCreateContextualFragment()). >> >> They're clear layering violations, and make little sense to me. >> >> Who wants to call this API? Can it be removed from Qt? >> >> -eric >> >

2 1

Re: [webkit-dev] Bad Qt API?
by Eric Seidel 25 Aug '10

25 Aug '10

My comments apply to all of the enclose* APIs in that file. On Wed, Aug 25, 2010 at 11:46 AM, Eric Seidel <eric(a)webkit.org> wrote: > /*! > Encloses the contents of this element with the result of parsing \a markup. > This element becomes the child of the deepest descendant within \a markup. > > \sa encloseWith() > */ > void QWebElement::encloseContentsWith(const QString &markup) > > > http://trac.webkit.org/browser/trunk/WebKit/qt/Api/qwebelement.cpp#L1248 > > These enclose methods use at least 2 deprecated parts of parser code > (HTMLElement::endTagRequirement() and > HTMLElement::deprecatedCreateContextualFragment()). > > They're clear layering violations, and make little sense to me. > > Who wants to call this API? Can it be removed from Qt? > > -eric >

1 0

Making GRefPtr more general
by Martin Robinson 25 Aug '10

25 Aug '10

Recently there was a need to make the smart pointer wrapping GLib reference-counted types more general. In this case it was to extend it to Cairo. Here is the bug for that: https://bugs.webkit.org/show_bug.cgi?id=44354 It struck me that the name GRefPtr wasn't very applicable to this type any longer, so I changed it to PlatformRefPtr in my patch. That seemed a bit wordy and others have suggested PRefPtr. Since this naming affects more than just the GTK+ port, I thought I would get some opinions on a name that is easy to type and fitting. Martin

3 4

Place For new Core Module
by Chinmaya Sn 25 Aug '10

25 Aug '10

Typically WebKit has 3 doors for external world (i.e whoever wants to use WebKit), platform specific interfaces and WebKit layer, and JavaScript bindings. I have special case in my hand; currently I maintain a custom branch of WebKit, I already have implemented a module which sits closely with WebCore and it works just fine. One of my goals is to deliver these changes back to community and even may be back to WebKit repository. In near future I will be implementing at least 2-3 such modules, which work closely with WebCore and JavaScriptCore but they may not have to anything to do HTML browser engine as such. I need some suggestions from WebKit Guru's on how such code can be structured so that this can be contributed back to WebKit without disturbing harmony of many WebKit embedders. Basically future proof. Thanks Chinmaya

8 12