First run page and subscription on install fix (FF for android)

This fix is related to current ticket:
https://issues.adblockplus.org/ticket/99

[saroyanm, 2014-04-25 12:08:06]

I've noticed that the UI module in appSupport.js (ln:832) is not being loaded.
Actually seams like this is somehow connected to load of appSupport module in
customizableUI.js (ln:288) on first run, 
I'm not sure, but my guess that could be circular reference issue, would really
appreciate your comment on this Wladimir.

Actually this fix looks to work Okey.

http://codereview.adblockplus.org/5705421486555136/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5705421486555136/diff/5629499534213120/lib/...
lib/ui.js:273: ({CustomizableUI}) =
Cu.import("resource:///modules/CustomizableUI.jsm", null);
Not sure if we need this here ?

[Wladimir Palant, 2014-04-25 12:39:54]

http://codereview.adblockplus.org/5705421486555136/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5705421486555136/diff/5629499534213120/lib/...
lib/ui.js:279: }
This doesn't resolve the circular reference. The modules load like this: ui =>
customizableUI => appSupport => ui. The original issue was that the ui module
didn't set exports.UI yet when appSupport loaded. Moving code around left the
circular reference but appSupport loads a bit later, when exports.UI is already
set.

Thing is: we don't even need CustomizableUI in Firefox Mobile, there is no
toolbar icon there. So I suggest you move CustomizableUI initialization into
UI.initDone(), inside the |if ("abp-toolbarbutton" in this.overlay &&
defaultToolbarPosition)| block. This way it won't be loaded if there is no
toolbar button.

[saroyanm, 2014-04-25 14:04:22]

Wladimir can you please have a look on updated patch.

http://codereview.adblockplus.org/5705421486555136/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5705421486555136/diff/5629499534213120/lib/...
lib/ui.js:279: }
On 2014/04/25 12:39:54, Wladimir Palant wrote:
> This doesn't resolve the circular reference. The modules load like this: ui =>
> customizableUI => appSupport => ui. The original issue was that the ui module
> didn't set exports.UI yet when appSupport loaded. Moving code around left the
> circular reference but appSupport loads a bit later, when exports.UI is
already
> set.
> 
> Thing is: we don't even need CustomizableUI in Firefox Mobile, there is no
> toolbar icon there. So I suggest you move CustomizableUI initialization into
> UI.initDone(), inside the |if ("abp-toolbarbutton" in this.overlay &&
> defaultToolbarPosition)| block. This way it won't be loaded if there is no
> toolbar button.

Thanks for descriptive comment.
Done.

[Wladimir Palant, 2014-04-25 14:32:19]

http://codereview.adblockplus.org/5705421486555136/diff/5086100271923200/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5705421486555136/diff/5086100271923200/lib/...
lib/ui.js:150: }
Same change would have to be done in fillIconMenu() as well. IMHO it is safer to
change isToolbarIconVisible() and toggleToolbarIcon() to check whether
CustomizableUI is set - that way future introducing new callers to these
functions won't break for non-obvious reasons.

[saroyanm, 2014-04-25 15:10:21]

The patch Updated.

http://codereview.adblockplus.org/5705421486555136/diff/5086100271923200/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5705421486555136/diff/5086100271923200/lib/...
lib/ui.js:150: }
On 2014/04/25 14:32:19, Wladimir Palant wrote:
> Same change would have to be done in fillIconMenu() as well. IMHO it is safer
to
> change isToolbarIconVisible() and toggleToolbarIcon() to check whether
> CustomizableUI is set - that way future introducing new callers to these
> functions won't break for non-obvious reasons.

Good point, thanks.

[Wladimir Palant, 2014-04-25 15:23:34]

LGTM