Page MenuHomePhabricator

Cleanup/feedback from the security review
Closed, ResolvedPublic

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)

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

Peter closed this task as Resolved.Aug 24 2016, 9:37 AM
Peter reopened this task as Open.
Peter triaged this task as Normal priority.
Peter moved this task from Inbox to Doing 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

Peter added a subscriber: Krinkle.Aug 24 2016, 1:14 PM

@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

  • 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?

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:

afterbefore

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.

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

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

Peter closed this task as Resolved.Sep 1 2016, 10:20 PM

These are fixed now, thanks for the feedback @Bawolff, and thanks for the help @Krinkle