Chromium already has an alternate WebSocket-based communication channel with the backend. Upstreaming it in this change. We will agree on the URI of the remote service as the protocol matures.
Created attachment 64338 [details] [PATCH] Proposed change.
Comment on attachment 64338 [details] [PATCH] Proposed change. WebCore/inspector/front-end/inspector.js:434 + var queryParams = window.location.search; Please wrap it into an anonymous function to avoid cluttering global context. WebCore/inspector/front-end/inspector.js:445 + WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); } Why not do this unconditionally after WebInspector has been loaded?
Comment on attachment 64338 [details] [PATCH] Proposed change. > +++ b/WebCore/inspector/front-end/inspector.js > + WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); } > +WebInspector_syncDispatch = function(message) What is the reason this uses an "_" and not "."? Is it so the backend can access it directly off of the global scope without requiring messy property lookups? > v8::Handle<v8::Value> dispatchFunction = frameContext->Global()->Get(v8::String::New("WebInspector_syncDispatch")); If so, it could use a comment in the code, along the lines of: "// This function is purposely put into the global scope for easy access." > + WebInspector.socket.onopen = function() { > + ... > + WebInspector.socketOpened = true; > + if (WebInspector.loadedDone) > + WebInspector.doLoadedDone(); > + }; > +} > + > WebInspector.loaded = function() > { > + if ("page" in WebInspector.queryParamsObject) { > + WebInspector.loadedDone = true; > + if (WebInspector.socketOpened) > + WebInspector.doLoadedDone(); > + return; > + } > + WebInspector.doLoadedDone(); > +} If I understand this correctly, this could potentially call doLoadedDone() twice if WebInspector.socket.onopen gets called before WebInspector.loaded gets called. Is that realistic? I would have thought both of the "if" statements above would be exclusive, so if (!...).
(In reply to comment #2) > (From update of attachment 64338 [details]) > WebCore/inspector/front-end/inspector.js:434 > + var queryParams = window.location.search; > Please wrap it into an anonymous function to avoid cluttering global context. > Done. > WebCore/inspector/front-end/inspector.js:445 > + WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); } > Why not do this unconditionally after WebInspector has been loaded? Done. Was not feasible before, but now communication starts post-load, so should be fine. (In reply to comment #3) > What is the reason this uses an "_" and not "."? Is it so the backend can access > it directly off of the global scope without requiring messy property lookups? > If so, it could use a comment in the code, along the lines of: > > "// This function is purposely put into the global scope for easy access." Done. > > If I understand this correctly, this could potentially call doLoadedDone() > twice if WebInspector.socket.onopen gets called before WebInspector.loaded > gets called. Is that realistic? I would have thought both of the "if" > statements above would be exclusive, so if (!...). The two flags (loadedDone and socketOpened) were supposed to guarantee single invocation. But given the above comments, this complexity goes away.
Created attachment 64447 [details] [PATCH] Review comments addressed.
Comment on attachment 64447 [details] [PATCH] Review comments addressed. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog > + Web Inspector: upstream frontend-side WebSocket transport. > + Chromium already has an alternate WebSocket-based communication channel with > + the backend. Upstreaming it in this change. We will agree on the URI > + of the remote service as the protocol matures. > + https://bugs.webkit.org/show_bug.cgi?id=43970 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. > +function parseQueryParameters() > +{ > + ... > +} > +WebInspector.queryParamsObject = parseQueryParameters(); Now this just puts the function name "parseQueryParameters" in the global namespace. An anonymous function might work best because right now the function is only used once. There are a number of choices, but I don't think this addressed Yury's concern. > +function installWebSocketTransport() > +{ > + WebInspector.socket = new WebSocket("ws://" + window.location.host + "/devtools/page/" + WebInspector.queryParamsObject.page); > + WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); } > + WebInspector.socket.onerror = function(error) { console.err(error); } I agree with the console.err(). It might also be worth a WebInspector.log to warn the user an attempt was made at a connection and that attempt failed. Currently it looks like there would be no user indication. > + WebInspector.socket.onopen = function() { > + ... > + }; NIT: Semicolon not needed here. I think we should add this as a style rule, to not add these. I'm guilty of it myself all the time too. > +//This function is purposely put into the global scope for easy access. NIT: Missing space. "// This..." > +WebInspector_syncDispatch = function(message) > +{ > + ... > +}; NIT: Semicolon not needed. Looks good to me, just I don't think you completely addressed Yury's comment.
Joe, thanks for the thorough review (as usually). > 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 ?!?! > Now this just puts the function name "parseQueryParameters" > in the global namespace. An anonymous function might work > best because right now the function is only used once. There > are a number of choices, but I don't think this addressed > Yury's concern. > 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. > > I agree with the console.err(). It might also be worth a WebInspector.log > to warn the user an attempt was made at a connection and that attempt > failed. Currently it looks like there would be no user indication. > 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). > NIT: Semicolon not needed here. I think we should add this as a style rule, > to not add these. I'm guilty of it myself all the time too. > Done. > NIT: Missing space. "// This..." > Done. > NIT: Semicolon not needed. > Done. > > Looks good to me, just I don't think you completely > addressed Yury's comment. Will upload new one shortly.
Created attachment 64449 [details] [PATCH] Proposed change.
Comment on attachment 64449 [details] [PATCH] Proposed change. Self-review notes: inspector.js:430 > (function parseQueryParameters() > 431 { This thing does not work unless I put semicolon after WebInspector.PlatformFlavor = { ... MacSnowLeopard: "mac-snowleopard" };
Created attachment 64450 [details] [PATCH] Proposed change.
Comment on attachment 64450 [details] [PATCH] Proposed change. > > 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.
> 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. That would be a nice rule to follow (and is in fact standard to Google), but WebInspector traditionally did not set semicolon after assigning to the prototype. Time to change the tradition?
Comment on attachment 64450 [details] [PATCH] Proposed change. Clearing flags on attachment: 64450 Committed r65415: <http://trac.webkit.org/changeset/65415>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/65415 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/65414 http://trac.webkit.org/changeset/65415
> That would be a nice rule to follow (and is in fact standard to Google), but > WebInspector traditionally did not set semicolon after assigning to the > prototype. Time to change the tradition? You're right. I hate special cases but I like tradition on this one.