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

Issue 29664623: Issue 6403 - Updated adblockplusui dependency (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by Thomas Greiner
Modified:
4 months, 3 weeks ago
Visibility:
Public.

Description

See also https://codereview.adblockplus.org/29680702/ for related changes to adblockplusui.

Patch Set 1 #

Total comments: 37

Patch Set 2 : Cleaned up preferences #

Patch Set 3 : Moved updatesVersion to adblockplusui #

Total comments: 12

Patch Set 4 : Made minor adjustments #

Patch Set 5 : Renamed pref and changed comment #

Patch Set 6 : Updated dependency to include adblockplusui/lib/prefs module #

Patch Set 7 : Rebased to 8b5f5dfaac03 #

Patch Set 8 : Updated dependency to include translations #

Total comments: 2

Patch Set 9 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+91 lines, -30 lines) Patch
M dependencies View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M lib/prefs.js View 1 2 3 4 3 chunks +10 lines, -2 lines 0 comments Download
M lib/subscriptionInit.js View 1 2 3 4 5 3 chunks +27 lines, -3 lines 0 comments Download
M metadata.chrome View 1 2 3 4 5 6 7 3 chunks +53 lines, -4 lines 0 comments Download
M metadata.gecko View 1 2 3 4 5 6 7 1 chunk +0 lines, -20 lines 0 comments Download

Messages

Total messages: 29
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 ...
6 months, 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) ...
6 months 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: > ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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: > ...
6 months 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 ...
6 months 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: > > ...
6 months 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 ...
6 months 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 ...
6 months ago (2018-01-18 16:23:12 UTC) #14
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/18 16:23:12, Sebastian Noack wrote: ...
6 months ago (2018-01-19 11:05:24 UTC) #15
Thomas Greiner
In my latest patch I've moved the `updatesVersion` constant into adblockplusui by introducing a lib/ui.js ...
5 months, 4 weeks ago (2018-01-22 17:10:22 UTC) #16
kzar
I'm much happier with this now, thanks for those changes. A few nits remain. https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js ...
5 months, 3 weeks ago (2018-01-23 10:07:14 UTC) #17
Thomas Greiner
https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcode209 lib/prefs.js:209: defaults.updates_version = 0; On 2018/01/23 10:07:13, kzar wrote: > ...
5 months, 3 weeks ago (2018-01-25 11:09:45 UTC) #18
kzar
https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcode209 lib/prefs.js:209: defaults.updates_version = 0; On 2018/01/25 11:09:45, Thomas Greiner wrote: ...
5 months, 3 weeks ago (2018-01-25 11:46:26 UTC) #19
Thomas Greiner
https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcode209 lib/prefs.js:209: defaults.updates_version = 0; On 2018/01/25 11:46:25, kzar wrote: > ...
5 months, 3 weeks ago (2018-01-25 13:09:04 UTC) #20
kzar
Once the dependency update is sorted this LGTM
5 months, 3 weeks ago (2018-01-25 13:13:26 UTC) #21
Thomas Greiner
Updated dependency to reflect changes from https://codereview.adblockplus.org/29680702/. We're still waiting for community translations though (see ...
5 months, 2 weeks ago (2018-01-31 12:03:26 UTC) #22
Thomas Greiner
I've updated the dependency so that it includes the translations. As mentioned in #6403 this ...
4 months, 3 weeks ago (2018-02-22 17:28:31 UTC) #23
kzar
https://codereview.adblockplus.org/29664623/diff/29705802/dependencies File dependencies (right): https://codereview.adblockplus.org/29664623/diff/29705802/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:2a04df826943 git:04b38a9 There have been more ...
4 months, 3 weeks ago (2018-02-23 10:17:47 UTC) #24
Thomas Greiner
https://codereview.adblockplus.org/29664623/diff/29705802/dependencies File dependencies (right): https://codereview.adblockplus.org/29664623/diff/29705802/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:2a04df826943 git:04b38a9 On 2018/02/23 10:17:46, kzar ...
4 months, 3 weeks ago (2018-02-23 11:57:15 UTC) #25
kzar
So just to confirm this now includes all the translations for the new UI(s) that ...
4 months, 3 weeks ago (2018-02-23 15:14:32 UTC) #26
saroyanm
On 2018/02/23 15:14:32, kzar wrote: > So just to confirm this now includes all the ...
4 months, 3 weeks ago (2018-02-23 15:48:01 UTC) #27
Thomas Greiner
On 2018/02/23 15:14:32, kzar wrote: > Also you've tested the changes again with the latest ...
4 months, 3 weeks ago (2018-02-23 16:34:41 UTC) #28
kzar
4 months, 3 weeks ago (2018-02-23 17:05:20 UTC) #29
Message was sent while issue was closed.
On 2018/02/23 16:34:41, Thomas Greiner wrote:

> I did some rough testing, yes. The first-run page, updates page, desktop
options
> page as well as mobile options page all load without issues on both Chrome and
> Firefox.

Cool OK, thanks sounds good to me.
Sign in to reply to this message.

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