Bug 190160 - Web Inspector: rename frontend managers to be more consistent with backend agents
Summary: Web Inspector: rename frontend managers to be more consistent with backend ag...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 190165 190226
  Show dependency treegraph
 
Reported: 2018-10-01 14:24 PDT by Devin Rousso
Modified: 2018-10-02 16:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (539.28 KB, patch)
2018-10-01 22:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (549.53 KB, patch)
2018-10-02 13:24 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 2018-10-01 14:24:58 PDT
e.g. `WI.FrameResourceManager` should really be `WI.NetworkManager`.
Comment 1 Devin Rousso 2018-10-01 22:49:52 PDT
Created attachment 351343 [details]
Patch
Comment 2 Joseph Pecoraro 2018-10-02 12:53:34 PDT
Comment on attachment 351343 [details]
Patch

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

Can you put up a new patch after adding the following to your git config:

    [diff]
        renames = copies

I think it would eliminate the full content of files in the diff that were just renamed and instead include an actual diff. Also when it is applied it should perform an svn copy of the files and carry blame history forwards more appropriately.

> Source/WebInspectorUI/ChangeLog:20
> +        * UserInterface/Controllers/DatabaseManager.js: Copied from Source/WebInspectorUI/UserInterface/Controllers/IssueManager.js.

Heh, the ChangeLog says this was copied from IssueManager.js I think probably StorageManager.js?

> Source/WebInspectorUI/UserInterface/Base/Main.js:120
>      this.issueManager = new WI.IssueManager;

This one can probably just fold into ConsoleManager. Heck, ConsoleManager is the only thing that adds issues to IssueManager.

> Source/WebInspectorUI/UserInterface/Base/Main.js:400
> +    this._dashboards = {
> +        default: new WI.DefaultDashboard,
> +        debugger: new WI.DebuggerDashboard,
> +    };

How about we just make this:

    this._defaultDashboard = ...;
    this._debuggerDashboard = ...;

Instead of a `dashboards` object.

> Source/WebInspectorUI/UserInterface/Controllers/ConsoleManager.js:155
> +    PreviousMessageRepeatCountUpdated: "console-manager-previous-message-repeat-count-updated"

Style: Trailing comma

> Source/WebInspectorUI/UserInterface/Controllers/DOMStorageManager.js:185
> +    Cleared: "dom-storage-manager-cleared"

Nit: Add the trailing comma. to these so if we add a new event it will be a simple diff.

> Source/WebInspectorUI/UserInterface/Controllers/IndexedDBManager.js:189
> +    Cleared: "indexed-db-manager-cleared"

Style: Trailing comma

> Source/WebInspectorUI/UserInterface/Test/Test.js:-53
> -    this.storageManager = new WI.StorageManager;

Do we not need IndexedDB or Database in tests?!

Please run:

    $ run-webkit-tests --no-retry-failures --no-sample-on-timeout --time-out-ms=5000 --force inspector

And see if there are failures on tests that might be getting skipped by bots.
Comment 3 Devin Rousso 2018-10-02 13:22:33 PDT
Comment on attachment 351343 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:400
>> +    };
> 
> How about we just make this:
> 
>     this._defaultDashboard = ...;
>     this._debuggerDashboard = ...;
> 
> Instead of a `dashboards` object.

I prefer the idea of separating like objects into containers, similar to <https://webkit.org/b/190165>.  The top-level `WI` is already "polluted" enough.

>> Source/WebInspectorUI/UserInterface/Test/Test.js:-53
>> -    this.storageManager = new WI.StorageManager;
> 
> Do we not need IndexedDB or Database in tests?!
> 
> Please run:
> 
>     $ run-webkit-tests --no-retry-failures --no-sample-on-timeout --time-out-ms=5000 --force inspector
> 
> And see if there are failures on tests that might be getting skipped by bots.

Our IndexedDB tests seem to directly call IndexedDBAgent.
Comment 4 Devin Rousso 2018-10-02 13:24:05 PDT
Created attachment 351435 [details]
Patch
Comment 5 WebKit Commit Bot 2018-10-02 15:15:04 PDT
Comment on attachment 351435 [details]
Patch

Clearing flags on attachment: 351435

Committed r236766: <https://trac.webkit.org/changeset/236766>
Comment 6 WebKit Commit Bot 2018-10-02 15:15:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-10-02 15:16:26 PDT
<rdar://problem/44955406>