Closed
Bug 1244859
Opened 9 years ago
Closed 9 years ago
Remove trailing slash from telemetry ping url
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox45 fixed, firefox46 fixed, firefox47 fixed, fennec45+)
RESOLVED
FIXED
Firefox 47
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
mfinkle
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details |
13:12 <@mreid> mcomella: you should remove that trailing slash though
13:12 <@mreid> (it doesn't appear to affect anything on the server, it's just not to-spec)
13:13 <mcomella> mreid: So `.../buildId` as opposed to `.../buildId/` ?
13:13 <@mreid> mcomella: yep
Assignee | ||
Comment 1•9 years ago
|
||
It was not to-spec (though it doesn't appear to have an effect on how the
server reads the data).
Review commit: https://reviewboard.mozilla.org/r/33097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33097/
Attachment #8714470 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 2•9 years ago
|
||
Only bothering to get a review so we can uplift later.
Comment 3•9 years ago
|
||
Comment on attachment 8714470 [details]
MozReview Request: Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle
https://reviewboard.mozilla.org/r/33097/#review29931
Attachment #8714470 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3e53d5cf6400fba69885bd2283e9a23976c7131d
Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle
Comment 5•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Assignee | ||
Comment 6•9 years ago
|
||
Let's make sure this gets uplifted with bug 1205835.
Assignee | ||
Updated•9 years ago
|
tracking-fennec: --- → 45+
Assignee | ||
Comment 7•9 years ago
|
||
(It's not strictly necessary for bug 1205835 though, just an optimization)
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8714470 [details]
MozReview Request: Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle
This is a micro-optimization on the implementation in bug 1205835, which must be uplifted first.
Approval Request Comment
[Feature/regressing bug #]: bug 1205835.
[User impact if declined]: Users send an extra byte (the trailing url slash) when requesting a url. Otherwise, no impact. It's not technically correct according to the server spec but it handles it gracefull.
[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Extremely low – we remove a character from a url string. Worst case, we upload to the wrong url and break uploads.
[String/UUID change made/needed]: None
Attachment #8714470 -
Flags: approval-mozilla-beta?
Attachment #8714470 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox45:
--- → affected
status-firefox46:
--- → affected
Comment 9•9 years ago
|
||
Comment on attachment 8714470 [details]
MozReview Request: Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle
Minor change, taking it.
Should be in 45 beta 4.
Attachment #8714470 -
Flags: approval-mozilla-beta?
Attachment #8714470 -
Flags: approval-mozilla-beta+
Attachment #8714470 -
Flags: approval-mozilla-aurora?
Attachment #8714470 -
Flags: approval-mozilla-aurora+
Comment 10•9 years ago
|
||
bugherder uplift |
Comment 11•9 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 12•9 years ago
|
||
This can't be uplifted without bug 1205835 as it changes files introduced in it. As such, it looks like these uplifted patches created the files in the state they build on top of, when they should be changing one line.
Can this get backed out until bug 1205835 lands?
Flags: needinfo?(sledru)
Flags: needinfo?(cbook)
This (and probably a bunch of the related uplift requests) needs to be rebased to work around the lack of bug 1107811 on the release branches.
Flags: needinfo?(michael.l.comella)
Comment 14•9 years ago
|
||
bugherder uplift |
Comment 15•9 years ago
|
||
bugherder uplift |
Comment 16•9 years ago
|
||
backout |
I had to back this out on both branches so that everything else could land cleanly and in the correct order before finally relanding it.
The backouts are in the following commits:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b923a7b09924
https://hg.mozilla.org/releases/mozilla-beta/rev/9b566531611e
Flags: needinfo?(michael.l.comella)
Comment 17•9 years ago
|
||
Wes did it.
Michael, do we want to land this again? Thanks
Flags: needinfo?(sledru) → needinfo?(michael.l.comella)
Updated•9 years ago
|
Flags: needinfo?(cbook)
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Wes did it.
> Michael, do we want to land this again? Thanks
This is already landed – the commits are posted out of order. The backout in comment 16 occurred before the relanding in comment 15 & comment 14.
Flags: needinfo?(michael.l.comella)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•