| Summary: | Web Inspector: refactoring: encapsulate lazy initialization of SourceFrame | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Pavel Podivilov <podivilov> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Pavel Podivilov
2010-12-30 02:38:02 PST
Created attachment 77667 [details]
Patch.
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 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. Created attachment 77685 [details]
Patch.
Comment on attachment 77685 [details]
Patch.
All comments addressed.
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. Created attachment 77716 [details]
Comments addressed.
Created attachment 78525 [details]
Rebase.
Committed r75937: <http://trac.webkit.org/changeset/75937> |