Bug 190858

*** Bug 190857 has been marked as a duplicate of this bug. ***

<rdar://problem/45527625>

Created attachment 353079 [details] Patch

Comment on attachment 353079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353079&action=review This looks good to me. Very neat! As mentioned in person: • I think `markObject` would be better named `associateObject` • Describe in the ChangeLog that you're enforcing a `toJSON` to allow for a serialization opportunity that is different from a direct structured clone • Handle abrupt termination, which you should be able to test by killing the NetworkProcess while Inspector is open and performance a database operation > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:14 > + * * Neither the name of Google Inc. nor the names of its This is the wrong license, unless you're copying a Google originated file, in which case they should be listed at the top as well. > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:49 > + let levelString = inspectionLevel > 1 ? "-" + inspectionLevel : ""; Style: Let's wrap the subexpression in parenthesis to avoid any confusion. > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:91 > + markObject(object, key, value) It is unclear to me what this means. It seems to associate an object with a value, but it is unclear why. > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:121 > + console.assert(typeof object.toJSON === "function"); Would be nice to include the object's constructor in the message. This has been helpful in finding ContentViews lacking saveToCookie/restoreFromCookie. Something like: console.assert(typeof object.toJSON === "function", "ObjectStore cannot store object without JSON serialization", object.constructor.name); > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:152 > + if (!(parts[0] in object)) This should probably be a hasOwnProperty check, it sounds like this wants to do a full key path of own properties. > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:170 > + // micro-task, so we need to do everything using event listeners instead of promises. Typo: "micro-task" => "microtask" https://html.spec.whatwg.org/multipage/webappapis.html#microtask > LayoutTests/ChangeLog:9 > + * inspector/model/objectStore/add-expected.txt: Added. These could be under `inspector/unit-test/objectStore/`. They aren't really model objects. > LayoutTests/inspector/model/objectStore/add.html:12 > + InspectorTest.ObjectStore.wrapTest(name, async function() { Style: Perhaps `async () => {` over `async function() {`, since we've been preferring arrow functions in many places. But I could go either way here. > LayoutTests/inspector/model/objectStore/add.html:34 > + ], Style: You could drop all these commas, we never expect anything after tests. > LayoutTests/inspector/model/objectStore/resources/objectStore-utilities.js:24 > + WI.ObjectStore.__testObjectStore = new WI.ObjectStore("__testing", options); Assert that WI.ObjectStore.__testObjectStore doesn't already exist, since it seems tests wouldn't expect that to get clobbered: InspectorTest.assert(!WI.ObjectStore.__testObjectStore, "__testObjectStore not cleaned up appropriately between tests"); > LayoutTests/inspector/model/objectStore/resources/objectStore-utilities.js:77 > + WI.ObjectStore.__testing = true; Is the __testing still needed? It seems to not be used anywhere. If it is needed, then I'd expect a try/catch around the contents before setting __testing to false.

Comment on attachment 353079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353079&action=review >> Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:14 >> + * * Neither the name of Google Inc. nor the names of its > > This is the wrong license, unless you're copying a Google originated file, in which case they should be listed at the top as well. Oops. Copypasta. >> LayoutTests/inspector/model/objectStore/add.html:12 >> + InspectorTest.ObjectStore.wrapTest(name, async function() { > > Style: Perhaps `async () => {` over `async function() {`, since we've been preferring arrow functions in many places. But I could go either way here. I think `async function` reads better. >> LayoutTests/inspector/model/objectStore/add.html:34 >> + ], > > Style: You could drop all these commas, we never expect anything after tests. I personally would rather add the comma for the sake of making a future diff cleaner. Also, we can add `teardown` functions after tests, so that isn't necessarily true.

Created attachment 353453 [details] Patch

Created attachment 353472 [details] Patch Remove accidental `InspectorTest.debug()`

Comment on attachment 353472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353472&action=review r=me with a quibble about keeping object stores out of view classes. > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:150 > + while (parts.length) { Please add a short comment here to point out the two cases of a key with a dot in it that doesn't resolve, and an actual key path in the Cocoa/multiple property access sense. It took me a while to figure out. > Source/WebInspectorUI/UserInterface/Views/AuditTabContentView.js:92 > + WI.objectStores.audits.getAll().then((results) => { I think it's wrong that this doesn't go through AuditManager. Are we ok with the uses of the object stores spread between view and model code? This could just as easily call WI.auditManager.loadSavedAuditResults(), and then this functionality is actually testable.

Comment on attachment 353472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353472&action=review >> Source/WebInspectorUI/UserInterface/Views/AuditTabContentView.js:92 >> + WI.objectStores.audits.getAll().then((results) => { > > I think it's wrong that this doesn't go through AuditManager. Are we ok with the uses of the object stores spread between view and model code? This could just as easily call WI.auditManager.loadSavedAuditResults(), and then this functionality is actually testable. The only reason I had this here was because I didn't want to eagerly load saved audits when the manager is created, as they may never be used (e.g. if the tab isn't shown). I think wrapping this inside a` WI.AuditManager` call is fine.

Created attachment 353553 [details] Patch

Comment on attachment 353553 [details] Patch Clearing flags on attachment: 353553 Committed r237665: <https://trac.webkit.org/changeset/237665>

All reviewed patches have been landed. Closing bug.