Bug 110866 - Web Inspector: Refactor AuditsPanel with AuditController as newly introduced entity
Summary: Web Inspector: Refactor AuditsPanel with AuditController as newly introduced ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vivek Galatage
URL:
Keywords:
Depends on:
Blocks: 103945
  Show dependency treegraph
 
Reported: 2013-02-26 03:43 PST by Vivek Galatage
Modified: 2013-02-27 07:13 PST (History)
9 users (show)

See Also:


Attachments
Patch (15.40 KB, patch)
2013-02-26 03:46 PST, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (17.96 KB, patch)
2013-02-26 19:31 PST, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (17.98 KB, patch)
2013-02-27 06:31 PST, Vivek Galatage
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vivek Galatage 2013-02-26 03:43:12 PST
AuditController is being introduced in order to follow MVC pattern with the Audits. This is the first step about the refactoring. 

Moving the methods from the AuditsPanel into AuditController would ease the event driven approach(to be added later) to de-couple the AuditsPanel and the AuditLauncherView.

Patch follows
Comment 1 Vivek Galatage 2013-02-26 03:46:53 PST
Created attachment 190253 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2013-02-26 07:43:47 PST
Comment on attachment 190253 [details]
Patch

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

Barring the file patching for the new JS addition, looks good for a start.

> Source/WebCore/ChangeLog:16
> +        * inspector/front-end/AuditController.js: Added.

You forgot to patch WebCore.vcproj and compile-front-end.py for the new file.

> Source/WebCore/inspector/front-end/AuditController.js:91
> +        this._auditsPanel._auditFinishedCallback(mainResourceURL, results);

For the time being, you should remove the leading '_' from "_auditFinishedCallback", since this denotes a file-private member. Hopefully, this will get removed altogether soon.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:160
> +        this._auditController.initiateAudit(catIds, this._progressIndicator, this._auditPresentStateElement.checked, onAuditStarted.bind(this), this._setAuditRunning.bind(this, false));

These callbacks will go away once we migrate to the events.
Comment 3 Vivek Galatage 2013-02-26 19:31:08 PST
Created attachment 190420 [details]
Patch
Comment 4 Vivek Galatage 2013-02-26 19:36:53 PST
(In reply to comment #2)
> Barring the file patching for the new JS addition, looks good for a start.
> 

Thank you for the review. :)

> > Source/WebCore/ChangeLog:16
> > +        * inspector/front-end/AuditController.js: Added.
> 
> You forgot to patch WebCore.vcproj and compile-front-end.py for the new file.
> 

Oh sure, forgot these ones. Done.

> > Source/WebCore/inspector/front-end/AuditController.js:91
> > +        this._auditsPanel._auditFinishedCallback(mainResourceURL, results);
> 
> For the time being, you should remove the leading '_' from "_auditFinishedCallback", since this denotes a file-private member. Hopefully, this will get removed altogether soon.
>

Modified the method to remove the leading '_'. Also modified the respective extension test which sniffs for this method.
 
> > Source/WebCore/inspector/front-end/AuditLauncherView.js:160
> > +        this._auditController.initiateAudit(catIds, this._progressIndicator, this._auditPresentStateElement.checked, onAuditStarted.bind(this), this._setAuditRunning.bind(this, false));
> 
> These callbacks will go away once we migrate to the events.

Yes with the events, every callback in controller should disappear.
Comment 5 Alexander Pavlov (apavlov) 2013-02-27 06:18:52 PST
Comment on attachment 190420 [details]
Patch

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

Once the JSDoc is added, we are good to land.

> Source/WebCore/inspector/front-end/AuditController.js:128
> +    _reloadResources: function(callback)

Please add a JSDoc:

/**
 * @param {function()=} callback
 */
Comment 6 Vivek Galatage 2013-02-27 06:31:37 PST
Created attachment 190510 [details]
Patch
Comment 7 WebKit Review Bot 2013-02-27 07:13:51 PST
Comment on attachment 190510 [details]
Patch

Clearing flags on attachment: 190510

Committed r144182: <http://trac.webkit.org/changeset/144182>
Comment 8 WebKit Review Bot 2013-02-27 07:13:54 PST
All reviewed patches have been landed.  Closing bug.