657539 - TBPL links to a single changeset shouldn't include &pusher=

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Justin Lebar (not reading bugmail)]

Clicking the time of a changeset takes you to a special TBPL page which shows just that changeset.  This is cool.

But if you have an &pusher= filter active, the single-changeset page retains this filter.  It's redundant to specify the pusher if you're specifying the changeset, and these single-changeset URLs are good for sharing, so I think they should be as clean as possible.

Comment 1

[Markus Stange [:mstange]]

Attachment: v1

This reorganizes our whole param / url / pushState setup. All param changes now go through Controller, and the current state is always extracted from the URL string, never from a pushState state object.

Apart from this bug itself, the patch also does these things:
 - Pusher links are turned into real links that can be middle-clicked.
 - usebuildbot=1 (from bug 656919) will be kept when switching trees.
 - Turning on any filtering will make the link to the current tree clickable
   so that you can return to the unfiltered view.

Comment 2

[Arpad Borsos [:Swatinem]]

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

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

Very nice!

::: js/Controller.js
@@ +37,5 @@
>      if (!("tree" in params) && ("branch" in params)) {
> +      document.location = this.getURLForChangedParams({
> +        tree: this._treeForBranch(params.branch),
> +        branch: null
> +      });

You haven’t pushed that ?branch= patch so far, just noticed ;-)

@@ +81,5 @@
> +  _getChangedParams: function Controller__getChangedParams(paramChanges) {
> +    var params = this.getParams();
> +    for (var key in paramChanges) {
> +      if (key in this._paramOverrides) {
> +        // Unset all overriden params.

overridden with 2 d like you wrote below.

Comment 3

[Markus Stange [:mstange]]

Attachment: part 1, v2

Comment 4

[Markus Stange [:mstange]]

Attachment: part 2, v1

Teach params about default values.
This causes the link to the Firefox tree to leave out the tree=Firefox part of the URL. It also makes it a non-link both on / and on /?tree=Firefox.

Comment 5

[Arpad Borsos [:Swatinem]]

Comment on attachment 536578 [details] [diff] [review]
part 1, v2

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

::: js/UserInterface.js
@@ +329,5 @@
> +    $("#treechooser").html(
> +      '<ul id="mruList"></ul>' +
> +      '<div id="moreListContainer" class="dropdown">' +
> +        '<h2>more</h2><ul id="moreList" class="dropdownContents"></ul>' +
> +      '</div>');

Looking at this again, I think you can push that into index.html.

Comment 6

[Arpad Borsos [:Swatinem]]

Comment on attachment 536579 [details] [diff] [review]
part 2, v1

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

Comment 7

[Markus Stange [:mstange]]

Attachment: part 1, v3

with constant HTML moved to index.html