Bug 192210 – Web Inspector: Audit: allow audits to be enabled/disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 356125 [details]
Patch

[Devin Rousso]

Created attachment 356126 [details]
[Image] After Patch is applied

[Radar WebKit Bug Importer]

<rdar://problem/46423583>

[Joseph Pecoraro]

Comment on attachment 356125 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:55
> +        if (Array.isArray(WI.ObjectStore._databaseCallbacks)) {

A simple if check be sufficient. Is there a benefit to the extra isArray() check?

Also this should be covered in the ChangeLog somewhere. Does this reduce duplicate IndexedDB open() calls?

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:78
> +            WI.objectStores.audits.clear();
> +            for (let test of this._tests)
> +                WI.objectStores.audits.addObject(test);

This feels like it could be put into a well named method. Something like `_resaveAudits`?

[Devin Rousso]

Comment on attachment 356125 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:55
>> +        if (Array.isArray(WI.ObjectStore._databaseCallbacks)) {
> 
> A simple if check be sufficient. Is there a benefit to the extra isArray() check?
> 
> Also this should be covered in the ChangeLog somewhere. Does this reduce duplicate IndexedDB open() calls?

Yes, this is meant to be a way of batching operations together, so that if you performed 3 actions in a row, they'd all be called on the same `open()` call.

    WI.objectStores.foo.a(); // initializes `WI.ObjectStore._databaseCallbacks`
    WI.objectStores.foo.b(); // enqueued
    WI.objectStores.foo.c(); // enqueued
    // once the "open" event fires, all three calls will happen, rather than having each have a separate open, which may resolve in different orders.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:78
>> +                WI.objectStores.audits.addObject(test);
> 
> This feels like it could be put into a well named method. Something like `_resaveAudits`?

Sure.  I'll have to modify this slightly anyways since we are no longer saving default audits.

[Devin Rousso]

Created attachment 358868 [details]
Patch

Merged <https://webkit.org/b/192211> into this change, as there were issues with reloading audit test groups from the IndexedDB due to the fact that the `disabled` value wasn't propagated to the sub-tests, meaning that they would always appear as not disabled (since the children wouldn't be disabled) even though the actual model object would be disabled.

[Joseph Pecoraro]

Comment on attachment 358868 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:81
> +            let saveDisabled = (test) => {

Might be better to have a more searchable name: saveDisabledDefaultTest.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:303
> +        let checkDisabled = (test) => {

Might be better to have a more searchable name: checkDisabledDefaultTest.

[Devin Rousso]

Created attachment 358871 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 358871 [details]
Patch

Clearing flags on attachment: 358871

Committed r239858: <https://trac.webkit.org/changeset/239858>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.