944890 - B2G SMS & MMS: remove convertTimeToInt(...) which is no longer needed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Gene Lian (I already quit Mozilla)]

At bug 939302, we interpret all the timestamps as long-long numbers instead of Date objects. Eventually, all the timestamp values in the DOM, IPDL, SMS/MMS internal codes can just be passed around as long-long numbers. We should be able to remove convertTimeToInt(...) which is no longer needed anymore.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why is this security-sensitive?

Comment 2

[Gene Lian (I already quit Mozilla)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Why is this security-sensitive?

It shouldn't be. Sorry I cloned that bug without unchecking that flag but I'm still not able to uncheck that option now. Could anyone please help me? Thanks!

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, implementation

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, test cases

Comment 5

[Gene Lian (I already quit Mozilla)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=48f15460849c

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393383 [details] [diff] [review]
Part 1, implementation

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

Sorry, can't remember which bug, but I remember I was once told not to align argument names.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393384 [details] [diff] [review]
Part 2, test cases

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

::: dom/mobilemessage/tests/test_smsservice_createsmsmessage.js
@@ -145,5 @@
>  
>  /**
> - * Test supplying the timestamp as a Date object.
> - */
> -add_test(function test_timestamp_date() {

Please rewrite this as what we have in test_invalid_timestamp_float.

@@ -168,5 @@
> -
> -/**
> - * Test that a floating point number for the timestamp is not allowed.
> - */
> -add_test(function test_invalid_timestamp_float() {

Please keep this and replace |new Date()| with |Date.now()|.  We still want to verify acceptable argument types in this script.

Comment 8

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> > -add_test(function test_invalid_timestamp_float() {
> 
> Please keep this and replace |new Date()| with |Date.now()|.  We still want
> to verify acceptable argument types in this script.

Since the float/null/undefined will be automatically casted to valid unsigned long long value, I'd prefer removing these tests.

Comment 9

[Gene Lian (I already quit Mozilla)]

> Since the float/null/undefined will be automatically casted to valid
> unsigned long long value, I'd prefer removing these tests.

Random object like {} will also implicitly be casted to integer.

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, implementation, V2

Hi Vicamo, note that I'd still prefer align the parameters for IDL and IPDL because they are just for generating codes. Also, I saw lots of examples in WebIDL/IDL aligning parameters in this way so I think it's OK to do so.

Comment 11

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, test cases, V2

https://tbpl.mozilla.org/?tree=Try&rev=e5a35e95c210

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393423 [details] [diff] [review]
Part 1, implementation, V2

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

Thank you :)

Comment 13

[Gene Lian (I already quit Mozilla)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/9023afb674fa
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/9b44086c1d1c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9023afb674fa
https://hg.mozilla.org/mozilla-central/rev/9b44086c1d1c