Cleanup/feedback from the security review
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Peter
	Aug 24 2016, 9:35 AM

Description

Got a couple of good feedback from the security review T137197, lets fix them:

[extension.json] i18n is marked as for the Boilerplate extension. This should be changed, as could mess up localizationCache if another extension used the same name.
The toolbox entry is shown always, but only works when doing a preview of a page (And you have js enabled). Toolbox entry really shouldn't be shown in cases where it wouldn't work, or at least some sort of error should pop-up in cases where it doesn't work.
Be nicer it the parser cache section of the report actually parsed the data (e.g. date should be human readable form. Cache expiry should specify the unit is seconds, etc.)
The bar-graph should probably have whitespace: nowrap for the module names, otherwise long module names smush into next row.
[modulesize collector toHumanSize()] - Note we also already have i18n messages for size units - size-megabytes and friends. Perhaps those should be used for the human readable sizes.
Missing message performanceinspector-imagesize-title, performanceinspector-newpp-title messages. (The current mustache template turns them into tags)

Details

Subject	Repo	Branch	Lines +/-
Make it easier to read date and expire time	mediawiki/extensions/PerformanceInspector	master	+20 -7
Hide Performance Inspector link if Javascript is turned off	mediawiki/extensions/PerformanceInspector	master	+6 -0
Style that handles long module names	mediawiki/extensions/PerformanceInspector	master	+25 -42
Use i18n messages for size	mediawiki/extensions/PerformanceInspector	master	+32 -7
mediawiki.inspect: Output size in bytes used in local storage	mediawiki/core	master	+1 -0
Remove unused message keys in mustache	mediawiki/extensions/PerformanceInspector	master	+0 -2
Set right MessagesDirs	mediawiki/extensions/PerformanceInspector	master	+1 -1

Customize query in gerrit

Related Objects

Mentioned Here: T137197: Security review of the Performance Inspector

Event Timeline

Peter created this task.Aug 24 2016, 9:35 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 24 2016, 9:35 AM

Change 306403 had a related patch set uploaded (by Phedenskog):
Set right MessagesDirs

https://gerrit.wikimedia.org/r/306403

gerritbot added a project: Patch-For-Review.Aug 24 2016, 9:36 AM

Peter closed this task as Resolved.Aug 24 2016, 9:37 AM

Peter reopened this task as Open.

Peter triaged this task as Medium priority.

Peter moved this task from Inbox, needs triage to Doing (old) on the Performance-Team board.

Change 306406 had a related patch set uploaded (by Phedenskog):
Remove unused message keys in mustache

https://gerrit.wikimedia.org/r/306406

Change 306411 had a related patch set uploaded (by Phedenskog):
Use i18n messages for size

https://gerrit.wikimedia.org/r/306411

@Krinkle need your advice on two things:

The toolbox entry is shown always, but only works when doing a preview of a page (And you have js enabled). Toolbox entry really shouldn't be shown in cases where it wouldn't work, or at least some sort of error should pop-up in cases where it doesn't work.

What's the best way to choose when to output the link? I've been checking docs but not really sure.

The bar-graph should probably have whitespace: nowrap for the module names, otherwise long module names smush into next row.

What's the best way to handle long words in CSS, this always fails for me.

Change 306403 merged by jenkins-bot:
Set right MessagesDirs

https://gerrit.wikimedia.org/r/306403

Change 306406 merged by jenkins-bot:
Remove unused message keys in mustache

https://gerrit.wikimedia.org/r/306406

Change 306717 had a related patch set uploaded (by Phedenskog):
Output total size in bytes used in local storage using inspect.

https://gerrit.wikimedia.org/r/306717

Change 306717 merged by jenkins-bot:
mediawiki.inspect: Output size in bytes used in local storage

https://gerrit.wikimedia.org/r/306717

ReleaseTaggerBot added projects: MW-1.28-release (WMF-deploy-2016-08-30_(1.28.0-wmf.17)), MW-1.28-release-notes.Aug 25 2016, 7:00 PM

In T143770#2578872, @Peter wrote:

In T137197#2576786, @Bawolff wrote:

The toolbox entry is shown always, but only works when doing a preview of a page (And you have js enabled).

What's the best way to choose when to output the link? I've been checking docs but not really sure.

I thought we already hid it when JavaScript is disabled. This can be done by adding .client-nojs #t-performanceinspector { display: none; } to a stylesheet. We use the same technique in VisualEditor.

