Embeddable Developer Tools
Created attachment 152793 [details] Patch
Comment on attachment 152793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152793&action=review > Source/WebCore/inspector/front-end/InspectorEmbedded.js:5 > +var domContentLoaded = function() { > + InspectorFrontendAPI.showTimeline(); This needs to be more generic -- e.g. allow various "commands" passed via query string. > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:39 > + this._hiddenPanels = []; I don't think this belongs to InspectorFrontendHostStub -- this is supposed to be stub for InspectorFornendHost, after all. > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:120 > + addHiddenPanel: function(panel) > + { > + if (this._hiddenPanels.indexOf(panel) == -1) > + this._hiddenPanels.push(panel); > + }, > + > + removeHiddenPanel: function(panel) > + { > + var panelIndex = this._hiddenPanels.indexOf(panel); > + if (panelIndex != -1) > + this._hiddenPanels.splice(panelIndex,1); > }, Ditto. > Source/WebCore/inspector/front-end/TimelineModel.js:169 > + loadRecords: function(records) Here and below, please add JSDoc annotations. > Source/WebCore/inspector/front-end/TimelinePanel.js:778 > + if(xhr.status == 200) { style: if (xhr.status === 200) also, is there a reason for this to be synchronous? > Source/WebCore/inspector/front-end/TimelinePanel.js:779 > + var data = JSON.parse(xhr.responseText); I think we need some diagnostics in case parsing fails. > Source/WebCore/inspector/front-end/inspector.html:224 > + <script type="text/javascript" src="InspectorEmbedded.js"></script> You will have to add this to several project files -- please look up changes that added other files recently. > Source/WebCore/inspector/front-end/inspector.js:415 > } > + else if (InspectorFrontendHost.isStub) { style: "} else" need to be on the same line. > Source/WebCore/inspector/front-end/inspector.js:419 > + var hiddenPanels = WebInspector.queryParamsObject["hiddenPanels"].split(","); > + for (var i = 0; i < hiddenPanels.length; ++i) > + InspectorFrontendHost.addHiddenPanel(hiddenPanels[i]); nit: WebInspector.queryParamsObject["hiddenPanels"].split(",").forEach(InspectorFrontendHost.addHiddenPanel, InspectorFrontendHost)
Created attachment 153026 [details] Patch
What do you recommend doing/where do you recommend putting the hidden panels? In the builtin mode it will look for the hidden panels in InspectorFrontend so wouldn't it make sense for it to come from the stub in the embedded case?
Comment on attachment 153026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153026&action=review > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:111 > + if (this._hiddenPanels.indexOf(panel) == -1) NB: == => === > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:118 > + if (panelIndex != -1) NB: != => !== > Source/WebCore/inspector/front-end/TimelineModel.js:170 > + * @param {Object[]} records Please run Source/WebCore/inspector/compile-front-end.py (you would need a closure compiler as ~/closure/compiler.jar). The proper way to specify an array of objects is {Array.<Object>} > Source/WebCore/inspector/front-end/TimelineModel.js:225 > + console.error(e.message); This needs to be shown to the inspector user, so please use WebInspector.log(). Also, please add some test to help users to understand the error, e.g. WebInspector.UIString("Failed to load timeline log: %s", e.message); > Source/WebCore/inspector/front-end/TimelineModel.js:226 > + return null; Just "return;"? > Source/WebCore/inspector/front-end/inspector.js:47 > + var hiddenPanels = InspectorFrontendHost.hiddenPanels(); How about just: var hiddenPanelsString = InspectorFrontendHost.hiddenPanels() || ""; if (WebInspector.queryParamsObject["hiddenPanels"]) hiddenPanelsString += "," + WebInspector.queryParamsObject["hiddenPanels"]; var hiddenPanels = hiddenPanelsString.split(",") > Source/WebCore/inspector/front-end/inspector.js:420 > + WebInspector.inspectorView.setCurrentPanel(WebInspector.firstPanel()); There's InspectorView._panelOrder that may be used to determine first panel. Also, in case we have multiple panels, we should probably open that last used one. > Source/WebCore/inspector/front-end/inspector.js:421 > + if (typeof WebInspector.queryParamsObject["url"] !== "undefined") We may want to load other files as well, so using just a presence of "url" parameter to trigger loading of timeline does not look fair. How about having something like command=openTimeline&url=foo, then have panels register command handlers for particular values of command parameter? > Source/WebCore/inspector/front-end/inspector.js:582 > +WebInspector.firstPanel = function() As above, this does not seem necessary.
Created attachment 154992 [details] Patch
Comment on attachment 154992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154992&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You should only have one ChangeLog entry. It should explain what changes as why here. > Source/WebCore/inspector/front-end/InspectorView.js:76 > + if (typeof x === "number") x should be a panel. We should annotate it with @param {WebInspector.Panel} x. > Source/WebCore/inspector/front-end/TimelineModel.js:172 > + loadRecords: function(records) Since this is private to this file, should be _loadRecords > Source/WebCore/inspector/front-end/TimelineModel.js:211 > + loadFromUrl: function(url) WebKit uses all caps for abbreviations (loadFromURL) > Source/WebCore/inspector/front-end/TimelineModel.js:214 > + var xhr = new XMLHttpRequest(); You should call InspectorFrontendHost.loadResourceSynchronously here and move this implementation into the InspectorFrontendHostStub. > Source/WebCore/inspector/front-end/TimelineModel.js:216 > + xhr.onreadystatechange = dataReceived; We don't use self = this method, we bind callbacks to this instead. i.e. xhr.onreadystatechange = dataReceived.bind(this); > Source/WebCore/inspector/front-end/TimelineModel.js:221 > + if (xhr.status === 200) { Prefer guard expressions and early returns. > Source/WebCore/inspector/front-end/TimelinePanel.js:776 > + loadFromUrl: function(url) loadFromURL > Source/WebCore/inspector/front-end/inspector.js:47 > + var hiddenPanelsString = InspectorFrontendHost.hiddenPanels() || ""; Move this code into InspectorFrontendHostStub's hiddenPanels method. > Source/WebCore/inspector/front-end/inspector.js:417 > + } else if (InspectorFrontendHost.isStub) { WebKit does not put "else" after a branch with "return". Just start with if (... on the next line. > Source/WebCore/inspector/front-end/inspector.js:419 > + WebInspector.inspectorView.setCurrentPanel(0); What if there are no panels? Instead of using 0, you should expose panels: function() on InspectorView that would return this._panelOrder.slice(). Then you would be able to fetch panels from here and select the first one if any. > Source/WebCore/inspector/front-end/inspector.js:420 > + if (typeof WebInspector.queryParamsObject["timelineUrl"] !== "undefined") There should be no special casing for timeline in here. I would put the code below into the InspectorFrontendAPI::loadTimelineFromURL and would dispatch query parameters into calls on InspectorFrontendAPI in some generic manner. In this case, it would be: inspector.html?callAPI=loadTimelineURL¶m=<url> or similarly.
Created attachment 155315 [details] Patch
Comment on attachment 155315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155315&action=review > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:132 > + loadTimelineFromURL: function(url) { style: { needs to be on a line by its own. Also, please annotate. > Source/WebCore/inspector/front-end/TimelineModel.js:213 > + var data = InspectorFrontendHost.loadResourceSynchronously(url); I think that this should better be async -- timeline logs happen to be huge. Also, we need some diagnostics shown to the user when loading fails. > Source/WebCore/inspector/front-end/TimelineModel.js:215 > + data = JSON.parse(data); I think using data for both string representation and parsed object is quite unfortunate. > Source/WebCore/inspector/front-end/TimelineModel.js:218 > + WebInspector.log(WebInspector.UIString('Failed to load timeline data with error: %s', e.message)); We prefer double quotes on strings. > Source/WebCore/inspector/front-end/TimelineModel.js:219 > + return; Redundant > Source/WebCore/inspector/front-end/inspector.js:417 > + WebInspector.inspectorView.setCurrentPanel(WebInspector.inspectorView.getPanel(0)); Do we really need this? > Source/WebCore/inspector/front-end/inspector.js:419 > + var callFunction = window["InspectorFrontendAPI"][WebInspector.queryParamsObject["callAPI"]]; Why not just InspectorFrontendAPI[WebInspector.queryParamsObject["callAPI"]]?
Comment on attachment 155315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155315&action=review Looks good overall, some nits inline. > Source/WebCore/inspector/front-end/InspectorView.js:105 > + getPanel: function(index) WebKit suggests that this method is called panel: But I don't think you need it, especially given there is no length getter. Just make sure that inspector selects the first available panel. >> Source/WebCore/inspector/front-end/TimelineModel.js:213 >> + var data = InspectorFrontendHost.loadResourceSynchronously(url); > > I think that this should better be async -- timeline logs happen to be huge. Also, we need some diagnostics shown to the user when loading fails. We can't make the host method asynchronous, so if you end up using async behaviour, you should introduce loadXHR(url, sync, callback) in utilities.js. Please leave stub for InspectorFrontendHost as you implemented it though. >> Source/WebCore/inspector/front-end/TimelineModel.js:218 >> + WebInspector.log(WebInspector.UIString('Failed to load timeline data with error: %s', e.message)); > > We prefer double quotes on strings. Also, you want to add this string into English.lproj/localizedStrings.js >> Source/WebCore/inspector/front-end/inspector.js:419 >> + var callFunction = window["InspectorFrontendAPI"][WebInspector.queryParamsObject["callAPI"]]; > > Why not just InspectorFrontendAPI[WebInspector.queryParamsObject["callAPI"]]? You could move this implementation into the InspectorFrontendAPI.js itself. Such as InspectorFrontendAPI.dispatchQueryCommands(). And I was also hoping that you could execute multiple commands with arbitrary parameter sets.
Created attachment 155817 [details] Patch
Comment on attachment 155817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155817&action=review > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:138 > + WebInspector.panels.timeline.loadFromURL(url); This could probably force switch to timeline panel. > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:108 > + var hiddenPanels = ""; > + if (WebInspector.queryParamsObject["hiddenPanels"]) > + hiddenPanels += "," + WebInspector.queryParamsObject["hiddenPanels"]; > + return hiddenPanels; return WebInspector.queryParamsObject["hiddenPanels"] || ""? Leading "," definitely appears redundant in this case. > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:189 > + var xhr = new XMLHttpRequest(); > + xhr.open("GET", url, false); > + xhr.send(null); > + if (xhr.status === 200) > + return xhr.responseText; so you've added loadXHR()... > Source/WebCore/inspector/front-end/InspectorView.js:105 > + getPanel: function(index) Do we really need this? > Source/WebCore/inspector/front-end/inspector.js:417 > + WebInspector.inspectorView.setCurrentPanel(WebInspector.inspectorView.getPanel(0)); I would rely in InspectorView choosing first panel on its own. > Source/WebCore/inspector/front-end/utilities.js:710 > + WebInspector.log(WebInspector.UIString("Synchronous resource load failed with XMLHttpRequest status %d", xhr.status)); I don't think we have to mention whether the XHR was async or sync in the error message -- especially that this is not always true with this implementation. > Source/WebCore/inspector/front-end/utilities.js:711 > + callback(""); It might be good idea to indicate error with something that outside of normal value range for the result -- e.g. use null or undefined instead of string here. > Source/WebCore/inspector/front-end/utilities.js:712 > + return; Redundant. > Source/WebCore/inspector/front-end/utilities.js:718 > + if (sync === false) I'd suggest to normalize sync first, then use it as a strict bool type, e.g. if (typeof sync === "undefined") sync = true; etc > Source/WebCore/inspector/front-end/utilities.js:725 > + WebInspector.log(WebInspector.UIString("Synchronous resource load failed with XMLHttpRequest status %d", xhr.status)); As above -- please drop "synchronous". > Source/WebCore/inspector/front-end/utilities.js:726 > + return ""; As above -- please return null or undefined here.
Created attachment 155833 [details] Patch
Comment on attachment 155817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155817&action=review >> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:189 >> + return xhr.responseText; > > so you've added loadXHR()... Pavel asked me to leave this one as is. > Source/WebCore/inspector/front-end/utilities.js:703 > +function loadXHR(url, sync, callback) { Style error { not on new line fixed in next path
Comment on attachment 155833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155833&action=review This change is missing the ChangeLog. > Source/WebCore/inspector/front-end/inspector.js:418 > + InspectorFrontendAPI.dispatch([WebInspector.queryParamsObject["callAPI"],WebInspector.queryParamsObject["params"]]) space after ",". I would also name it dispatchQueryParameters and move queryParamsObject along with the constants into the InspectorFrontendAPI > Source/WebCore/inspector/front-end/utilities.js:703 > +function loadXHR(url, sync, callback) { XHR is typically using "async" parameter. > Source/WebCore/inspector/front-end/utilities.js:720 > + xhr.open("GET", url, false); false -> async ? > Source/WebCore/inspector/front-end/utilities.js:721 > + if (sync === false) if (async) > Source/WebCore/inspector/front-end/utilities.js:724 > + if (sync === true) { else or if (!async)
Comment on attachment 155833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155833&action=review >> Source/WebCore/inspector/front-end/inspector.js:418 >> + InspectorFrontendAPI.dispatch([WebInspector.queryParamsObject["callAPI"],WebInspector.queryParamsObject["params"]]) > > space after ",". I would also name it dispatchQueryParameters and move queryParamsObject along with the constants into the InspectorFrontendAPI Would this be in addition to or instead of the existing dispatch method?
Created attachment 156447 [details] Patch
Comment on attachment 156447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156447&action=review > Source/WebCore/English.lproj/localizedStrings.js:725 > +localizedStrings["Synchronous resource load failed with XMLHttpRequest status %d"] = "Synchronous resource load failed with XMLHttpRequest status %d"; Please update to match the error string in loadXHR() > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:184 > + var xhr = new XMLHttpRequest(); > + xhr.open("GET", url, false); > + xhr.send(null); So why not just call loadXHR(url, false)? > Source/WebCore/inspector/front-end/utilities.js:699 > + * @param {boolean} async {boolean=}? > Source/WebCore/inspector/front-end/utilities.js:700 > + * @param {function(string)} callback ditto -- we're not using callback for sync requests. > Source/WebCore/inspector/front-end/utilities.js:701 > + * @return {string} value just @return {string} > Source/WebCore/inspector/front-end/utilities.js:708 > + function onDataReceived() onReadyStateChanged? > Source/WebCore/inspector/front-end/utilities.js:710 > + if (xhr.readyState === 4) { if (xhr.readyState !== XMLHttpRequest.DONE) return; > Source/WebCore/inspector/front-end/utilities.js:716 > + WebInspector.log(WebInspector.UIString("Resource load failed with XMLHttpRequest status %d", xhr.status)); nit: I think logging URL as well might be useful. > Source/WebCore/inspector/front-end/utilities.js:731 > + WebInspector.log(WebInspector.UIString("Resource load failed with XMLHttpRequest status %d", xhr.status)); ditto.
Comment on attachment 156447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156447&action=review > Source/WebCore/inspector/front-end/TimelineModel.js:213 > + //var data = InspectorFrontendHost.loadResourceSynchronously(url); Remove this?
Comment on attachment 156447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156447&action=review > Source/WebCore/inspector/front-end/utilities.js:705 > + if (typeof async === "undefined") You don't need these two lines.
Comment on attachment 156447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156447&action=review >> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:184 >> + xhr.send(null); > > So why not just call loadXHR(url, false)? Because Pavel asked me to leave my implementation as is. >> Source/WebCore/inspector/front-end/utilities.js:705 >> + if (typeof async === "undefined") > > You don't need these two lines. Why? I think it makes sense to make it default to synchronous because if async is undefined, presumably callback will be too, thereby making the function useless and why make the user do loadXHR(url, false) when they can just do loadXHR(url). Also, async defaults to true.
(In reply to comment #21) > (From update of attachment 156447 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156447&action=review > > >> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:184 > >> + xhr.send(null); > > > > So why not just call loadXHR(url, false)? > > Because Pavel asked me to leave my implementation as is. > > >> Source/WebCore/inspector/front-end/utilities.js:705 > >> + if (typeof async === "undefined") > > > > You don't need these two lines. > > Why? I think it makes sense to make it default to synchronous because if async is undefined, presumably callback will be too, thereby making the function useless and why make the user do loadXHR(url, false) when they can just do loadXHR(url). Also, async defaults to true. Waiting for a response to this to submit a patch.
Comment on attachment 156447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156447&action=review >>> Source/WebCore/inspector/front-end/utilities.js:705 >>> + if (typeof async === "undefined") >> >> You don't need these two lines. > > Why? I think it makes sense to make it default to synchronous because if async is undefined, presumably callback will be too, thereby making the function useless and why make the user do loadXHR(url, false) when they can just do loadXHR(url). Also, async defaults to true. Oh, my point was that if (undefined) works as if (false). So there is no need in converting it to boolean.
Created attachment 158822 [details] Patch
Comment on attachment 158822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158822&action=review > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:135 > + InspectorFrontendAPI.dispatch([WebInspector.queryParamsObject["callAPI"],WebInspector.queryParamsObject["params"]]) s/,/, / > Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:187 > + var xhr = new XMLHttpRequest(); > + xhr.open("GET", url, false); > + xhr.send(null); > + if (xhr.status === 200) > + return xhr.responseText; > + I still don't think we should duplicate code here. > Source/WebCore/inspector/front-end/TimelineModel.js:175 > + this._loadNextChunk(records, 1); I think this needs a rebaseline now -- we've added progress bars and loadNextChunk now requires a progress parameter. > Source/WebCore/inspector/front-end/utilities.js:707 > + if (xhr.readyState != XMLHttpRequest.DONE) != => !== > Source/WebCore/inspector/front-end/utilities.js:719 > + > + Please drop extra blank like here.
Comment on attachment 158822 [details] Patch Almost there. r- as per caseq's comments.
Comment on attachment 158822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158822&action=review >> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:187 >> + > > I still don't think we should duplicate code here. Agreed. Changed in next patch
Created attachment 158871 [details] Patch
Comment on attachment 158871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158871&action=review > Source/WebCore/ChangeLog:10 > + TBD Nuke this line. > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:148 > + WebInspector.panels.timeline.loadFromURL(url); You should now to WebInspector.showPanel("timeline").loadFromURL(url) instead of these two lines. > Source/WebCore/inspector/front-end/TimelineModel.js:254 > + function onDataReceived(data) Is this function used? > Source/WebCore/inspector/front-end/TimelineModel.js:273 > + function onLoad(responseText) This does not need to be a function. > Source/WebCore/inspector/front-end/TimelinePanel.js:372 > + this.toggleTimelineButton.toggled = false; You probably want to call _toggleTimelineButtonClicked here to reduce copypaste. > Source/WebCore/inspector/front-end/utilities.js:732 > + * @param {function(string)=} callback function(?string)= > Source/WebCore/inspector/front-end/utilities.js:733 > + * @return {string} ?string
Comment on attachment 158871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158871&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:372 >> + this.toggleTimelineButton.toggled = false; > > You probably want to call _toggleTimelineButtonClicked here to reduce copypaste. I think we need to go further and just extract a method that accepts a loader callback, sets up everything for load (including progress bars), loads timeline using a callback and then goes on to parse and add this. Otherwise we have too much of code duplication.
> I think we need to go further and just extract a method that accepts a loader callback, sets up everything for load (including progress bars), loads timeline using a callback and then goes on to parse and add this. Otherwise we have too much of code duplication. Sounds like bloating the scope of this change. caseq@, do you want to do that in a separate change?
Comment on attachment 158871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158871&action=review >> Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:148 >> + WebInspector.panels.timeline.loadFromURL(url); > > You should now to WebInspector.showPanel("timeline").loadFromURL(url) instead of these two lines. showPanel doesn't return the panel. Should it?
Comment on attachment 158871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158871&action=review >>> Source/WebCore/inspector/front-end/TimelinePanel.js:372 >>> + this.toggleTimelineButton.toggled = false; >> >> You probably want to call _toggleTimelineButtonClicked here to reduce copypaste. > > I think we need to go further and just extract a method that accepts a loader callback, sets up everything for load (including progress bars), loads timeline using a callback and then goes on to parse and add this. Otherwise we have too much of code duplication. calling _toggleTimelineButtonClicked would duplicate code in and of itself and would usually cause the timeline to start recording which is not what we want. That is probably the reason loadFromFile didn't use _toggleTimelineButtonClicked.
Created attachment 159136 [details] Patch
Comment on attachment 159136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159136&action=review > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:149 > + WebInspector.panels.timeline.loadFromURL(url); > + WebInspector.showPanel("timeline"); I think you either need to reverse the orde of these two lines to assure timeline panel is already loaded, or use WebInspector.panel("timeline") in the first line.
Comment on attachment 159136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159136&action=review I'll fix the nits and land it for you. >> Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:149 >> + WebInspector.showPanel("timeline"); > > I think you either need to reverse the orde of these two lines to assure timeline panel is already loaded, or use WebInspector.panel("timeline") in the first line. Should be WebInspector.showPanel("timeline").loadFromURL(url); > Source/WebCore/inspector/front-end/inspector.js:417 > + if (InspectorFrontendHost.isStub) { It should go after WebInspector.doLoadedDone.
Created attachment 159636 [details] Patch
@caseq: please cq it if you find it Ok.
Comment on attachment 159636 [details] Patch Clearing flags on attachment: 159636 Committed r126215: <http://trac.webkit.org/changeset/126215>
All reviewed patches have been landed. Closing bug.
*** Bug 96383 has been marked as a duplicate of this bug. ***