If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

Tree Management Graveyard
TBPL
--
minor
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: mstange)

Tracking

Dependency tree / graph

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Severity: normal → minor
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

6 years ago
Created attachment 535649 [details] [diff] [review]
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #535649 - Flags: review?(arpad.borsos)
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.
Attachment #535649 - Flags: review?(arpad.borsos) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 536578 [details] [diff] [review]
part 1, v2
Attachment #535649 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Created attachment 536579 [details] [diff] [review]
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.
Attachment #536579 - Flags: review?(arpad.borsos)
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 on attachment 536579 [details] [diff] [review]
part 2, v1

Review of attachment 536579 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536579 - Flags: review?(arpad.borsos) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 536596 [details] [diff] [review]
part 1, v3

with constant HTML moved to index.html
Attachment #536578 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 661365
(Assignee)

Updated

6 years ago
Depends on: 669000

Updated

6 years ago
Depends on: 682914
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.