I'm not sure what "Doing a preview of a page" means here. The performance inspector works on any page or action. It doesn't relate to previewing an edit. @Bawolff Maybe you meant a different kind of preview?

In T143770#2578872, @Peter wrote:

In T137197#2576786, @Bawolff wrote:

The bar-graph should probably have whitespace: nowrap for the module names, otherwise long module names smush into next row.

What's the best way to handle long words in CSS, this always fails for me.

Every row currently has a fixed height and the labels are absolutely positioned which means text overflow will stay on the same line and (if needed) exceed the page width and create a scrollbar. This should be solved differently as it overlaps with the size value and because the background of the overflow is white (white text on white background). However, I can't reproduce any case where the text would wrap to the next line as @Bawolff suggests. white-space: nowrap has no effect on absolutely positioned elements.

Here's one idea I drafted locally:

.barchart .row {
    position: relative;
    min-height: 26px;
    margin-bottom: 2px;
    padding: 0 10px;
    background: #999999;
    overflow: hidden;
}
.barchart .barWrap {}
.barchart .barWrap .bar {
    position: absolute;
    top: 0;
    left: 0;
    height: 100%;
    background: #006496;
}
.barchart .label {
    position: relative;
    font-size: 12px;
    line-height: 26px;
    font-weight: bold;
    float: left;
}
.barchart .value {
    position: relative;
    font-size: 14px;
    line-height: 26px;
    font-weight: bold;
    float: right;
}

<div class="row">
	<div class="barWrap"><div class="bar" data-value="31816" style="width: 63%;"></div></div>
	<span class="label">jquery.uls.data</span>
	<span class="value">31.1 KB</span>
</div>

Main differences:

(simplify) Bar node before label and value. (natural stacking order, no z-index)
(fix) Float instead of absolute position, and no width on label. This fixes the issue where the label text overlapped the size text).
Add overflow: hidden; to row. (needed because float took elements out of normal flow, which meant the row no longer had any height and bar's 100% height got collapsed. Hiding overflow triggers element flow containment, similar to setting clear: both; on an extra element as last child of row).
(fix) Set min-height instead of height. This way the row will grow vertically to span multiple lines if needed.
(simplify) Padding on outer .row instead of on label and value.
(simplify) Grey background on .row directly. The wrapper element around .bar could be removed.

Preview:

after	before

Change 306411 merged by jenkins-bot:
Use i18n messages for size

https://gerrit.wikimedia.org/r/306411

I thought we already hid it when JavaScript is disabled. This can be done by adding .client-nojs #t-performanceinspector { display: none; } to a stylesheet. We use the same technique in VisualEditor.

I actually didn't test. I assumed it wasn't because it was showing up on pages where there was js errors and nothing was working.

I'm not sure what "Doing a preview of a page" means here. The performance inspector works on any page or action. It doesn't relate to previewing an edit. @Bawolff Maybe you meant a different kind of preview?

When I was testing, wgPageParseReport was only available during a preview of an edit window. This made the button not work on anything that wasn't a preview operation.

bar code stuff

The before picture that you have looked quite different from what I saw. When I saw overly long module names, they wrapped into the next bar, resulting in the labels for two bars overlapping.

In T143770#2584876, @Bawolff wrote:

When I was testing, wgPageParseReport was only available during a preview of an edit window. This made the button not work on anything that wasn't a preview operation.

Interesting. This may've been just around the time that that variable was introduced in core, and support for it just landed in PerformanceInspector the day before you did the review. Due to being output via parser cache, it was probably not on most page views for you. At this point it should've have all rolled over, though.

Change 306913 had a related patch set uploaded (by Phedenskog):
Style that handles long module names

https://gerrit.wikimedia.org/r/306913

Change 306920 had a related patch set uploaded (by Phedenskog):
Hide Performance Inspector link if Javascript is turned off

https://gerrit.wikimedia.org/r/306920

Change 306913 merged by jenkins-bot:
Style that handles long module names

https://gerrit.wikimedia.org/r/306913

Change 306920 merged by jenkins-bot:
Hide Performance Inspector link if Javascript is turned off

https://gerrit.wikimedia.org/r/306920

Change 307756 had a related patch set uploaded (by Phedenskog):
Make it easier to read date and expire time

https://gerrit.wikimedia.org/r/307756

Change 307756 merged by jenkins-bot:
Make it easier to read date and expire time