Issue 6286955629248512: disable acceptable ads from first run page (Closed)
|
Created:
April 30, 2014, 1:16 p.m. by saroyanm Modified:
May 9, 2014, 7:07 a.m. Reviewers:
Wladimir Palant CC:
Philip Hill Visibility:
Public. |
DescriptionThis Review is related to current issue:
https://issues.adblockplus.org/ticket/255
Patch Set 1 #
Total comments: 1
Patch Set 2 : trigger popstate event #
Total comments: 9
Patch Set 3 : Fixes after Wladimir comments #
Total comments: 4
Patch Set 4 : state declaration in runasync function #Patch Set 5 : call getAddonsByTypes method #
Total comments: 5
Patch Set 6 : Description to getAddonsByTypes method and minore changes #MessagesTotal messages: 12
Wladimir can you please have a look on this review, I've updated the issue with description why I'm having no chance to navigate into addons preferences page: https://issues.adblockplus.org/ticket/255#comment:3 Hope we are okey with current sollution. Thanks in advance. http://codereview.adblockplus.org/6286955629248512/diff/5629499534213120/chro... File chrome/locale/en-US/global.properties (right): http://codereview.adblockplus.org/6286955629248512/diff/5629499534213120/chro... chrome/locale/en-US/global.properties:38: disable_aa_promt_cancel=Cancel Not sure if I should move this to "firstRun.properties".
I don't think this is the right approach - we should override
UI.openFiltersDialogs() in appSupport.js similarly to how it is done for
UI.openSubscriptionDialog(). It should still open add-on settings. While I
realize that there is no *supported* way to do this, there seems to be a way
that is safe enough: open about:addons in a new tab, wait for it to load and
then dispatch a "popstate" event where state is {id: require("info").addonID}.
The code behind the about:addons page can be seen under
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/a....
Thanks for your comment Wladimir, I really like your solution, the only thing that I couldn't find better way to trigger the event after tab load rather then setting timeout. Hope we will be Okey with new patch. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:930: browser._contentWindow.setTimeout(function() Couldn't find better approach while aboutAddons.js using window.addEventListener("load", init, false); to add popstate event in init method.
http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:922: let window = this.currentWindow; Please use UI.currentWindow explicitly - it wasn't really obvious to me what "this" means here. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:926: let browser = window.BrowserApp.addTab("about:addons").browser; How about: let browser = exports.addTab(window, "about:addons").browser; That function is defined above, this way you won't duplicate its code. Also, it will automatically select the new tab. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:930: browser._contentWindow.setTimeout(function() On 2014/05/02 11:18:30, saroyanm wrote: > Couldn't find better approach while aboutAddons.js using > window.addEventListener("load", init, false); > to add popstate event in init method. Yes, you need to run this immediately after the page processes the "load" event. Please use Utils.runAsync() for that rather than using random timeout values. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:937: }, true); I don't really see a point for having a capturing event listener here, won't it also work if you use addEventListener("load", ..., false)?
Thanks for your notes Wladimir, New patch uploaded. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:922: let window = this.currentWindow; On 2014/05/02 11:50:00, Wladimir Palant wrote: > Please use UI.currentWindow explicitly - it wasn't really obvious to me what > "this" means here. Done. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:926: let browser = window.BrowserApp.addTab("about:addons").browser; On 2014/05/02 11:50:00, Wladimir Palant wrote: > How about: > > let browser = exports.addTab(window, "about:addons").browser; > > That function is defined above, this way you won't duplicate its code. Also, it > will automatically select the new tab. Sorry for not being attentive on that.. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:930: browser._contentWindow.setTimeout(function() On 2014/05/02 11:50:00, Wladimir Palant wrote: > On 2014/05/02 11:18:30, saroyanm wrote: > > Couldn't find better approach while aboutAddons.js using > > window.addEventListener("load", init, false); > > to add popstate event in init method. > > Yes, you need to run this immediately after the page processes the "load" event. > Please use Utils.runAsync() for that rather than using random timeout values. Thanks for pointing on the method, gave me understanding behind threadManagement in FF. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/... lib/appSupport.js:937: }, true); On 2014/05/02 11:50:00, Wladimir Palant wrote: > I don't really see a point for having a capturing event listener here, won't it > also work if you use addEventListener("load", ..., false)? Yes actually It will not be triggered, not sure why, no success on understanding the case. The only thing I've noticed most of the load events attached to browser for mobile FF are using UseCapture param set to true: http://mxr.mozilla.org/mobile-browser/search?string=addEventListener%28%22loa... https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/Code_snippets...
http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... lib/appSupport.js:936: }, this, {id: require("info").addonID}); Passing in this and the state parameter is unnecessary - just use require("info") inside the function as before. For reference: new code shouldn't use Utils.runAsync() with more than one parameter, that's what Function.bind() is good for (it didn't exist back when Utils.runAsync() was written).
New patch uploaded. Sorry for late answer. http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... lib/appSupport.js:936: }, this, {id: require("info").addonID}); On 2014/05/02 16:36:47, Wladimir Palant wrote: > Passing in this and the state parameter is unnecessary - just use > require("info") inside the function as before. > > For reference: new code shouldn't use Utils.runAsync() with more than one > parameter, that's what Function.bind() is good for (it didn't exist back when > Utils.runAsync() was written). My bad that I didn't left comment on this. When I was declaring require("info") inside the function I was getting current Error (In 3/5 tests) : TypeError: aListItem is null http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/a... I thought it's somehow connected with async function. Actually Now seams like that I'm not getting that error anymore, not sure why.. Tested around 10 times and only 1-st time got that error (Have a doubt that it was because of cache). I've changed declaration as it was before, if you can drop a comment on this Wladimir, I will really appreciate that. P.S. For the parameters in runAsync() function, will keep in mind for future. Thanks.
http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... lib/appSupport.js:936: }, this, {id: require("info").addonID}); On 2014/05/03 08:02:06, saroyanm wrote: > I was getting current > Error (In 3/5 tests) : > TypeError: aListItem is null > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/a... That's indeed an issue. The list of add-ons is being filled by Addons.init() which calls the asynchronous method AddonManager.getAddonsByTypes(). The first time it is called the add-on manager will read the information from disk which takes some time - so we dispatch our popstate event too early, the list hasn't been filled yet. I don't see any good way to solve this issue, there is no event that we can listen to in order to see that the list has been filled. And using some timeout value is always error-prone - we have no idea how much time this operation might take on a mobile device. If anything, we could call AddonManager.getAddonsByTypes(["extension", "theme", "locale"], callback) ourselves and dispatch the event in the callback. However, I doubt that this is a reliable solution. Want to try this?
Wladimir I've updated the patch with your suggested solution which works more reliable (Replied also under you comment). Thanks. http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/... lib/appSupport.js:936: }, this, {id: require("info").addonID}); On 2014/05/03 17:54:18, Wladimir Palant wrote: > On 2014/05/03 08:02:06, saroyanm wrote: > > I was getting current > > Error (In 3/5 tests) : > > TypeError: aListItem is null > > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/a... > > That's indeed an issue. The list of add-ons is being filled by Addons.init() > which calls the asynchronous method AddonManager.getAddonsByTypes(). The first > time it is called the add-on manager will read the information from disk which > takes some time - so we dispatch our popstate event too early, the list hasn't > been filled yet. > > I don't see any good way to solve this issue, there is no event that we can > listen to in order to see that the list has been filled. And using some timeout > value is always error-prone - we have no idea how much time this operation might > take on a mobile device. > > If anything, we could call AddonManager.getAddonsByTypes(["extension", "theme", > "locale"], callback) ourselves and dispatch the event in the callback. However, > I doubt that this is a reliable solution. Want to try this? Thanks for descriptive answer Wladimir, I also have a doubt of using that method, but while there seams not to be any better solution, that solution is better then patch #4. As you already mentioned addons list is being populated in callback function of AddonManager.getAddonsByTypes: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/a... So seams like with this solution we saving time and it works more reliable then the previous one after my tests. http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... lib/appSupport.js:937: if (addonItem) Maybe we can use opposite case and implement similar solution as in patch #1 ?
http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... lib/appSupport.js:933: AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function() Please add a comment on why we are doing this. E.g.: "The page won't be ready until the add-on manager data is loaded so we ask the add-on manager for this data to know when this will be." http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... lib/appSupport.js:937: if (addonItem) Please remove this check, it is making assumptions about how the add-ons page is structured. The point of calling AddonManager.getAddonsByType() was exactly that we don't need to do this.
Uploaded new patch, hope we are ready to go.. http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... lib/appSupport.js:933: AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function() On 2014/05/08 13:38:00, Wladimir Palant wrote: > Please add a comment on why we are doing this. E.g.: "The page won't be ready > until the add-on manager data is loaded so we ask the add-on manager for this > data to know when this will be." Done. http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/... lib/appSupport.js:937: if (addonItem) On 2014/05/08 13:38:00, Wladimir Palant wrote: > Please remove this check, it is making assumptions about how the add-ons page is > structured. The point of calling AddonManager.getAddonsByType() was exactly that > we don't need to do this. Done.
LGTM 
|
