Bug 192210

Created attachment 356125 [details] Patch

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

<rdar://problem/46423583>

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`?

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.

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.

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.

Created attachment 358871 [details] Patch

Comment on attachment 358871 [details] Patch Clearing flags on attachment: 358871 Committed r239858: <https://trac.webkit.org/changeset/239858>

All reviewed patches have been landed. Closing bug.