[webkit-reviews] review granted: [Bug 43970] Web Inspector: upstream frontend-side WebSocket transport. : [Attachment 64450] [PATCH] Proposed change.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 43970: Web Inspector: upstream frontend-side WebSocket transport.
https://bugs.webkit.org/show_bug.cgi?id=43970

Attachment 64450: [PATCH] Proposed change.
https://bugs.webkit.org/attachment.cgi?id=64450&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> > NIT: I should have commented on this last iteration. This is not the
> > usual formatting? "Title \n Bug \n \n Description" is much easier to read.
> > 
> 
> Done. Ok, I now glue Title and Bug with no \n\n and even that is not enough
?!?!

This is perfect. This is how I'd say 90% of ChangeLogs are formatted.
And its what `prepare-ChangeLog`encourages.

In the past, you have been putting an extra line between the title and
bug link. The `prepare-ChangeLog` script does not, so I try and keep
consistent with that. I commented on how you did it in the past =),
but I remember Tim saying he liked your way better. Either way is
fine, but I think the description should really be separate from the
title/description.


> I know. It is just that we never practiced anonymous closures for
> hiding functions in WebKit (see preloadImages). But if everyone
> agrees on the practice, I am happy to start using it. Migrated
> preloadImages as well.

True. I think there is only a few usages. I prefer them. I like keeping
the global scope clean, and not storing a reference to 1-off functions.

I had one final thought! parseQueryParameters should probably
use window.decodeURIComponent for pair[1]!


> We will need to wire complete error handling to the new protocol.
> We will need to consider ui indication for it (such as connection indicator
> or something).

Gotcha. If you think its appropriate, please file a follow-up bug. Or, just
keep it in mind as I'm sure it will be addressed later.

Thanks for addressing the comments. r=me

-------------

> This thing does not work unless I put semicolon after
> 
> WebInspector.PlatformFlavor = {
>     ...
>     MacSnowLeopard: "mac-snowleopard"
> };

Interesting. How about this for the semicolon rule:

  Semicolons SHOULD be used for object literals:

    var obj = { ... };
    some.obj = {
      ...
    };

  But SHOULD NOT be used for closing scopes like
  functions or anonymous scopes (which I don't think
  we use at all currently).

Let me know if that misses anything.