Bug 190853 - Web Inspector: Audit: show metadata for results
Summary: Web Inspector: Audit: show metadata for results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: WebInspectorAuditTab
Blocks: 191045
  Show dependency treegraph
 
Reported: 2018-10-23 18:36 PDT by Devin Rousso
Modified: 2018-10-31 11:26 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.53 KB, patch)
2018-10-30 23:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (750.20 KB, image/png)
2018-10-30 23:27 PDT, Devin Rousso
no flags Details
Archive of layout-test-results from ews101 for mac-sierra (2.40 MB, application/zip)
2018-10-31 00:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.14 MB, application/zip)
2018-10-31 00:37 PDT, EWS Watchlist
no flags Details
Patch (27.29 KB, patch)
2018-10-31 00:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-10-23 18:36:25 PDT
This would include:
 - date run
 - time taken
 - website
 - system info (maybe)
Comment 1 Radar WebKit Bug Importer 2018-10-24 11:42:00 PDT
<rdar://problem/45527623>
Comment 2 Devin Rousso 2018-10-30 23:27:03 PDT
Created attachment 353466 [details]
Patch
Comment 3 Devin Rousso 2018-10-30 23:27:23 PDT
Created attachment 353467 [details]
[Image] After Patch is applied
Comment 4 EWS Watchlist 2018-10-31 00:27:53 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-10-31 00:27:55 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-10-31 00:37:31 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-10-31 00:37:32 PDT Comment hidden (obsolete)
Comment 8 Devin Rousso 2018-10-31 00:58:12 PDT
Created attachment 353471 [details]
Patch
Comment 9 BJ Burg 2018-10-31 09:10:23 PDT
Comment on attachment 353471 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:116
> +            let evaluateResponse = await RuntimeAgent.evaluate.invoke(evaluateArguments);

Nit: better name is evaluateResult or evaluationResult.

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:109
> +                    durationElement.textContent = Number.secondsToString((metadata.endTimestamp - metadata.startTimestamp) / 1000);

The .00 after every duration is kind of ugly. I thought that Number.secondsToString would have elided that rather than add fake precision.

> Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.css:59
> +    --metadata-width: 60px;

This could be brittle for localization, so we have to fix later.
Comment 10 Devin Rousso 2018-10-31 10:33:06 PDT
Comment on attachment 353471 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:116
>> +            let evaluateResponse = await RuntimeAgent.evaluate.invoke(evaluateArguments);
> 
> Nit: better name is evaluateResult or evaluationResult.

I didn't want to use "result" because the returned object has a "result" member, as `evaluateResult.result` seems "ugly".

>> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:109
>> +                    durationElement.textContent = Number.secondsToString((metadata.endTimestamp - metadata.startTimestamp) / 1000);
> 
> The .00 after every duration is kind of ugly. I thought that Number.secondsToString would have elided that rather than add fake precision.

I'll do a followup for this.

>> Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.css:59
>> +    --metadata-width: 60px;
> 
> This could be brittle for localization, so we have to fix later.

Correct me if I'm wrong, but isn't the "%" symbol the same in all localizations?  Same with numbers?
Comment 11 BJ Burg 2018-10-31 10:50:51 PDT
(In reply to Devin Rousso from comment #10)
> Comment on attachment 353471 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353471&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:116
> >> +            let evaluateResponse = await RuntimeAgent.evaluate.invoke(evaluateArguments);
> > 
> > Nit: better name is evaluateResult or evaluationResult.
> 
> I didn't want to use "result" because the returned object has a "result"
> member, as `evaluateResult.result` seems "ugly".
> 
> >> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:109
> >> +                    durationElement.textContent = Number.secondsToString((metadata.endTimestamp - metadata.startTimestamp) / 1000);
> > 
> > The .00 after every duration is kind of ugly. I thought that Number.secondsToString would have elided that rather than add fake precision.
> 
> I'll do a followup for this.
> 
> >> Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.css:59
> >> +    --metadata-width: 60px;
> > 
> > This could be brittle for localization, so we have to fix later.
> 
> Correct me if I'm wrong, but isn't the "%" symbol the same in all
> localizations?  Same with numbers?

No. For example, Arabic uses different number glyphs, which tend to be wider. Percentages also are locale dependent even for Indic numbers.
Comment 12 WebKit Commit Bot 2018-10-31 11:26:03 PDT
Comment on attachment 353471 [details]
Patch

Clearing flags on attachment: 353471

Committed r237644: <https://trac.webkit.org/changeset/237644>
Comment 13 WebKit Commit Bot 2018-10-31 11:26:05 PDT
All reviewed patches have been landed.  Closing bug.