1124185 - Replace removeAllPages with history.clear() and deprecate it

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

now that we have history.clear() and it has tests coverage, it's time to replace removeAllPages usage in code and mark it as deprecated.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://mxr.mozilla.org/mozilla-central/search?string=removeAllPages

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Should not be too hard.
The scope here is to replace the calls to removeAllPages() with PlacesUtils.history.clear(), eventually waiting for the promise, or using .catch(Components.utils.reportError) where we don't need to wait.
The files under metro/ folder can be ignored.
Finally, need to add PLACES_WARN_DEPRECATED(); to RemoveAllPages() in nsNavHistory.cpp

Comment 3

[Daniel Miller]

Hi Marco,
I'd like to work on this bug. I am coming to this on the advice of the person who mentored my first bug (https://bugzilla.mozilla.org/show_bug.cgi?id=699860#c33). I am going to start wrapping my head around the code, but is there anything you'd like me to be aware of before I start working on a patch?

Thanks,
Daniel

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Hi,
Yes, I'd suggest to create 3 separate patches here:
1. changes in services/sync/
2. changes in addon-sdk/
3. all of the other changes

I will feedback part 1 and 2, and will review part 3, I will ask other reviewers for part 1 and 2.

this is the current list of consumers:
http://mxr.mozilla.org/mozilla-central/search?string=removeAllPages

browser/base/content/sanitize.js
In this case you will likely need to .catch(Cu.reportError) since you cannot yield, 
bug 1089695 will change that but it's currently still being worked on

services/sync/modules/engines/history.js
you should likely do the same here, but I'm not sure if tests will be happy... so the first patch should just do the replacement like in sanitize... if tests will fail we will have to make this sync method use an async callback... this can be done using
let cb = Async.makeSpinningCallback();
cb.wait();
Then just invoke cb() when done.

toolkit/components/places/nsNavHistory.cpp
This is where you should put the PLACES_WARN_DEPRECATED(); call

toolkit/components/places/nsIBrowserHistory.idl 
Here you should update the javadoc with
@deprecated Please use PlacesUtils.history.clear() instead.

toolkit/components/places/nsPlacesExpiration.js
Here there is only one comment to s/removeAllPages/History.clear()

addon-sdk/source/test/addons/places/lib/places-helper.js
Here you must invoke the done callback at the right time, I think there's no need for this to wait for the expiration topic, so you can remove that and we'll ask review to the appropriate person.

Comment 5

[Daniel Miller]

Thanks Marco.

Is there an expected/preferred timeline for completion?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

no, this is not urgent. Clearly it's better if it doesn't take months, or we will both forget what we are doing :)

Comment 7

[Amod [:AMoz]]

Hi Daniel, 
Would you please confirm your interest on this bug ? So that I can search for another bug to work on.

Comment 8

[Voltaire]

Hi Daniel and Amod,

I'm also interested by this bug. Give me its status now ? Taken or not taken ?

Thx

Comment 9

[Daniel Miller]

(In reply to Amod [:AMoz] from comment #7)
> Hi Daniel, 
> Would you please confirm your interest on this bug ? So that I can search
> for another bug to work on.

Hi Amod,

I have not yet been able to spend time on this bug, so please do take it.

Daniel

Comment 10

[Amod [:AMoz]]

Thank you Daniel. :)
Voltaire, I am returning to Bugzilla after a very long time, so thought of taking few simple bugs to get started. If you need any help in searching a bug for you, so let me know.

Comment 11

[Amod [:AMoz]]

Marco, Hi. I will upload the first patch within a day or two.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

sounds good, thanks

Comment 13

[Amod [:AMoz]]

Attachment: DemoPatch

Hi Mak,
I got introduced to | Promises | through this Bug. So, as of now, I have changed only one instance.
Apologies in advance if my changes deviate from acceptable limits.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8621951 [details] [diff] [review]
DemoPatch

Review of attachment 8621951 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, first of all, I must unfortunately tell that sanitize.js is being fixed in bug 1089695, so there should be nothing left to do here on that file.

But I'm going to feedback regardless cause it might be useful for the other conversions.

