Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(34)

Issue 29664623: Issue 6008 - Integrated updates page

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week ago by Thomas Greiner
Modified:
19 hours, 36 minutes ago
Reviewers:
kzar, Sebastian Noack
Visibility:
Public.

Description

This review includes the following changes: - Updated adblockplusui (other than the updates page there were merely style changes to the options page) - Included resources for updates page - Introduced new preference: lastUpdatesVersion - Added logic for opening updates page

Patch Set 1 #

Total comments: 37

Patch Set 2 : Cleaned up preferences #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -6 lines) Patch
M dependencies View 1 chunk +1 line, -1 line 0 comments Download
M lib/prefs.js View 1 3 chunks +10 lines, -2 lines 0 comments Download
M lib/subscriptionInit.js View 1 3 chunks +31 lines, -3 lines 0 comments Download
M metadata.chrome View 3 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 15
Thomas Greiner
https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcode204 lib/prefs.js:204: * The version of major updates that the user ...
1 week ago (2018-01-12 16:33:40 UTC) #1
kzar
> (other than the updates page there were merely style changes to the options page) ...
4 days, 18 hours ago (2018-01-15 11:58:36 UTC) #2
kzar
https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js#newcode226 lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion) On 2018/01/15 11:58:35, kzar wrote: > ...
4 days, 18 hours ago (2018-01-15 12:17:20 UTC) #3
Thomas Greiner
It's important to discuss the possible implications of tying the updates version to the addon ...
2 days, 12 hours ago (2018-01-17 18:00:07 UTC) #4
Sebastian Noack
Note that since the update page will announce the new options page, we have to ...
2 days, 8 hours ago (2018-01-17 22:14:43 UTC) #5
kzar
What do you think Sebastian? https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js#newcode224 lib/subscriptionInit.js:224: // Chromium-based browsers to ...
1 day, 19 hours ago (2018-01-18 10:53:14 UTC) #6
Thomas Greiner
On 2018/01/17 22:14:43, Sebastian Noack wrote: > Note that since the update page will announce ...
1 day, 19 hours ago (2018-01-18 11:33:50 UTC) #7
kzar
https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js#newcode233 lib/subscriptionInit.js:233: browser.tabs.create({url: browser.extension.getURL(page)}); On 2018/01/18 10:53:13, kzar wrote: > On ...
1 day, 19 hours ago (2018-01-18 11:38:24 UTC) #8
Thomas Greiner
https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js#newcode233 lib/subscriptionInit.js:233: browser.tabs.create({url: browser.extension.getURL(page)}); On 2018/01/18 11:38:24, kzar wrote: > On ...
1 day, 18 hours ago (2018-01-18 11:50:27 UTC) #9
kzar
https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js#newcode233 lib/subscriptionInit.js:233: browser.tabs.create({url: browser.extension.getURL(page)}); On 2018/01/18 11:50:26, Thomas Greiner wrote: > ...
1 day, 18 hours ago (2018-01-18 12:01:40 UTC) #10
kzar
Just a thought but we should probably be careful to set the lastUpdateVersion when the ...
1 day, 18 hours ago (2018-01-18 12:07:07 UTC) #11
Sebastian Noack
On 2018/01/18 11:33:50, Thomas Greiner wrote: > On 2018/01/17 22:14:43, Sebastian Noack wrote: > > ...
1 day, 17 hours ago (2018-01-18 12:46:48 UTC) #12
Thomas Greiner
On 2018/01/18 12:07:07, kzar wrote: > Just a thought but we should probably be careful ...
1 day, 17 hours ago (2018-01-18 13:36:02 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionInit.js#newcode38 lib/subscriptionInit.js:38: const updatesVersion = 1; On 2018/01/18 13:36:01, Thomas Greiner ...
1 day, 14 hours ago (2018-01-18 16:23:12 UTC) #14
kzar
19 hours, 36 minutes ago (2018-01-19 11:05:24 UTC) #15
https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/18 16:23:12, Sebastian Noack wrote:
> On 2018/01/18 13:36:01, Thomas Greiner wrote:
> > That means that none of our devbuild users will receive the updates page
since
> > they're already on 3.x. Furthermore, it'd make it a bit more complicated to
> show
> > the same updates page to Firefox users later on since they are also already
on
> > 3.x.
> 
> Well, we have to special case either Firefox or Chromium, either way. But you
> have a point, that if we rely on the extension version, the behavior on the
> development builds, would be flawed (and difficult to test). Even if we
wouldn't
> have accidentally advanced the devbuilds to 3.x, the devbuild users (and
> testers) still wouldn't see the update page before the release.

Yea, I'm coming around to the idea of having the separate updatesVersion number
as well, I think Thomas has a point.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/18 13:36:01, Thomas Greiner wrote:
> On 2018/01/18 12:46:48, Sebastian Noack wrote:
> > FWIW, I think, in the (rather theoretical) scenario when some updates with
> > update information have been skipped, it is good enough, to just show the
> latest
> > update information. I expect this scenario to be extremely rare, and
otherwise
> > it will also make testing a pain (and unlikely to happen).
> 
> Ok, so if Dave agrees with that as well I'd not implement this feature at this
> point.
> 
> If we want to we can continue this discussion when we start working on the
next
> version of the updates page.

Alright then, seems I'm outnumbered.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5