874363 - The timeline header in the Netmonitor should be clickable

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Geoff Lankow (:darktrojan)]

+++ This bug was initially created as a clone of Bug #859039 +++

Once the order has been changed there's no way to return it to the original (request start time) order.

Comment 1

[Victor Porof [:vporof][:vp]]

(In reply to Geoff Lankow (:darktrojan) from comment #0)

Clicking on a header once sorts ascending.
Clicking again sorts descending.
Clicking once again clears sorting (which means, sorts by timing).

Does that not work for you?

Comment 2

[Geoff Lankow (:darktrojan)]

Oh, okay. That's a behaviour I hadn't thought to look for. I think the header of the timeline needs to be clickable as well.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: v1

I've put the waterfall time labels into a button to match the other headers. The glowing edge doesn't appear but that seems to be an unrelated bug (it also happens on the file and domain headers for me).

Comment 4

[Geoff Lankow (:darktrojan)]

Filed bug 878659 on the headers' buggy appearance.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 757192 [details] [diff] [review]
v1

Review of attachment 757192 [details] [diff] [review]:
-----------------------------------------------------------------

Delicious! See the comments below, r+ with those addressed. Please also rebase on fx-team tip, a small comment hunk failed when patching netmonitor-view.js.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +379,2 @@
>  
>      // Sort by timing.

Nit: I guess you can remove this comment now.

@@ +381,5 @@
>      if (!target || !direction) {
> +      aType = "waterfall";
> +      target = $("#requests-menu-waterfall-button");
> +      target.setAttribute("sorted", direction = "ascending");
> +      target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));

The target was already queried and I don't think you need to set those attributes here. IIRC they're already set in the conditionals above. Just keep aType = "waterfall".

::: browser/devtools/netmonitor/netmonitor.xul
@@ +85,5 @@
> +            <button id="requests-menu-waterfall-button"
> +                   class="requests-menu-header-button requests-menu-waterfall"
> +                   onclick="NetMonitorView.RequestsMenu.sortBy('waterfall')"
> +                   pack="start"
> +                   flex="1">

Indent class, onclick, pack and flex with one more space.

@@ +86,5 @@
> +                   class="requests-menu-header-button requests-menu-waterfall"
> +                   onclick="NetMonitorView.RequestsMenu.sortBy('waterfall')"
> +                   pack="start"
> +                   flex="1">
> +              &netmonitorUI.toolbar.waterfall;

This string should defined as a xul:label, inside the button, to avoid some terrible XUL warnings. Like this:

<hbox id="requests-menu-waterfall-header-box"
      class="requests-menu-header requests-menu-waterfall"
      align="center"
      flex="1">
  <button id="requests-menu-waterfall-button"
          class="requests-menu-header-button requests-menu-waterfall"
          onclick="NetMonitorView.RequestsMenu.sortBy('waterfall')"
          pack="start"
          flex="1">
    <label id="requests-menu-waterfall-label"
           class="plain requests-menu-waterfall"
           value="&netmonitorUI.toolbar.waterfall;"/>
  </button>
</hbox>

Comment 6

[Geoff Lankow (:darktrojan)]

(In reply to Victor Porof [:vp] from comment #5)
> @@ +381,5 @@
> >      if (!target || !direction) {
> > +      aType = "waterfall";
> > +      target = $("#requests-menu-waterfall-button");
> > +      target.setAttribute("sorted", direction = "ascending");
> > +      target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));
> 
> The target was already queried and I don't think you need to set those
> attributes here. IIRC they're already set in the conditionals above. Just
> keep aType = "waterfall".

It is needed, otherwise clicking on the header switches between ascending, descending and unsorted (actually sorted ascending), which is confusing for the user. I've changed the comment to explain.

https://hg.mozilla.org/integration/mozilla-inbound/rev/609de2f93690

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Geoff Lankow (:darktrojan) from comment #6)
> It is needed, otherwise clicking on the header switches between ascending,
> descending and unsorted (actually sorted ascending), which is confusing for
> the user. I've changed the comment to explain.
> 

In that case, the else block above is redundant, or at least overlaps with the current logic. Right now, there is no more "unsorted" state for the buttons. Thus, removing attributes just so that you can add them again is awkward.

Also, you didn't fully respect the xul attributes I mentioned in comment #5: the inner label does not have the id and .requests-menu-waterfall class, which was used in netmonitor.css (content css, not theme).

> https://hg.mozilla.org/integration/mozilla-inbound/rev/609de2f93690

We usually land things on fx-team first, then merge them to m-c, to avoid the need for eventual extra devtools rebasing.

Comment 8

[Ed Morley [:emorley]]

Backed out for failures in browser_net_timeline_ticks.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23759464&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=23759555&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cd7757cba6

Comment 9

[Geoff Lankow (:darktrojan)]

Take 2, now with correctly IDed label:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0214893763c

Comment 10

[Geoff Lankow (:darktrojan)]

Or not, didn't see comment 7. Sorry about the spam.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: v3

Removed the third sorting state.

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 757901 [details] [diff] [review]
v3

Review of attachment 757901 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with green try run.

Comment 13

[Victor Porof [:vporof][:vp]]

I played with this for a while, it works really great! Thanks Geoff.

Comment 14

[Victor Porof [:vporof][:vp]]

Green! Land away! https://tbpl.mozilla.org/?tree=Try&rev=87dca3256532

Comment 15

[Geoff Lankow (:darktrojan)]

Landed on the right tree this time too!
https://hg.mozilla.org/integration/fx-team/rev/8f9ba85eb61c

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8f9ba85eb61c