Web Inspector: Audit: save imported audits across WebInspector sessions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

We should save any audits imported by the user and automatically show them when reopening WebInspector.

[Devin Rousso]

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

[Radar WebKit Bug Importer]

<rdar://problem/45527625>

[Devin Rousso]

Created attachment 353079 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 353453 [details]
Patch

[Devin Rousso]

Created attachment 353472 [details]
Patch

Remove accidental `InspectorTest.debug()`

[Blaze Burg]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 353553 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353553 [details]
Patch

Clearing flags on attachment: 353553

Committed r237665: <https://trac.webkit.org/changeset/237665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.