Bug 141011 - Web Inspector: Make BasicBlockAnnotator lessen the saturation of syntax highlighting instead of graying out unexecuted code
Summary: Web Inspector: Make BasicBlockAnnotator lessen the saturation of syntax highl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-28 14:45 PST by Saam Barati
Modified: 2015-01-29 20:03 PST (History)
7 users (show)

See Also:

Attachments
patch (11.90 KB, patch)
2015-01-29 11:21 PST, Saam Barati 		timothy: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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