Bug 51738 - Web Inspector: refactoring: encapsulate lazy initialization of SourceFrame
Summary: Web Inspector: refactoring: encapsulate lazy initialization of SourceFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-30 02:38 PST by Pavel Podivilov
Modified: 2011-01-17 06:14 PST (History)
10 users (show)

See Also:


Attachments
Patch. (25.15 KB, patch)
2010-12-30 02:38 PST, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Patch. (18.06 KB, patch)
2010-12-30 09:13 PST, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Comments addressed. (17.73 KB, patch)
2010-12-31 05:19 PST, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Rebase. (18.89 KB, patch)
2011-01-11 06:52 PST, Pavel Podivilov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2010-12-30 02:38:02 PST
Web Inspector: refactoring: encapsulate lazy initialization of SourceFrame
Comment 1 Pavel Podivilov 2010-12-30 02:38:45 PST
Created attachment 77667 [details]
Patch.
Comment 2 Pavel Podivilov 2010-12-30 02:39:17 PST
Comment on attachment 77667 [details]
Patch.

Extract content loading logic from SourceView and ScriptView to ContentProvider implementations.
Pass ContentProvider in SourceFrame constructor to allow SourceFrame manage it's lazy initialization.
Comment 3 Pavel Feldman 2010-12-30 06:09:50 PST
Comment on attachment 77667 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=77667&action=review

Could you please split this into smaller patches? It is really hard to follow the changes here and I feel there are bugs introduced.

> WebCore/inspector/front-end/ScriptView.js:34
> +    this.sourceFrame.addScript(script.sourceID, script.startingLine);

I did not know SourceFrame is sourceID aware. It probably should not be.

> WebCore/inspector/front-end/SourceFrame.js:869
> +WebInspector.ContentProvider = function()

SourceFrameContentProvider ?

> WebCore/inspector/front-end/SourceFrame.js:914
> +WebInspector.ResourceContentProvider = function(resource)

I thought the whole point of content provider was to isolate SourceFrame from Script / Resource. Now I see explicit resource usage in SourceFrame.js. This seems wrong.
Comment 4 Pavel Podivilov 2010-12-30 09:13:41 PST
Created attachment 77685 [details]
Patch.
Comment 5 Pavel Podivilov 2010-12-30 09:13:56 PST
Comment on attachment 77685 [details]
Patch.

All comments addressed.
Comment 6 Pavel Feldman 2010-12-31 04:13:09 PST
Comment on attachment 77685 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=77685&action=review

> WebCore/inspector/front-end/SourceFrame.js:835
> +        var sourceIDToStartingLineMap = this._contentProvider.sourceIDToStartingLineMap();

You will be able to use short for syntax if you make properties on Array's prototype non-enumerable:
Object.defineProperty(Array.prototype, "remove", { value: function() { ... }}); By default it'll create non-writable, non-enumerable, non-configurable property.

> WebCore/inspector/front-end/SourceFrame.js:874
> +    sourceIDToStartingLineMap: function()

This is a very strange content provider. I'd suggest to provide Scripts array here: you can then get both ids and starting lines from the scripts.
Comment 7 Pavel Podivilov 2010-12-31 05:19:09 PST
Created attachment 77716 [details]
Comments addressed.
Comment 8 Pavel Podivilov 2011-01-11 06:52:36 PST
Created attachment 78525 [details]
Rebase.
Comment 9 Pavel Podivilov 2011-01-17 06:14:16 PST
Committed r75937: <http://trac.webkit.org/changeset/75937>