[Webkit-unassigned] [Bug 26854] [GTK] Needs API to allow more control over outgoing requests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26854





--- Comment #18 from Gustavo Noronha (kov) <gns at gnome.org>  2009-09-07 07:07:37 PDT ---
New Note 30

(In reply to comment #17)
> > +#include "CString.h"
> > +#include "GOwnPtr.h"
> > +#include "PlatformString.h"
> 
> I'd say some of these are unused here?

Only GOwnPtr.h =)

> > +#include <libsoup/soup.h>
> 
> And this one is included in ResourceResponse.h already?

Right =)

> As mentioned on IRC it would we good to share this function with
> ResourceRequest, since it's identical to what's there but for one line.

I am not sure where I would add a single function that both call, and I think
the only thing that makes sense sharing is the copying of headers, for now, so
I decided to not mess with this right now.

> I think we should just pass NULL instead of a dummy object here? Since the
> point is to inform the user of whether this is a redirect and such, I think
> that makes it much more clear.

OK, I for some reason have in my mind that passing NULL as the value of an
object might blow up the signal emission, but I agree.

> Also you are leaking the networkResponse object AFAICT.

Yeah! Turned all of them into GOwnPtr's.

> > +    // The URI is important to have in the resource because that is what we use
> > +    // to fetch the WebCore object, later on. We use the private structure
> > +    // directly here to avoid changing the property mode from READABLE to
> > +    // READWRITE.
> > +    webResource->priv->uri = g_strdup(request.url().string().utf8().data());
> 
> I don't understand the comment. You are creating the object now, so you
> certainly pass the URI you want in the parameters.

Only explaining why I am setting the URI directly instead of initing with the
core resource object, really, but this is gone - this part of the code is also
only needed for the other signals. I removed all the identifier handling, and
the hashtable.

> > +    const char* uri = webkit_web_resource_get_uri(webResource);
> > +    RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(), uri)));
> > +    webkit_web_resource_init_with_core_resource(webResource, coreResource.get());
> 
> I don't understand why are you init again the resource if you are about the
> remove it from the view?

Yeah, this only makes sense with the load-status thing; this is the time when
WebCore already has its ArchivedResource object, so I was depending on
load-status to inform the user that the WebResource object just turned more
useful than just containing the URI, so this is for the next patch.

> > +    notImplemented();
> 
> notImplemented()? The function does something. Do you know now what should it
> do that it's not doing? If so add a comment.

Yeah, this is load-error.

> >      PROP_ENCODING,
> > -    PROP_FRAME_NAME
> > +    PROP_FRAME_NAME,
> >  };
> 
> Uh? :)

More debris from removing load-status =/

> Since this function is only used before removing a resource now I'm really
> curious about what's its point.

Yeah, next patch material.

> This seems subtle enough to warrant a comment. Was it broken before? 

Not really, but now we want to be able to have an URI even with no
ArchivedResource available, because we're creating the object before we have an
ArchivedResource, and the URI is the only useful thing we have.

> I don't think you need all these now, just the first one.

okidoki

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.