Bug 33618 - Web Inspector: Introduce SourceFrame2 with basic breakpoint / execution line rendering capabilities.
Summary: Web Inspector: Introduce SourceFrame2 with basic breakpoint / execution line ...
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 Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-13 12:31 PST by Pavel Feldman
Modified: 2010-01-13 14:13 PST (History)
4 users (show)

See Also:


Attachments
[IMAGE] Looks like the original SourceFrame. (83.22 KB, image/png)
2010-01-13 12:34 PST, Pavel Feldman
no flags Details
[PATCH] Proposed change (32.30 KB, patch)
2010-01-13 12:38 PST, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-01-13 12:31:07 PST
This is the first step towards embedding editor into the inspector. SourceFrame2 will replace SourceFrame when ready.
Comment 1 Pavel Feldman 2010-01-13 12:34:16 PST
Created attachment 46487 [details]
[IMAGE] Looks like the original SourceFrame.
Comment 2 Pavel Feldman 2010-01-13 12:38:34 PST
Created attachment 46489 [details]
[PATCH] Proposed change
Comment 3 Timothy Hatcher 2010-01-13 12:39:06 PST
This looks great.

Could we use a non-monospace font for the line numbers, so the font cn be smaller? Right now the breakpoints look funny with monospace text.
Comment 4 Timothy Hatcher 2010-01-13 12:50:10 PST
Comment on attachment 46489 [details]
[PATCH] Proposed change


> +    selectionRange: function()
> +    {
> +        this._selection.range();
> +    },

Should this return the range? Make it a getter too.


> +        // Paint currenlt line for editable mode only.

Typo.

> +            if (this._lineNumberDecorator.mouseDown(location.line, e))            

It seems odd to me that a decorator handles events. But it does make it easier. Can this be named handleMouseDown?


> +        this._cursorElement.style.display = "none";

Should be addStyleClass("hidden").

r+, but I suspect the selectionRange function has a bug. But it dosen't look to be called.
Comment 5 Pavel Feldman 2010-01-13 14:13:54 PST
Addressed comments (all but the addStyleClass one since it needs to be consistent with other places in class - will fix later).

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	A	WebCore/inspector/front-end/SourceFrame2.js
	M	WebCore/inspector/front-end/TextEditor.js
	M	WebCore/inspector/front-end/TextEditorHighlighter.js
	M	WebCore/inspector/front-end/TextEditorModel.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
Committed r53205