[webkit-reviews] review granted: [Bug 190160] Web Inspector: rename frontend managers to be more consistent with backend agents : [Attachment 351343] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 190160: Web Inspector: rename frontend managers to be more consistent with
backend agents
https://bugs.webkit.org/show_bug.cgi?id=190160

Attachment 351343: Patch

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 351343
  --> https://bugs.webkit.org/attachment.cgi?id=351343
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.