Issue 4540363985387520: Embed Adblock Plus (Closed)
|
Created:
Nov. 19, 2014, 3:54 p.m. by Felix Dahlke Modified:
May 21, 2015, 11:49 a.m. Visibility:
Public. |
DescriptionEmbed Adblock Plus
Note that this patch also adds the file a file called adblockplus-2.6.6.3875.xpi - for some reason Rietveld doesn't show it.
Please note that this is based on: http://codereview.adblockplus.org/6349873972510720/
Patch Set 1 #Patch Set 2 : Get rid of distribute_abp, add ABP as subrepository #Patch Set 3 : WIP - Use the extensions directory #
Total comments: 1
Patch Set 4 : WIP - Use the mobile/android/extensions directory #Patch Set 5 : Install as application scope extension #
Total comments: 7
MessagesTotal messages: 19
This is how I've embedded ABP so far: Our latest XPI is added to the repository, and a shell script puts it in the distribute directory, from which ABP for Android will install it at runtime. That's arguably a bit of a hack, but it was a proof of concept. I've looked a bit into extending the mach package command to do this at least, but that'll need some more research. Since this will get us started at least, I'd like to push it as-is and create a follow-up issue for doing it more elegantly.
I don't think that the distribution mechanism makes sense if we create our own builds - the primary purpose of having extensions copied to user's profile is to have them updated there and to allow the user to remove them if necessary. In our case a globally installed extension that is updated with the application makes more sense - like the default theme. Mozilla allows such extensions to be defined during build (see https://developer.mozilla.org/en-US/Add-ons/Creating_Custom_Firefox_Extension...), I wonder whether we can somehow use that mechanism instead of hacking things. Note that Rietveld generally cannot handle binary patches - these are meaningless in the SVN patch format that Rietveld works with :-(
I think I figured this out. |ac_add_options --enable-extensions=default,adblockplus| is merely a way to add a custom step to the build process. It means that the extensions/adblockplus/ directory will be processed during build. This directory doesn't have to contain a browser extension, it can be anything. As to the contents of this directory, we can do it similarly to the default theme: http://hg.mozilla.org/mozilla-central/file/7d17b594834f/browser/app/profile/e.... It's merely placing a file in the extensions/ directory, we would need to do the same. There is also a slight complication. Unlike with the distribution/ directory, we have to modify http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/packag... and make sure our file is listed explicitly.
On 2014/11/20 20:57:33, Wladimir Palant wrote: > I think I figured this out. |ac_add_options > --enable-extensions=default,adblockplus| is merely a way to add a custom step to > the build process. It means that the extensions/adblockplus/ directory will be > processed during build. This directory doesn't have to contain a browser > extension, it can be anything. > > As to the contents of this directory, we can do it similarly to the default > theme: > http://hg.mozilla.org/mozilla-central/file/7d17b594834f/browser/app/profile/e.... > It's merely placing a file in the extensions/ directory, we would need to do the > same. > > There is also a slight complication. Unlike with the distribution/ directory, we > have to modify > http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/packag... > and make sure our file is listed explicitly. I've investigated this a bit last week. Does indeed look like the proper way to bundle extensions, but not as trivial as the distribute mechanism. It seems like we will have to list every file included with ABP (apparently both in /extensions/adblockplus/moz.build and in /mobile/android/installer/package-manifest.in), I really think we should automate that. Also, it seems like we'll have to add "--enable-application=extensions" and avoid listing "default" with "--enable-extensions" - or we just move the extension source to /mobile/android/extensions and modify the build files there. All in all, I think we should stick with the distribute mechanism for now and tackle this in a follow-up.
On 2014/11/29 05:48:10, Felix H. Dahlke wrote: > On 2014/11/20 20:57:33, Wladimir Palant wrote: > > I think I figured this out. |ac_add_options > > --enable-extensions=default,adblockplus| is merely a way to add a custom step > to > > the build process. It means that the extensions/adblockplus/ directory will be > > processed during build. This directory doesn't have to contain a browser > > extension, it can be anything. > > > > As to the contents of this directory, we can do it similarly to the default > > theme: > > > http://hg.mozilla.org/mozilla-central/file/7d17b594834f/browser/app/profile/e.... > > It's merely placing a file in the extensions/ directory, we would need to do > the > > same. > > > > There is also a slight complication. Unlike with the distribution/ directory, > we > > have to modify > > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/packag... > > and make sure our file is listed explicitly. > > I've investigated this a bit last week. Does indeed look like the proper way to > bundle extensions, but not as trivial as the distribute mechanism. > > It seems like we will have to list every file included with ABP (apparently both > in /extensions/adblockplus/moz.build and in > /mobile/android/installer/package-manifest.in), I really think we should > automate that. > > Also, it seems like we'll have to add "--enable-application=extensions" and > avoid listing "default" with "--enable-extensions" - or we just move the > extension source to /mobile/android/extensions and modify the build files there. > > All in all, I think we should stick with the distribute mechanism for now and > tackle this in a follow-up. For some reason I couldn't resist digging into the Mozilla build system tonight. Uploaded a new patch that gets rid of distribute_abp and adds the step to mach's package phase instead. I've looked into adding this as a separate Mach phase, but since Mach is just a relatively thin, optional wrapper around the (apparently still largely autoconf-based) build system, it rather belongs in the makefiles. mobile/android/installer/Makefile.in would have been a good place, but I haven't found any way to run code in there. Thus I added it to mobile/android/build.mk. This is more invasive than running distribute_abp manually, but IMO it's definitely worth it if it gets rid of a manual build step that's easily forgotten. Note that while at it, I also replaced the checked in XPI file by including ABP as a subrepository and building the XPI from that.
On 2014/11/29 05:48:10, Felix H. Dahlke wrote: > All in all, I think we should stick with the distribute mechanism for now and > tackle this in a follow-up. I strongly suspect that we don't - the distribute mechanism will be a mess in our case (see updates). Note that we are only talking about a single XPI file that we want to distribute, so adding it to the lists won't be a big issue.
On 2014/12/01 10:20:28, Wladimir Palant wrote: > I strongly suspect that we don't - the distribute mechanism will be a mess in > our case (see updates). What's wrong with updates? I've just tested what happens if we add a newer version of ABP, and the extension was upgraded just fine. Or am I missing something? > Note that we are only talking about a single XPI file > that we want to distribute, so adding it to the lists won't be a big issue. From what I've seen, the bundled extensions aren't added as XPIs, always the whole directory. Do you think it's going to work if I put just a single XPI into /extensions? That'd be manageable indeed, I'll see if I can get that to work tomorrow. But I get the impression that's what the distribute mechanism is for, bundling XPIs.
On 2014/12/01 16:59:52, Felix H. Dahlke wrote: > What's wrong with updates? I've just tested what happens if we add a newer > version of ABP, and the extension was upgraded just fine. Or am I missing > something? Exactly, the extension updated. So now you have code in your application (e.g. settings) that expects a particular ABP version and you have some random ABP version installed in the profile. That random ABP version might have a different and incompatible API, and things will break horribly. Extensions that are an integral part of the application should be updated together with the application rather than separately from it. But that's exactly what's impossible with the distribution functionality. > Do you think it's going to work if I put just a single XPI into > /extensions? Yes, I'm pretty sure that it will work.
On 2014/12/01 19:46:22, Wladimir Palant wrote: > On 2014/12/01 16:59:52, Felix H. Dahlke wrote: > > What's wrong with updates? I've just tested what happens if we add a newer > > version of ABP, and the extension was upgraded just fine. Or am I missing > > something? > > Exactly, the extension updated. So now you have code in your application (e.g. > settings) that expects a particular ABP version and you have some random ABP > version installed in the profile. That random ABP version might have a different > and incompatible API, and things will break horribly. I actually meant that I first deployed Adblock Browser with ABP version A in it, then with ABP version B in it, and it was version B after installing the new APK. But you're right, I verified this to be sure: Extensions embedded this way will auto-update to the latest version :( So if we do go with the distribute mechanism, we'll have to cut off auto updates for ABP. Too big of a hack. > Extensions that are an integral part of the application should be updated > together with the application rather than separately from it. But that's exactly > what's impossible with the distribution functionality. > > > Do you think it's going to work if I put just a single XPI into > > /extensions? > > Yes, I'm pretty sure that it will work. Alright, will give that a go - seems a lot cleaner now.
I've uploaded a WIP patch for the extensions directory mechanism (it also adds the XPI file in extensions/, not listed here though). I think I did it the way you suggested Wladimir, can you check? Problem is, the extension is not getting installed. I'll research a bit more, but from what I've seen so far, nothing suggests that we can just dump a .xpi there - I do get the impression it needs to be a directory with all the extension files. Trouble is, this is all quite tricky/cumbersome to test. I think hacking the extension updater code to not check for updates if it encounters our extension ID doesn't look all that bad in comparison, for now. http://codereview.adblockplus.org/4540363985387520/diff/5684049913839616/mobi... File mobile/android/build.mk (right): http://codereview.adblockplus.org/4540363985387520/diff/5684049913839616/mobi... mobile/android/build.mk:10: # abp_source_dir=$(topsrcdir)/adblockplus Ignore changes to this file please, we don't need to touch it for the extensions mechanism.
Alright, investigated this further: /extensions definitely seems like the wrong place. How I see it, these aren't extensions in the add-on sense, they're just unfortunately using the same term. I still haven't found any code looking for XPIs in the extensions directory, which doesn't even end up in the distribution unless explicitly added. But there's /browser/extensions, which contains pdfjs and shumway - actual add-ons. /browser is ignored on Android (it's the XUL-based desktop frontend after all), but there's /mobile/android/extensions which serves the same purpose. Fortunately, unlike with /extensions, we don't have to mess with .mozconfig here. I'm still in the middle of implementing this, but I've uploaded a WIP patch for now. It has two issues: 1. The Makefile assumes there's an extracted XPI at /adblockplus - I intend to generate this from adblockplus included as a submodule instead. 2. The extension is not getting initialised (and thus doesn't show the first run page or block ads), we will have to do this manually - similar to how PdfJs.init() is called in browser/components/nsBrowserGlue.js. Neither should pose a problem from what I've seen, stay tuned.
Discussed this with Wladimir today, and he actually proved me wrong and showed me that there's indeed code that loads XPI extensions from the extensions directory! We'll just have to enable this code path then, and won't have to deal with initialising ABP manually.
Finally an approach that works as desired! 1. No automatic updates. 2. The user can't uninstall the extension. 3. Replacing the XPI works fine, even downgrading. The downside is that I had to add code that extracts the extensions directory from the APK and places it in the data directory - apparently there's no way to get Gecko to load it directly from the APK. But considering the alternatives, that looks like the least evil.
This looks good to me but René should check the changes to Distribution.java. http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... File mobile/android/build.mk (right): http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:13: cd $(abp_source_dir); ./build.py build $(abp_xpi_file) No need to change directory, build.py can be called from anywhere: $(abp_source_dir)/build.py build $(DIST)/bin/extensions/$(abp_xpi_file) http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:16: package: package_abp Do we have to mess up this makefile? Won't creating a directory under /extensions with our own build instructions work as well? --enable-extensions=adblockplus needs to be specified but that should be fine.
Java part: LGTM
http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... File mobile/android/build.mk (right): http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:13: cd $(abp_source_dir); ./build.py build $(abp_xpi_file) On 2015/02/18 17:54:01, Wladimir Palant wrote: > No need to change directory, build.py can be called from anywhere: > > $(abp_source_dir)/build.py build $(DIST)/bin/extensions/$(abp_xpi_file) Unfortunately not, the build process expects some stuff in the cwd (e.g. the dependencies file and metadata.gecko). Seems a bit like a bug to me actually, but I wasn't sure if it's supposed to work. That'd allow us to simplify the code here quite a bit, but I'd rather not have this blocked. http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:16: package: package_abp On 2015/02/18 17:54:01, Wladimir Palant wrote: > Do we have to mess up this makefile? Won't creating a directory under > /extensions with our own build instructions work as well? > --enable-extensions=adblockplus needs to be specified but that should be fine. /extensions doesn't have anything to do with the extensions directory that ends up in the APK - the former has native extensions (not add ons), definitely the wrong place for us IMO. Secondly, modifying that makefile is the only way I've found to add this to the package phase, rather than the build phase. I think it belongs there, and it gives us a faster turnaround.
LGTM http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... File mobile/android/build.mk (right): http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:13: cd $(abp_source_dir); ./build.py build $(abp_xpi_file) On 2015/02/19 10:54:14, Felix H. Dahlke wrote: > Unfortunately not, the build process expects some stuff in the cwd (e.g. the > dependencies file and metadata.gecko). Seems a bit like a bug to me actually, Yes, it is indeed a bug - while all the code works with any directory, our build.py boilerplate passes in '.' as basedir :-( http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:16: package: package_abp On 2015/02/19 10:54:14, Felix H. Dahlke wrote: > /extensions doesn't have anything to do with the extensions directory that ends > up in the APK - the former has native extensions (not add ons), definitely the > wrong place for us IMO. I know - it's build system extensions, meant for custom build steps. And here we have one. > Secondly, modifying that makefile is the only way I've found to add this to the > package phase, rather than the build phase. I think it belongs there, and it > gives us a faster turnaround. While technically this might be true, it will also make updates more complicated. But that's up to you.
On 2015/02/19 18:03:32, Wladimir Palant wrote: > LGTM > > http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... > File mobile/android/build.mk (right): > > http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... > mobile/android/build.mk:13: cd $(abp_source_dir); ./build.py build > $(abp_xpi_file) > On 2015/02/19 10:54:14, Felix H. Dahlke wrote: > > Unfortunately not, the build process expects some stuff in the cwd (e.g. the > > dependencies file and metadata.gecko). Seems a bit like a bug to me actually, > > Yes, it is indeed a bug - while all the code works with any directory, our > build.py boilerplate passes in '.' as basedir :-( > > http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... > mobile/android/build.mk:16: package: package_abp > On 2015/02/19 10:54:14, Felix H. Dahlke wrote: > > /extensions doesn't have anything to do with the extensions directory that > ends > > up in the APK - the former has native extensions (not add ons), definitely the > > wrong place for us IMO. > > I know - it's build system extensions, meant for custom build steps. And here we > have one. IIRC enable-extensions doesn't even have any effect in Android, but I don't fully recall. I created a follow-up for this, should be investigated when we have more resources: https://trello.com/c/FFcSMEd2/50-find-a-less-intrusive-way-of-integrating-abp... If we can indeed add it there, I could live with having this in the build phase, it shouldn't make the turnarounds that much longer.
http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... File mobile/android/build.mk (right): http://codereview.adblockplus.org/4540363985387520/diff/5732694713434112/mobi... mobile/android/build.mk:13: cd $(abp_source_dir); ./build.py build $(abp_xpi_file) On 2015/02/19 18:03:32, Wladimir Palant wrote: > On 2015/02/19 10:54:14, Felix H. Dahlke wrote: > > Unfortunately not, the build process expects some stuff in the cwd (e.g. the > > dependencies file and metadata.gecko). Seems a bit like a bug to me actually, > > Yes, it is indeed a bug - while all the code works with any directory, our > build.py boilerplate passes in '.' as basedir :-( Here we go :) https://issues.adblockplus.org/ticket/2020 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