::: browser/base/content/sanitize.js
@@ +265,5 @@
>  
>          if (this.range)
>            PlacesUtils.history.removeVisitsByTimeframe(this.range[0], this.range[1]);
> +        else {
> +          var promise = new Promise(function(resolve, reject) {

Assigning this to a promise is not useful cause then you are not doing anything with the promise.
Usually on a promise you either want to wait or you want to at least .catch() failures

@@ +266,5 @@
>          if (this.range)
>            PlacesUtils.history.removeVisitsByTimeframe(this.range[0], this.range[1]);
> +        else {
> +          var promise = new Promise(function(resolve, reject) {
> +            PlacesUtils.history.clear().done(function(res){

Which promises documentation are you looking at?
Please have a look here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
there no .done() method, there is a .then method.

@@ +268,5 @@
> +        else {
> +          var promise = new Promise(function(resolve, reject) {
> +            PlacesUtils.history.clear().done(function(res){
> +              try {
> +                fulfill(JSON.parse(res));

the resolving method you passed to new Promise is resolve, not fulfill
And you don't get back a JSON string. I wonder if you are looking at some node.js promise library documentation...

@@ +269,5 @@
> +          var promise = new Promise(function(resolve, reject) {
> +            PlacesUtils.history.clear().done(function(res){
> +              try {
> +                fulfill(JSON.parse(res));
> +              } catch (Components.utils.reportError Ex) {

what I meant with catching with reportError was
{promise}.catch(Components.utils.reportError), not a try/catch construct

@@ +272,5 @@
> +                fulfill(JSON.parse(res));
> +              } catch (Components.utils.reportError Ex) {
> +                reject(Ex);
> +              }
> +            }, reject);

In any case, clear() already gives you back a promise, so there should be no need to wrap it with another promise

Comment 15

[Andrew Krawchyk]

Attachment: replacing removeAllPages with history.clear() progress

Looks like this hasn't been worked on in a while so I'm going to take a stab at it :)

I'm not sure what to do in the case of http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/sync/xpcshell.ini#12 so any help would be appreciated with it. My other notes are inline below.


(In reply to Marco Bonardo [::mak] from comment #4)
> Hi,
> Yes, I'd suggest to create 3 separate patches here:
> 1. changes in services/sync/
> 2. changes in addon-sdk/
> 3. all of the other changes
> 
> I will feedback part 1 and 2, and will review part 3, I will ask other
> reviewers for part 1 and 2.
> 
> this is the current list of consumers:
> http://mxr.mozilla.org/mozilla-central/search?string=removeAllPages
> 
> browser/base/content/sanitize.js
> In this case you will likely need to .catch(Cu.reportError) since you cannot
> yield, 
> bug 1089695 will change that but it's currently still being worked on


I don't think this file needs to be updated because bug 1089695 has been fixed and `browser/base/content/sanitize.js` has been updated with a yield statement: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#345


> services/sync/modules/engines/history.js
> you should likely do the same here, but I'm not sure if tests will be
> happy... so the first patch should just do the replacement like in
> sanitize... if tests will fail we will have to make this sync method use an
> async callback... this can be done using
> let cb = Async.makeSpinningCallback();
> cb.wait();
> Then just invoke cb() when done.


I used a yield statement, like above, here as well. I tried running automated tests and everything seemed OK, but it's my first time doing so locally. Is try server access hard to get?


> addon-sdk/source/test/addons/places/lib/places-helper.js
> Here you must invoke the done callback at the right time, I think there's no
> need for this to wait for the expiration topic, so you can remove that and
> we'll ask review to the appropriate person.


I'm not sure what to do with this file either, I don't know what to import but I'm thinking I should `require('sdk/places/utils')`?

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

thanks, changing assignee

(In reply to akrawchyk from comment #15)
> I'm not sure what to do in the case of
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> tests/sync/xpcshell.ini#12 so any help would be appreciated with it.

Ugh! I had no idea this stuff was still around.
The whole /sync folder must be removed, it's old stuff, not even built.

> I don't think this file needs to be updated because bug 1089695 has been
> fixed and `browser/base/content/sanitize.js` has been updated with a yield
> statement:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.
> js#345

Right

> > services/sync/modules/engines/history.js
> > you should likely do the same here, but I'm not sure if tests will be
> > happy... so the first patch should just do the replacement like in
> > sanitize... if tests will fail we will have to make this sync method use an
> > async callback... this can be done using
> > let cb = Async.makeSpinningCallback();
> > cb.wait();
> > Then just invoke cb() when done.
> 
> 
> I used a yield statement, like above, here as well. I tried running
> automated tests and everything seemed OK, but it's my first time doing so
> locally. Is try server access hard to get?

Usually not, but I think you should first have a couple patches reviewed and landed to get vouched. I can push to Try for you though. just needinfo me when you need that.

> > addon-sdk/source/test/addons/places/lib/places-helper.js
> > Here you must invoke the done callback at the right time, I think there's no
> > need for this to wait for the expiration topic, so you can remove that and
> > we'll ask review to the appropriate person.
> 
> 
> I'm not sure what to do with this file either, I don't know what to import
> but I'm thinking I should `require('sdk/places/utils')`?

the clearHistory call is using hsrv.removeAllPages(); I think it would be better to import PlacesUtils.jsm and start from there...but I'm not sure if there's a requirement to not use jsms in the sdk, since I see it's still using the old service getters. Maybe Dave knows.

Comment 17

[Dave Townsend [:mossop]]

(In reply to Marco Bonardo [::mak] from comment #16)
> > > addon-sdk/source/test/addons/places/lib/places-helper.js
> > > Here you must invoke the done callback at the right time, I think there's no
> > > need for this to wait for the expiration topic, so you can remove that and
> > > we'll ask review to the appropriate person.
> > 
> > 
> > I'm not sure what to do with this file either, I don't know what to import
> > but I'm thinking I should `require('sdk/places/utils')`?
> 
> the clearHistory call is using hsrv.removeAllPages(); I think it would be
> better to import PlacesUtils.jsm and start from there...but I'm not sure if
> there's a requirement to not use jsms in the sdk, since I see it's still
> using the old service getters. Maybe Dave knows.

It's fine to use jsms.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8672429 [details] [diff] [review]
replacing removeAllPages with history.clear() progress

Review of attachment 8672429 [details] [diff] [review]:
-----------------------------------------------------------------

there is also a call in /browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js

::: browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ +1687,5 @@
>  
>    testObserver = new UrlDeletionTester();
>    DirectoryLinksProvider.addObserver(testObserver);
> +  // remove all history
> +  PlacesUtils.history.clear()

To stay on the safe side, this should import PlacesTestUtils and yield PlacesTestUtils.clearHistory(), that way it will properly wait for the operation to complete.
Generally tests should do this

::: services/sync/modules/engines/history.js
@@ +364,5 @@
>      return record;
>    },
>  
>    wipe: function HistStore_wipe() {
> +    yield PlacesUtils.history.clear();

wipe is not a generator, and it's not used like a generator, so using yield here is not going to work.
it is used ina bunch of places (http://mxr.mozilla.org/mozilla-central/search?string=.wipe%28%29&find=sync) and thus I'm not comfortable blindly making it async.
For now let's use the Async.waitForSyncCallback() trick (see on mxr), I know there's an ongoing effort to convert engines to be async, so that will also remove the need for this...

Comment 19

[Andrew Krawchyk]

Attachment: Bug_1124185.diff

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8674020 [details] [diff] [review]
Bug_1124185.diff

Review of attachment 8674020 [details] [diff] [review]:
-----------------------------------------------------------------

it's mostly good, just some minor details.

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +13,5 @@
>  const tagsrv = Cc['@mozilla.org/browser/tagging-service;1'].
>                getService(Ci.nsITaggingService);
>  const asyncHistory = Cc['@mozilla.org/browser/history;1'].
>                getService(Ci.mozIAsyncHistory);
> +const PlacesUtils = Cu.import("resource://gre/modules/PlacesUtils.jsm");

I'd suggest to use defineLazyModuleGetter for this.

If you wish, in a separate bug to be filed yet, you could remove all of the above service instantiations and directly use PlacesUtils getters all around this file. It would make future conversions easier.

::: services/sync/modules/engines/history.js
@@ +365,5 @@
>    },
>  
>    wipe: function HistStore_wipe() {
> +    let cb = Async.makeSyncCallback();
> +    PlacesUtils.history.clear().then(cb).catch(Cu.reportError);

I'd invert the catch and the then, the idea is that we should continue regardless, so if you put the catch before, it will handle eventual issues, a following then would be invoked regardless of success or failure.
It would also be wise to ask feedback to :markh about this change, so that at least he's aware of it.

Comment 21

[Andrew Krawchyk]

Attachment: Use defineLazyModuleGetter and rework promises order

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8674649 [details] [diff] [review]
Use defineLazyModuleGetter and rework promises order

Review of attachment 8674649 [details] [diff] [review]:
-----------------------------------------------------------------

Gotta love fixing deprecated code :) But I think Mak should have a look over this too...

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +25,5 @@
>    save, search,
>    MENU, TOOLBAR, UNSORTED
>  } = require('sdk/places/bookmarks');
>  
> +XPCOMUtils.defineLazyModuleGetter(LoopUI, "PlacesUtils",

LoopUI doesn't seem right here - I'm surprised it works at all?

@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.clear();

I'd think that this would be better written as:

PlacesUtils.history.clear().then(done, err => <whatever makes sense on error, even if just Cu.reportError>);

as it removes an somewhat obscure and hidden assumption on observers (and it's not immediately clear to me that the notification will now be sent - at the very least, it looks like history.clear() may well do more work after it is. Mak might know more here.

::: services/sync/modules/engines/history.js
@@ +366,5 @@
>  
>    wipe: function HistStore_wipe() {
> +    let cb = Async.makeSyncCallback();
> +    PlacesUtils.history.clear().catch(Cu.reportError).then(cb);
> +    return Async.waitForSyncCallback();

waitForSyncCallback should be passed |cb| - but given wipe could (presumably) throw before, I think this should be:

let cb = Async.makeSyncCallback();
PlacesUtils.history.clear().then(result => {cb(null, result)}, err => {cb(err)});
Async.waitForSyncCallback(cb);

to keep the same semantics as before.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mark Hammond [:markh] from comment #22)
>  function clearHistory (done) {
> > +  PlacesUtils.history.clear();
> 
> I'd think that this would be better written as:
> 
> PlacesUtils.history.clear().then(done, err => <whatever makes sense on
> error, even if just Cu.reportError>);

I think Cu.reportError is fine

> as it removes an somewhat obscure and hidden assumption on observers (and
> it's not immediately clear to me that the notification will now be sent - at
> the very least, it looks like history.clear() may well do more work after it
> is. Mak might know more here.

So far it's basically acting the same as the old API (modulo sync/async), it may change in the future since I'd like to remove some pieces from the expiration component and merge them back to clear.

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8674649 [details] [diff] [review]
Use defineLazyModuleGetter and rework promises order

please address Mark's feedback first.

Comment 25

[Andrew Krawchyk]

Attachment: changeset_adjustments.patch

(In reply to Mark Hammond [:markh] from comment #22)
> Comment on attachment 8674649 [details] [diff] [review]
> 
> ::: addon-sdk/source/test/addons/places/lib/places-helper.js
> @@ +25,5 @@
> >    save, search,
> >    MENU, TOOLBAR, UNSORTED
> >  } = require('sdk/places/bookmarks');
> >  
> > +XPCOMUtils.defineLazyModuleGetter(LoopUI, "PlacesUtils",
> 
> LoopUI doesn't seem right here - I'm surprised it works at all?
> 

Whoops, you're right, that's a copy pasta error. Meant to replace `LoopUI` with `this`, is that correct?


(In reply to Marco Bonardo [::mak] from comment #24)
> Comment on attachment 8674649 [details] [diff] [review]
> Use defineLazyModuleGetter and rework promises order
> 
> please address Mark's feedback first.


I've adjusted the changes to reflect Mark's feedback :)

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8676469 [details] [diff] [review]
changeset_adjustments.patch

Review of attachment 8676469 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please make this fix and push a try run? Then upload a new patch and request feedback on the patch from :mak.

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.clear().then(done, err => Cu.reportError);

This will call |done| twice - I think the |once(...)| call should be removed.

Comment 27

[Andrew Krawchyk]

Attachment: removed call to once

(In reply to Mark Hammond [:markh] from comment #26)
> Comment on attachment 8676469 [details] [diff] [review]
> changeset_adjustments.patch
> 
> Review of attachment 8676469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please make this fix and push a try run? Then upload a new patch and
> request feedback on the patch from :mak.

I uploaded the updated patch. I can't push to the try server though, I've filed a bug to request access though https://bugzilla.mozilla.org/show_bug.cgi?id=1216841

Comment 28

[Andrew Krawchyk]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e68d4d78335a

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8676593 [details] [diff] [review]
removed call to once

Review of attachment 8676593 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, it looks mostly good.

r=me with the following fixed, and a green-er Try run.

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +26,5 @@
>    MENU, TOOLBAR, UNSORTED
>  } = require('sdk/places/bookmarks');
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +  "resource://gre/modules/PlacesUtils.jsm");

For this to work you must also Cu.import XPCOMUtils. this should solve the JP failures you see on the Try run

@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.clear().then(done, err => Cu.reportError);

I'd rather do
PlacesUtils.history.clear().catch(Cu.reportError).then(done);
so we call done in any case.

::: browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js
@@ +1287,5 @@
>  
>    testObserver = new UrlDeletionTester();
>    RemoteDirectoryLinksProvider.addObserver(testObserver);
> +  // remove all history
> +  yield PlacesTestUtils.clearHistory();

nit: the comment is likely pointless now, the API speaks for itself

::: browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ +1689,5 @@
>  
>    testObserver = new UrlDeletionTester();
>    DirectoryLinksProvider.addObserver(testObserver);
> +  // remove all history
> +  yield PlacesTestUtils.clearHistory();

nit: ditto

Comment 30

[Andrew Krawchyk]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e67ec075076

Comment 31

[Andrew Krawchyk]

Attachment: import XPCOMUtils and cleanup comments

(In reply to Marco Bonardo [::mak] from comment #29)
> Comment on attachment 8676593 [details] [diff] [review]
> removed call to once
> 
> Review of attachment 8676593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you, it looks mostly good.
> 
> r=me with the following fixed, and a green-er Try run.


Great! I've made the requested updates and have pushed another try run. I wasn't sure about the `r=me` part, so I added a review flag to you for the updated patch. Should I have committed with the `r=` in the message?

Also, is there a resource/documentation for reading Treeherder output? I'm struggling to follow the test results in the output... there's a lot going on there.

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Andrew Krawchyk from comment #31)
> Great! I've made the requested updates and have pushed another try run. I
> wasn't sure about the `r=me` part, so I added a review flag to you for the
> updated patch. Should I have committed with the `r=` in the message?

having a commit message is mandatory, and usually you append . r=reviewer to it
in this case the commit message could be:

Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak

> Also, is there a resource/documentation for reading Treeherder output? I'm
> struggling to follow the test results in the output... there's a lot going
> on there.

I honestly don't know, it mostly comes from experience with it. Basically the orange/red are what you care about, clicking on them you get a failure summary that usually points out the failure reason.
Some reasons may be recognized as intermittent failures (we have some) and a bug is suggested, in that case it's likely not your failure. Other cases could be network problems or similar.
Until you are not actively contributing from some time and you get experienced with it, you can just ask your reviewer to look at the results and eventually comment on them.
From the ui you can also retrigger a failed test, if it succeeds the next run it's likely an intermittent.
Due to the many intermittent failures it can happen that something is missed and the patch breaks the tree, but generally the system works, and in the worst case the patch gets backed out and needs some more work.

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8678342 [details] [diff] [review]
import XPCOMUtils and cleanup comments

Review of attachment 8678342 [details] [diff] [review]:
-----------------------------------------------------------------

Are you sure this is the right patch? It looks like a backout patch...

Comment 34

[Andrew Krawchyk]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f93e4dc87476

Comment 35

[Andrew Krawchyk]

Attachment: MozReview Request: Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak

Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak

Comment 36

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8680144 [details]
MozReview Request: Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak

https://reviewboard.mozilla.org/r/23575/#review21185

yep, this looks perfect, thank you!
please set the checkin-needed keyword in the keywords field of this bug, so it will be pushed to the fx-team tree.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5fc57433f68a

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5fc57433f68a

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5fc57433f68a

Comment 40

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion