[Webkit-unassigned] [Bug 177932] [GTK] New API to add, retrieve and delete cookies via WebKitCookieManager

[bugzilla-daemon at webkit.org]

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

--- Comment #14 from Michael Catanzaro <mcatanzaro at igalia.com> ---
(In reply to Mario Sanchez Prada from comment #13)
> > > Source/WebCore/platform/network/soup/CookieSoup.cpp:57
> > > +Cookie::operator SoupCookie *() const
> > 
> > This is too risky IMO. We can't have implicit conversion operators returning
> > newly-allocated memory: that's a recipe for accidental leaks. You can
> > achieve almost the same result by adding a static toSoupCookie() method,
> > with the benefit that it has to be called explicitly.
> 
> This was actually a suggestion made by Carlos in the hackfest, although it's
> entirely possible I misunderstood him. That said, I don't mind changing the
> code as you suggest, but I think I'll wait for his review first, just in
> case we are missing something.

Sometimes what seems to be a good idea is not, in fact, a good idea. Consider a few of the many ways this can go badly:

void someFunction(SoupCookie* transferNone);
WebCore::Cookie cookie;
someFunction(&cookie); // hidden leak

or:

WebCore::Cookie cookie;
auto* cookie = static_cast<SoupCookie*>(&cookie); // hidden leak

In both cases, it's pretty surprising that an allocation occurs at all.

> I agree it's a bit obscure, yes. You'r convincing me by the moment!

Good. ;)

> > > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:271
> > > +            // This can only happen in cases where the web process is not available,
> > > +            // consider the operation "cancelled" from the point of view of the client.
> > > +            g_task_return_new_error(task.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled"));
> > > +            return;
> > 
> > Ah, maybe this really should block on bug #175265, so we don't have to add
> > an unnecessary error here.
> 
> Hmmm.. you sure? I feel like returning this error here, even it's kind of a
> generic catch-all kind of error is not a bad idea. Future is uncertain, as
> they say

Well, it's certainly a good idea to return the error until we have that bug fixed. Fixing that bug entails ensuring these functions work without a web process, so your patch actually complicates that work.

> > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:65
> > > +        , m_cookies(0)
> > > +        , m_cookieAdded(false)
> > >          , m_cookiesChanged(false)
> > > +        , m_cookieDeleted(false)
> > 
> > Use inline initialization for the new data members. Bonus points if you
> > convert the entire initializer list to inline initialization. (It's project
> > style to do small cleanups like that regularly in larger patches, rather
> > than in separate patches as is common in GNOME.)
> 
> Sure, but to avoid me misunderstanding things again... would you mind
> posting a pointer to some other test that's already looking the way you
> suggest?

Initialize them in the class declaration like this:

    WebKitCookieManager* m_cookieManager { nullptr };
    WebKitCookieAcceptPolicy m_acceptPolicy { WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY };
    char** m_domains { nullptr };
    bool m_cookiesChanged { false };
    bool m_finishLoopWhenCookiesChange { false };
    GUniquePtr<char> m_cookiesTextFile;
    GUniquePtr<char> m_cookiesSQLiteFile;

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171108/cb58cbff/attachment.html>