[webkit-reviews] review granted: [Bug 190858] Web Inspector: Audit: save imported audits across WebInspector sessions : [Attachment 353472] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 190858: Web Inspector: Audit: save imported audits across WebInspector
sessions
https://bugs.webkit.org/show_bug.cgi?id=190858

Attachment 353472: Patch

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




--- Comment #8 from Brian Burg <bburg at apple.com> ---
Comment on attachment 353472
  --> https://bugs.webkit.org/attachment.cgi?id=353472
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.