Bug 148143 - Web Inspector: Modernize CSSStyleManager
Summary: Web Inspector: Modernize CSSStyleManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2015-08-18 14:32 PDT by Joseph Pecoraro
Modified: 2015-08-18 15:12 PDT (History)
7 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (6.65 KB, patch)
2015-08-18 14:34 PDT, Joseph Pecoraro 		bburg: 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 Joseph Pecoraro 2015-08-18 14:32:10 PDT 
* SUMMARY
Modernize CSSStyleManager.

  - Eliminate `delete`
  - Use Map instead of Object for a Map.
Comment 1 Joseph Pecoraro 2015-08-18 14:34:48 PDT 
Created attachment 259298 [details]
[PATCH] Proposed Fix
Comment 2 BJ Burg 2015-08-18 15:01:08 PDT 
Comment on attachment 259298 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:-227
> -        function syleSheetsFetched()

Ouch!

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:233
>          var key = this._frameURLMapKey(frame, url);

Can we move this above the callback and make it `let`?

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:236
> +        var styleSheet = this._styleSheetFrameURLMap.get(key) || this._styleSheetFrameURLMap.get(url) || null;

let?

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:347
> +            this._ignoreResourceContentDidChangeEventForResource = null;

Why null?
Comment 3 Joseph Pecoraro 2015-08-18 15:08:36 PDT 
> > Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:347
> > +            this._ignoreResourceContentDidChangeEventForResource = null;
> 
> Why null?

I find `null` clearer here. It better signifies our intent for that property, "an object type goes here", better than undefined.
Comment 4 Joseph Pecoraro 2015-08-18 15:12:19 PDT 
http://trac.webkit.org/changeset/188601