Bug 141011

Summary:

Web Inspector: Make BasicBlockAnnotator lessen the saturation of syntax highlighting instead of graying out unexecuted code

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

Web Inspector

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	timothy: review+

Description Saam Barati 2015-01-28 14:45:52 PST

This provides a nicer user experience.

Comment 1 Radar WebKit Bug Importer 2015-01-28 14:46:10 PST

<rdar://problem/19638077>

Comment 2 Saam Barati 2015-01-29 11:21:19 PST

Created attachment 245635 [details]
patch

Comment 3 Timothy Hatcher 2015-01-29 12:32:29 PST

Comment on attachment 245635 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:-106
> -        if (this._isTextRangeOnlyWhitespace(startPosition, endPosition) || this._isTextRangeOnlyClosingBrace(startPosition, endPosition))
> -            return null;

Should we keep the _isTextRangeOnlyClosingBrace check?

Comment 4 Saam Barati 2015-01-29 16:24:17 PST

(In reply to comment #3)
> Comment on attachment 245635 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245635&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:-106
> > -        if (this._isTextRangeOnlyWhitespace(startPosition, endPosition) || this._isTextRangeOnlyClosingBrace(startPosition, endPosition))
> > -            return null;
> 
> Should we keep the _isTextRangeOnlyClosingBrace check?

I don't think it's necessary. It shouldn't affect the UI in any way with the risk that it complicates the code and also uses more computation resources because that regexp will execute every time because if a range is white space only, we won't cache it.

Comment 5 Saam Barati 2015-01-29 16:25:04 PST

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 245635 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=245635&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:-106
> > > -        if (this._isTextRangeOnlyWhitespace(startPosition, endPosition) || this._isTextRangeOnlyClosingBrace(startPosition, endPosition))
> > > -            return null;
> > 
> > Should we keep the _isTextRangeOnlyClosingBrace check?
> 
> I don't think it's necessary. It shouldn't affect the UI in any way with the
> risk that it complicates the code and also uses more computation resources
> because that regexp will execute every time because if a range is white
> space only, we won't cache it.

read: ... the UI in any way and has the risk ...

Comment 6 Saam Barati 2015-01-29 16:50:35 PST

(In reply to comment #3)
> Comment on attachment 245635 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245635&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:-106
> > -        if (this._isTextRangeOnlyWhitespace(startPosition, endPosition) || this._isTextRangeOnlyClosingBrace(startPosition, endPosition))
> > -            return null;
> 
> Should we keep the _isTextRangeOnlyClosingBrace check?

I totally misread this as keep _isTextRangeOnlyWhitespace.

We do want to keep the _isTextRangeOnlyClosingBrace check.

Comment 7 Saam Barati 2015-01-29 20:03:16 PST

landed in:
http://trac.webkit.org/changeset/179390