Bug 91528 – Web Inspector: Embeddable Web Inspector

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

[Gabriel Peal]

Embeddable Developer Tools

[Gabriel Peal]

Created attachment 152793 [details]
Patch

[Andrey Kosyakov]

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)

[Gabriel Peal]

Created attachment 153026 [details]
Patch

[Gabriel Peal]

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?

[Andrey Kosyakov]

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.

[Gabriel Peal]

Created attachment 154992 [details]
Patch

[Pavel Feldman]

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&param=<url> or similarly.

[Gabriel Peal]

Created attachment 155315 [details]
Patch

[Andrey Kosyakov]

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"]]?

[Pavel Feldman]

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.

[Gabriel Peal]

Created attachment 155817 [details]
Patch

[Andrey Kosyakov]

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.

[Gabriel Peal]

Created attachment 155833 [details]
Patch

[Gabriel Peal]

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

[Pavel Feldman]

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)

[Gabriel Peal]

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?

[Gabriel Peal]

Created attachment 156447 [details]
Patch

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

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?

[Pavel Feldman]

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.

[Gabriel Peal]

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.

[Gabriel Peal]

(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.

[Pavel Feldman]

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.

[Gabriel Peal]

Created attachment 158822 [details]
Patch

[Andrey Kosyakov]

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.

[Pavel Feldman]

Comment on attachment 158822 [details]
Patch

Almost there. r- as per caseq's comments.

[Gabriel Peal]

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

[Gabriel Peal]

Created attachment 158871 [details]
Patch

[Pavel Feldman]

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

[Andrey Kosyakov]

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.

[Pavel Feldman]

> 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?

[Gabriel Peal]

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?

[Gabriel Peal]

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.

[Gabriel Peal]

Created attachment 159136 [details]
Patch

[Andrey Kosyakov]

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.

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 159636 [details]
Patch

[Pavel Feldman]

@caseq: please cq it if you find it Ok.

[WebKit Review Bot]

Comment on attachment 159636 [details]
Patch

Clearing flags on attachment: 159636

Committed r126215: <http://trac.webkit.org/changeset/126215>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ahmad Saleem]

*** Bug 96383 has been marked as a duplicate of this bug. ***