Bug 147537 - Web Inspector: Convert CSSStyleDeclarationSection to ES6 Class
Summary: Web Inspector: Convert CSSStyleDeclarationSection to ES6 Class
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-02 00:13 PDT by Devin Rousso
Modified: 2015-08-07 10:09 PDT (History)
7 users (show)

See Also:


Attachments
Patch (26.55 KB, patch)
2015-08-02 00:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (26.88 KB, patch)
2015-08-03 23:09 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2015-08-02 00:13:55 PDT
The rest of the CSSStyleSidebar classes are ES6 classes except for CSSStyleDeclarationSection.  We should change that.
Comment 1 Devin Rousso 2015-08-02 00:45:35 PDT
Created attachment 258027 [details]
Patch
Comment 2 Joseph Pecoraro 2015-08-03 15:19:38 PDT
Comment on attachment 258027 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:28
> +    constructor(delegate, style) {

Style: Treat "constructor" like a method, and put the opening brace on its own line.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:33
> +        console.assert(style);

Nit: I know this is not new, but it is better to assert not just that "style" exists, but that it adheres to some expected type. Perhaps:

    console.assert(style instanceof WebInspector.CSSStyleDeclaration);
Comment 3 Devin Rousso 2015-08-03 23:09:49 PDT
Created attachment 258157 [details]
Patch
Comment 4 Joseph Pecoraro 2015-08-03 23:37:35 PDT
Note, if you have already gotten a review, you can put up the new patch with the "reviewed by" line filled in, and just cq+ it for the commit-queue to land.
Comment 5 Devin Rousso 2015-08-03 23:38:05 PDT
(In reply to comment #4)
> Note, if you have already gotten a review, you can put up the new patch with
> the "reviewed by" line filled in, and just cq+ it for the commit-queue to
> land.

I did not know that.  Thanks for the tip!
Comment 6 Joseph Pecoraro 2015-08-03 23:38:20 PDT
Comment on attachment 258157 [details]
Patch

r=me
Comment 7 Nikita Vasilyev 2015-08-03 23:40:48 PDT
(In reply to comment #4)
> Note, if you have already gotten a review, you can put up the new patch with
> the "reviewed by" line filled in, and just cq+ it for the commit-queue to
> land.

The review dropdown must be left empty. If you set it to "r+", the commit queue will fail.
Comment 8 Timothy Hatcher 2015-08-07 10:09:00 PDT
Comment on attachment 258157 [details]
Patch

It looks like Brian got this. It is now a class in TOT.