Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29759558: Issue 6599 - Hotfix to drop updated page everywhere (Closed)

Created:
April 23, 2018, 7:38 a.m. by a.giammarchi
Modified:
April 24, 2018, 4:01 p.m.
CC:
saroyanm, wspee, rossg
Visibility:
Public.

Description

I think we have definitively a better solution here so there is no point in keeping this opened. https://codereview.adblockplus.org/29760565/

Patch Set 1 #

Total comments: 1

Patch Set 2 : appplied Sebastian's changes #

Total comments: 2

Patch Set 3 : removed also skin/updates #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -40 lines) Patch
M lib/prefs.js View 1 1 chunk +0 lines, -8 lines 0 comments Download
M lib/subscriptionInit.js View 1 2 chunks +2 lines, -23 lines 0 comments Download
M metadata.chrome View 1 2 1 chunk +0 lines, -9 lines 0 comments Download

Messages

Total messages: 15
a.giammarchi
Since we are unable to properly address, or reproduce, the root cause of the update ...
April 23, 2018, 7:42 a.m. (2018-04-23 07:42:23 UTC) #1
kzar
Please could you add this review to the issue, mark it as under review and ...
April 23, 2018, 9:21 a.m. (2018-04-23 09:21:36 UTC) #2
a.giammarchi
On 2018/04/23 09:21:36, kzar wrote: > Please could you add this review to the issue, ...
April 23, 2018, 9:36 a.m. (2018-04-23 09:36:44 UTC) #3
kzar
On 2018/04/23 09:36:44, a.giammarchi wrote: > not sure how to mark it as under review ...
April 23, 2018, 10:04 a.m. (2018-04-23 10:04:58 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29759558/diff/29759559/lib/subscriptionInit.js File lib/subscriptionInit.js (left): https://codereview.adblockplus.org/29759558/diff/29759559/lib/subscriptionInit.js#oldcode251 lib/subscriptionInit.js:251: updatesVersion > Prefs.last_updates_page_displayed) Nit: Please also remove the now ...
April 23, 2018, 7:17 p.m. (2018-04-23 19:17:07 UTC) #5
Sebastian Noack
And we might want to update metadata.chrome too, to not include the unused update page ...
April 23, 2018, 7:22 p.m. (2018-04-23 19:22:55 UTC) #6
Thomas Greiner
My impression was that simply removing the else-condition would be sufficient for the hotfix.
April 24, 2018, 9:31 a.m. (2018-04-24 09:31:29 UTC) #7
a.giammarchi
On 2018/04/24 09:31:29, Thomas Greiner wrote: > My impression was that simply removing the else-condition ...
April 24, 2018, 9:36 a.m. (2018-04-24 09:36:49 UTC) #8
Thomas Greiner
On 2018/04/24 09:36:49, a.giammarchi wrote: > On 2018/04/24 09:31:29, Thomas Greiner wrote: > > My ...
April 24, 2018, 9:48 a.m. (2018-04-24 09:48:49 UTC) #9
Sebastian Noack
On 2018/04/24 09:48:49, Thomas Greiner wrote: > On 2018/04/24 09:36:49, a.giammarchi wrote: > > On ...
April 24, 2018, 9:58 a.m. (2018-04-24 09:58:48 UTC) #10
a.giammarchi
I've applied all requested changes.
April 24, 2018, 10:12 a.m. (2018-04-24 10:12:47 UTC) #11
Thomas Greiner
Based on https://issues.adblockplus.org/ticket/6599#comment:16 we may want to pause this review.
April 24, 2018, 10:20 a.m. (2018-04-24 10:20:15 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome File metadata.chrome (left): https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome#oldcode137 metadata.chrome:137: skin/updates.css = adblockplusui/skin/updates.css What is about the graphical assets ...
April 24, 2018, 10:24 a.m. (2018-04-24 10:24:22 UTC) #13
a.giammarchi
good point, removed skin/updates files too https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome File metadata.chrome (left): https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome#oldcode137 metadata.chrome:137: skin/updates.css = adblockplusui/skin/updates.css ...
April 24, 2018, 10:37 a.m. (2018-04-24 10:37:56 UTC) #14
Sebastian Noack
April 24, 2018, 10:49 a.m. (2018-04-24 10:49:09 UTC) #15
Looks good to me, if we decide to move forward with removing the update page.
But as Thomas pointed out, we might as well want to handle the data corruption
and show a targeted message instead, now where we have new information.

Powered by Google App Engine
This is Rietveld