807138 - Move Messages App OOP

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect, P1)

Description

[Gregor Wagner [:gwagner]]

Once bug 775997 is resolved we can move the Messages app OOP.

We should probably get some more testing for this before we push this to all the unagis. It works fine for me but....

Tony can you help here? Basically we have to test the patch  https://bug775997.bugzilla.mozilla.org/attachment.cgi?id=676790

in combination with moving the Messages app OOP. I will post a patch soon that moves the messages app OOP.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: Gaia patch

Comment 2

[Dave Hylands [:dhylands]]

Shouldn't your patch also remove it from the black list in apps/system/js/background_service.js

Comment 3

[Mike Connor [:mconnor]]

Cert requirement -> P1.

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Dave Hylands [:dhylands] from comment #2)
> Shouldn't your patch also remove it from the black list in
> apps/system/js/background_service.js

Thx for checking. My git diff didn't do what I wanted.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 677464 [details] [diff] [review]
patch

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

I wasn't intended to do a review, but since I was looking at the differences, I figured I'd throw in my comments.

::: apps/system/js/window_manager.js
@@ +987,1 @@
>        'Cost Control',

Do you need to remove the trailing comma here? (I'm not familiar enough with JS to know for sure)

@@ +987,2 @@
>        'Cost Control',
>        // Cross-process SMS (bug 775997)

Presumably, this comment should also be removed

Comment 7

[Dave Hylands [:dhylands]]

(In reply to Dave Hylands [:dhylands] from comment #6)

Replying to myself - obviously I haven't had enough coffee yet :)

> ::: apps/system/js/window_manager.js
> @@ +987,1 @@
> >        'Cost Control',
> 
> Do you need to remove the trailing comma here? (I'm not familiar enough with
> JS to know for sure)
> 
> @@ +987,2 @@
> >        'Cost Control',
> >        // Cross-process SMS (bug 775997)
> 
> Presumably, this comment should also be removed

Nevermind, I see that the comment goes with Cost Control - Doh.

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Dave Hylands [:dhylands] from comment #6)
> Comment on attachment 677464 [details] [diff] [review]
> patch
> 
> Review of attachment 677464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wasn't intended to do a review, but since I was looking at the
> differences, I figured I'd throw in my comments.
> 
> ::: apps/system/js/window_manager.js
> @@ +987,1 @@
> >        'Cost Control',
> 
> Do you need to remove the trailing comma here? (I'm not familiar enough with
> JS to know for sure)
> 

We don't need it bug I learned from the webdev team that they usually add the trailing comma so that the revision history per line makes more sense.

Comment 9

[Andreas Gal :gal]

Comment on attachment 677464 [details] [diff] [review]
patch

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

::: apps/system/js/window_manager.js
@@ +987,1 @@
>        'Cost Control',

A trailing comma will cause a void value to be appended to the array (also called a hole in JS lingo). So length will be +1. As long you are fine with that, its ok.

Comment 10

[Andreas Gal :gal]

Actually I lied:

js> [1,].length
1
js> [1].length
1

Comment 11

[Gregor Wagner [:gwagner]]

Comment on attachment 677464 [details] [diff] [review]
patch

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 677464 [details] [diff] [review]
patch

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

No need for approval this is a blocking-basecamp+ bug.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 677464 [details] [diff] [review]
patch

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

Can you delete the background service blacklist. Communications does not use it anymore. R+ with that.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/c08e6bed1ffd06d152f5d7a63aadb99fc4d7b00a