Bug 28269 – Inspector: Improve Cookie DataGrid to Show Hidden Data

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Now that the C++ is in place to access the cookie data, it should be shown in the DataGrid!

[Joseph Pecoraro]

Created attachment 34759 [details]
IMAGE: Improved Cookie DataGrid with Hidden Cookie Data

[Joseph Pecoraro]

Created attachment 34760 [details]
IMAGE: Fallback behavior (document.cookie) if platform hasn't implemented raw cookie access

[Joseph Pecoraro]

Created attachment 34805 [details]
Improved Cookies DataGrid

[Joseph Pecoraro]

C++ NOTES:

  - changed getRawCookies to return a bool, true if implemented, false otherwise
  - Custom Backend Binding will return jsUndefined if unimplemented
  - added deleteCookie, implemented on mac
  - added v8 stubs to prevent a crash

JavaScript NOTES:

  - if getRawCookies was unimplemented resort to fallback behavior
  - fallback behavior just shows cookies Name/Value, no delete button
  - normal behavior shows Name, Value, Domain, Path, Expires, Size, HTTP, Secure
  - delete cookie button is allows for _any_ cookie

QUESTIONS:

  - is "HTTP" the best header name
  - should the UI be updated, for instance the Storage Panel's Sidebar
  - the DataGrid seems crowded when undocked, but looks awesome docked

Cheers

[Joseph Pecoraro]

Just a heads up: I go on vacation early Friday - late Sunday.  Its possible that early Friday I can respond to comments, but otherwise I'll be gone all weekend and unable to respond at all.

[Timothy Hatcher]

Comment on attachment 34805 [details]
Improved Cookies DataGrid


> +    this._useFallback = typeof InspectorController.cookies() === "undefined";

This should be use typeof. We have had this talk before. :)

This does seem expensive for the case when it is implemented, sicne you get the cookie array twice. Maybe you can store the cookies here for use later. Or delate this until the first update is done?
> +            cookies.push(new WebInspector.Cookie(cookie.name, cookie.value, cookie.domain, cookie.path, cookie.expires, cookie.size, cookie.httpOnly, cookie.secure, cookie.session));

We don't need to use WebInspector.Cookie except for the fallback case. You can just directly use the cookie objects in the aray.

> +                cookies.push(new WebInspector.Cookie(name, value, null, null, null, size));

Maybe we can remove WebInspector.Cookie and just make inline objects to fake a cookie. So:

cookies.push({name: name, value: value});

> +        if (String([cookie name]) == cookieName) {

This should be written the other way so you don't convert each cookie's name to C++.

Convert cookieName once to NSString * before the loop. Then do:

if ([[cookie name] isEqualToString:cookieNameString])

[Joseph Pecoraro]

(In reply to comment #6)
> (From update of attachment 34805 [details])
> 
> > +    this._useFallback = typeof InspectorController.cookies() === "undefined";
> 
> This should be use typeof. We have had this talk before. :)

It does use typeof =)

> This does seem expensive for the case when it is implemented, sicne you get the
> cookie array twice. Maybe you can store the cookies here for use later. Or
> delate this until the first update is done?

Since there is no event when cookies change, InspectorController.cookies() is called every time the datagrid is refreshed/updated, this way if some javascript on the page added/removed a cookie, it would be reflected in the next DataGrid update.  If I store it here, when the Panel is created, if any cookies were added via the Console or the Page it wouldn't be reflected in the first draw (when the user first clicks the Cookie Sidebar Section).

> > +            cookies.push(new WebInspector.Cookie(cookie.name, cookie.value, cookie.domain, cookie.path, cookie.expires, cookie.size, cookie.httpOnly, cookie.secure, cookie.session));
> 
> We don't need to use WebInspector.Cookie except for the fallback case. You can
> just directly use the cookie objects in the aray.
> 
> > +                cookies.push(new WebInspector.Cookie(name, value, null, null, null, size));
> 
> Maybe we can remove WebInspector.Cookie and just make inline objects to fake a
> cookie. So:
> 
> cookies.push({name: name, value: value});

Sounds good, I'll remove WebInspector.Cookie.

> > +        if (String([cookie name]) == cookieName) {
> 
> This should be written the other way so you don't convert each cookie's name to
> C++.
> 
> Convert cookieName once to NSString * before the loop. Then do:
> 
> if ([[cookie name] isEqualToString:cookieNameString])

Okay.  I'll take a swing at this!

[Joseph Pecoraro]

Created attachment 34807 [details]
Fixed based on Review

CHANGES:

- Removed WebInspector.Cookie
  - git rm WebCore/inspector/front-end/Cookie.js
  - removed from inspector.html, and WebKit.qrc

- Casted to (NSString *), it looks like PlatformString.h allowed for that with "operator NSString*()"

[Joseph Pecoraro]

(In reply to comment #7)
> > This does seem expensive for the case when it is implemented, sicne you get the
> > cookie array twice. Maybe you can store the cookies here for use later. Or
> > delate this until the first update is done?

Didn't do this. The extra check can be removed once other ports implement it and the fallback behavior is not needed.

> > Maybe we can remove WebInspector.Cookie and just make inline objects to fake a
> > cookie.

Done.

> > Convert cookieName once to NSString * before the loop. Then do:
> > 
> > if ([[cookie name] isEqualToString:cookieNameString])

Done.

[Eric Seidel (no email)]

Comment on attachment 34807 [details]
Fixed based on Review

Rejecting patch 34807 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34807 from bug 28269 failed to download and apply.

[Joseph Pecoraro]

> This patch will require manual commit.

I was just typing how this would fail due to a git binary diff (localizedStrings)!!  If someone would be kind enough to land this I'd greatly appreciate it.  I'm confident this won't produce a build error.  Otherwise I'll try to get the ball rolling when I get back next week.

[Adam Barth]

I'll try.

[Adam Barth]

I lied.  I'm to go to sleep.  If this patch is still around next time I have some cycles, I'll land it.  Someone else should feel free to land it in the meantime.

[Joseph Pecoraro]

I'm back!  This still applies cleanly to ToT (other then ChangeLogs).  Anyone have the time?

[Joseph Pecoraro]

Landed in r47363 thanks to ariya!
http://trac.webkit.org/changeset/47363

[Pavel Feldman]

Sorry for jumping in late. Few comments from my side:

- I don't see why JSValue JSInspectorBackend::deleteCookie needs to be custom. It can be generated.

- JSValue JSInspectorBackend::cookies can also be implemented in InspectorBackend and only have custom accessors (see InspectorBackend::highlight). Otherwise produces too much duplicated code.

- InspectorController.cookies() should not be called synchronously from the frontend. It is actually not clear how it relates to InjectedScript.getCookies() that is asynchronous. Should latter call into it?

[Joseph Pecoraro]

> - I don't see why JSValue JSInspectorBackend::deleteCookie needs to be custom.
> It can be generated.
> 
> - JSValue JSInspectorBackend::cookies can also be implemented in
> InspectorBackend and only have custom accessors (see
> InspectorBackend::highlight). Otherwise produces too much duplicated code.

This sounds like a good idea.  I'm rather new to this and I don't know the differences between Custom and Non-Custom bindings.  It seems here that a non-Custom is better, and if that is the case then I'm all for it.  Can you point me to some reading, or some code to read, that would help me understand IDL and how WebKit handles bindings?

> - InspectorController.cookies() should not be called synchronously from the
> frontend. It is actually not clear how it relates to
> InjectedScript.getCookies() that is asynchronous. Should latter call into it?

That is a good point.  Right now, InjectedScript.getCookies() is only there for the fallback behavior, by which I mean for platforms that haven't implemented CookieJar's getRawCookies().  You make a good point about making it async.

I can look into renaming the current version as InjectedScript.fallbackGetCookies or something similar and make the current version async with the clearer name.  The end goal is to have all the platforms (or most) implement the new cookies functionality and all of the fallback code can then be removed, but in the meantime they can still get some data (name/value pairs) via the inspected page's document.cookie string.

[Pavel Feldman]

(In reply to comment #17)
> > - I don't see why JSValue JSInspectorBackend::deleteCookie needs to be custom.
> > It can be generated.
> > 
> > - JSValue JSInspectorBackend::cookies can also be implemented in
> > InspectorBackend and only have custom accessors (see
> > InspectorBackend::highlight). Otherwise produces too much duplicated code.
> 
> This sounds like a good idea.  I'm rather new to this and I don't know the
> differences between Custom and Non-Custom bindings.  It seems here that a
> non-Custom is better, and if that is the case then I'm all for it.  Can you
> point me to some reading, or some code to read, that would help me understand
> IDL and how WebKit handles bindings?
> 

You should only use Custom when you need to produce JSC (or V8) specific calls. This includes methods that receive / return DOMObject values in the IDL. In case of your methods you only talk to WebCore and wrap the results in a general way. You should place all code into InspectorBackend, wrap these results into Script(Object|Array|Value) objects and provide small accessors in the cutom classes that would convert JSObject (or v8::Object) into the ScriptObject instances. See InspectorBackend.(un)?wrapObject and how they are being accessed from the custom bindings for reference.

> > - InspectorController.cookies() should not be called synchronously from the
> > frontend. It is actually not clear how it relates to
> > InjectedScript.getCookies() that is asynchronous. Should latter call into it?
> 
> That is a good point.  Right now, InjectedScript.getCookies() is only there for
> the fallback behavior, by which I mean for platforms that haven't implemented
> CookieJar's getRawCookies().  You make a good point about making it async.
> 
> I can look into renaming the current version as
> InjectedScript.fallbackGetCookies or something similar and make the current
> version async with the clearer name.  The end goal is to have all the platforms
> (or most) implement the new cookies functionality and all of the fallback code
> can then be removed, but in the meantime they can still get some data
> (name/value pairs) via the inspected page's document.cookie string.

Actually, things can be even simpler. Here is how I would do this:
- Introduce non-Custom  InspectorBackend::getCookies that would make all necessary computations and call InspectorFrontend::setCookies(String, ScriptObject) that you introduce. Other InspectorFrontend methods for the reference + InspectorResource on how to create a ScriptObject with your data.
- Implement WebInspector.setCookies that would receive and process the data.
- Make a decision on whether to use real cookies or fallback ones right in the InspectorBackend::getCookies().
- Introduce non-Custom InspectorBackend::deleteCookie and do all the same except for it does not need a callback (or does it?).

Anyway, following this pattern you won't need any [Cutom] things at all, you will not need to touch InjectedScript either. InjectedScript is really for the operations that are easier to implement in JavaScript. In your case all the real work is done in C++, so you don't need it.

[Joseph Pecoraro]

> Actually, things can be even simpler. Here is how I would do this:
> - Introduce non-Custom  InspectorBackend::getCookies that would make all
> necessary computations and call InspectorFrontend::setCookies(String,
> ScriptObject) that you introduce. Other InspectorFrontend methods for the
> reference + InspectorResource on how to create a ScriptObject with your data.
> - Implement WebInspector.setCookies that would receive and process the data.

This is certainly a different way of thinking for me.  I'd just rather call a function and get a result.  This approach would mean storing the cookie data somwhere (most likely in the WebInspector namespace), whenever getCookies is called.  I'd rather just call getCookies and get a return value with the data.  What is the advantage to calling a function over just returning a value?

[Pavel Feldman]

(In reply to comment #19)
> > Actually, things can be even simpler. Here is how I would do this:
> > - Introduce non-Custom  InspectorBackend::getCookies that would make all
> > necessary computations and call InspectorFrontend::setCookies(String,
> > ScriptObject) that you introduce. Other InspectorFrontend methods for the
> > reference + InspectorResource on how to create a ScriptObject with your data.
> > - Implement WebInspector.setCookies that would receive and process the data.
> 
> This is certainly a different way of thinking for me.  I'd just rather call a
> function and get a result.  This approach would mean storing the cookie data
> somwhere (most likely in the WebInspector namespace), whenever getCookies is
> called.  I'd rather just call getCookies and get a return value with the data. 
> What is the advantage to calling a function over just returning a value?

Well, you need to pass the result within some callback to make the call asynchronous. If you don't want to store your state / data in between the call and response, look at DOMAgent.js. Here is a snippet you might be interested in:

WebInspector.Cookies.prototype = {
    doAsyncWork: function() {
        function mycallback(results, are, here) {
            // process the result here
        }
        var callId = WebInspector.Callback.wrap(mycallback);
        InspectorController.getCookies(callId, parameters, go, here);
    }
}
WebInspector.didGetCookies = WebInspector.Callback.processCallback;

Then you add 'didGetCookies' into InspectorFrontend and call it from backend with the same callId you are getting.

[Joseph Pecoraro]

> Well, you need to pass the result within some callback to make the call
> asynchronous.

Okay, so the advantage is to make this async.  I wasn't too worried about that, but its probably better to do that now rather then later.

> If you don't want to store your state / data in between the call
> and response, look at DOMAgent.js. Here is a snippet you might be interested
> in:
> 
> WebInspector.Cookies.prototype = {
>     doAsyncWork: function() {
>         function mycallback(results, are, here) {
>             // process the result here
>         }
>         var callId = WebInspector.Callback.wrap(mycallback);
>         InspectorController.getCookies(callId, parameters, go, here);
>     }
> }
> WebInspector.didGetCookies = WebInspector.Callback.processCallback;
> 
> Then you add 'didGetCookies' into InspectorFrontend and call it from backend
> with the same callId you are getting.

Okay, the way that I see this right now, I'll have something like:

  InspectorBackend.getCookies(callId) // Starting point in IDL
   -> InspectorController.getCookies(callid) // To get the Document for cookiesUrl
    -> InspectorDOMAgent.getCookies(callid, doc) // Deal with the CookieJar
     -> InspectorFrontEnd.didGetCookies(callid, data) // Data to send the callback
      -> WebInspector.didGetCookies(callid, data) // Run the JS callback function

It seems like that callstack is a bit over the top but it keeps consistent with current conventions.  Does this look okay?

I'll also need to turn a Vector<Cookie> into a ScriptObject. There are a number of "buildArray..." in InspectorDOMAgent that I could emulate to build an array of cookie objects.  Its actually much easier to work with ScriptObject and ScriptArray then the Custom implementations, I wish I had gone this route before.

Thanks for the guidance so far Pavel, I really appreciate it.

[Pavel Feldman]

Sounds good.

>   InspectorBackend.getCookies(callId) // Starting point in IDL
>    -> InspectorController.getCookies(callid) // To get the Document for
> cookiesUrl

You can skip this one and talk to m_domAgent from within InspectorBackend.cpp. They are friends with InspectorController. It is more of a convention thingy that declares InspectorController's interface to the frontend.

[Joseph Pecoraro]

Created attachment 35133 [details]
Non-Custom Functions

- Removed the [Custom] IDL bindings and instead made simpler Non-Custom bindings that are handled in the Inspector*.cpp files.
- Converted CookiesItemView.js to asynchronously load Cookies via the DOMAgent

[Timothy Hatcher]

Pavel, can you take a look at this patch?

[Joseph Pecoraro]

This should hopefully cover Pavel's comments, but I didn't put it up for review, I'm typing a longer comment (to show up in a minute or so).

[Joseph Pecoraro]

I didn't set that patch as r? because I had some other things that I was
wondering about:

Another developer emailed me with some feedback about the "webkit cookie api". 
He listed a number of good points.  I think I should work on cleaning up and
drafting a good cookie api now, rather then adding it piecewise like I have
been doing.  Some of the suggestions that came out of our email exchange where:

- Change the name of "getRawCookies" to something better like "cookiesList",
and apparently returning a bool is not consistent with other sections.  This
seems more appropriate:

void cookiesList(const Document* document, const KURL& url, Vector<Cookie>&
cookiesList, bool& implemented = false);

- Change the name of "deleteCookie" to "deleteCookieByName" which is more
correct.  I think this is also a good idea.

- It would be nice to prepare for adding support for Creating/Modifying
cookies.  This behavior must already be possible via CookieJar's already
existing setCookies(.., .., String).  I think it would be appropriate to
provide an addCookie convenience function and possibly updateCookieByName.  Or
should these be avoided in favor of specific calls to setCookies?

- He raised a good point as well => "On this topic what is the goal of the
Cookie.h class?  The file didn't contain any comments.  Is it just for the
inspector currently?  Any long term plans?  It would be nice to have a unified
string -> cookie parser for all webkit as it is such as messy job and every
port does it a tiny bit differently."

Any ideas if there could be plans to "unify string cookie parsing" rather then
the current platform specific  mess, or are things too coupled to the network
stacks?

[Pavel Feldman]

This looks so much better from the InspectorController interaction perspective. Couple of comments below:


> +    m_inspectorController->domAgent()->getCookies(callId, m_inspectorController->m_inspectedPage->mainFrame()->document());

domAgent has private mainFrameDocument() method.

> +    value.set("size", (int)(cookie.name.length() + cookie.value.length()));

not sure if WebKit source base allows casts - leaving for Timothy's advice

> +ScriptArray InspectorDOMAgent::buildArrayForCookies(const Vector<Cookie>& cookiesList)

Here and in other places: I try to maintain same order of methods in .h and .cpp


>  
> +void InspectorFrontend::didGetCookies(int callId, const ScriptArray& cookies)
> +void InspectorFrontend::didGetCookies(int callId, const String& cookiesString)

There is no overloading in JavaScript, while InspectorFrontend is supposed to reflect the JavaScript API. I'd either call these differently, or if you are bound to a single name due the callback nature, I would use didGetCookies(int, String, ScriptArray) signature and get rid of extra
boolean flag. In javascript it would be if (cookies) { good code } else  { fallback(cookiesString); }

[Joseph Pecoraro]

Created attachment 35140 [details]
Fixed Based on Pavel's Comments

[Joseph Pecoraro]

(In reply to comment #27)
> This looks so much better from the InspectorController interaction perspective.

Hehe, for me this felt like more work jumping between multiple files with duplication.  But, at least there is no specific bindings, so it works immediately with v8.

> domAgent has private mainFrameDocument() method.

Done. Can't believe I missed that.

> not sure if WebKit source base allows casts - leaving for Timothy's advice

Waiting for comment.

> Here and in other places: I try to maintain same order of methods in .h and
> .cpp

Done. I reordered all my new functions such that if the function comes after X the header, it comes after X in the implementation.

> > +void InspectorFrontend::didGetCookies(int callId, const ScriptArray& cookies)
> > +void InspectorFrontend::didGetCookies(int callId, const String& cookiesString)
> 
> There is no overloading in JavaScript, while InspectorFrontend is supposed to
> reflect the JavaScript API. I'd either call these differently, or if you are
> bound to a single name due the callback nature, I would use didGetCookies(int,
> String, ScriptArray) signature and get rid of extra
> boolean flag. In javascript it would be if (cookies) { good code } else  {
> fallback(cookiesString); }

Done. I did keep the boolean when it actually gets to the CookieItemsView rather then inspecting the Cookie Data itself.

[Pavel Feldman]

Thanks for fixing that.
> 
> Hehe, for me this felt like more work jumping between multiple files with
> duplication.  But, at least there is no specific bindings, so it works
> immediately with v8.
> 

It is more important that we follow the same pattern. InspectorController's interaction will get serialized through the JSON soon, InjectedScript will get into an isolated context. It'll be much easier for me to refactor the code now!

[Timothy Hatcher]

Comment on attachment 35140 [details]
Fixed Based on Pavel's Comments


> +    value.set("size", (int)(cookie.name.length() + cookie.value.length()));

Casting should use C++ casts, like static_cast<int>(cookie.name.length() + cookie.value.length()).

Should this be long?

[Joseph Pecoraro]

(In reply to comment #31)
> (From update of attachment 35140 [details])
> 
> > +    value.set("size", (int)(cookie.name.length() + cookie.value.length()));
> 
> Casting should use C++ casts, like static_cast<int>(cookie.name.length() +
> cookie.value.length()).
> 
> Should this be long?

From this Spec:
http://www.w3.org/Protocols/rfc2109/rfc2109

Implementation Limits:
- at least 4096 bytes per cookie (as measured by the size of the characters that comprise the cookie non-terminal in the syntax description of the Set-Cookie header)

As far as I know most browsers keep this 4k limit.  Developers cannot depend on any more. So an int should be just fine.  The arguments in this case I believe are "unsigned".

[Timothy Hatcher]

Ok, makes sesne. You should use unsigned in your cast unless the set function prevents it.

[Joseph Pecoraro]

Created attachment 35146 [details]
Fixed the Cast

Fixed the cast.

Any comments on Comment 26?

[Benjamin Meyer]

Another thing is that if you compile webkit without inspector these functions (and cookie.h) should probably not be included as they are inspector specific.  Wrapping it in the ifdef would be good.

[Joseph Pecoraro]

(In reply to comment #35)
> Another thing is that if you compile webkit without inspector these functions
> (and cookie.h) should probably not be included as they are inspector specific. 
> Wrapping it in the ifdef would be good.

I don't think there are any generic ifdefs for the inspector. Although maybe there should be if anyone ever wants to build WebKit without the Inspector (small gains?).

Does this patch need anything else?  Any more comments on the API?  Is it worth putting in the work?

[Timothy Hatcher]

Comment on attachment 35146 [details]
Fixed the Cast

Nice clean up.

[Joseph Pecoraro]

- Added the localizedStrings that were supposed to have landed in the first patch but most have gotten lost when ariya did the commit.
- Made "Session" a localizedString as well.  Donno how I missed that before.

Should I close this bug?

---

Landed in r47656 = f0c84dbeb8261b637a75a601fd799530971aa3e5
http://trac.webkit.org/changeset/47656