1219323 - Remove unnecessary table created check from BrowserDatabaseHelper

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Vivek Balakrishnan[:vivek]]

This is a follow up bug to 1025128 to remove the redundant boolean flag "didCreateTabsTable" from BrowserDatabaseHelper.java, so that the control flow is simple.[1] 

Also, fix the nits mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1025128#c7.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1025128#c9

Comment 1

[Vishnu (:malayaleecoder)]

Hello Vivek,

I am back :)
As far as I understand, for this bug, there is no necessity for the 'if' condition as mentioned in c9, so should be removed, and there are a few nit fixes. Please correct me if I am wrong.

Thanks.

Comment 2

[Vivek Balakrishnan[:vivek]]

Hi malayaleecoder,

Thanks for picking this up
Yes, you are right. so please fix the nits and remove the table creation boolean check.

Comment 3

[Vishnu (:malayaleecoder)]

Attachment: Bug1219323_v1.diff

Hello Vivek,

Have attached an initial patch. Please review and let me know any further changes to be made.

Thank You.

Comment 4

[Vishnu (:malayaleecoder)]

Attachment: Bug1219323_v1.diff

Please ignore the previous attachment as it is an older version.

Thank You.

Comment 5

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8719008 [details] [diff] [review]
Bug1219323_v1.diff

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

Hi malayaleecoder,

Thanks for the patch. 
LGTM. However, can you please update your patch to remove didCreateCurrentReadingListTable check [1].
That check is also redundant.


[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#408

Comment 6

[Vishnu (:malayaleecoder)]

Attachment: Bug1219323_v2.diff

Hello Vivek,

I have made the said changes and uploaded a new patch. Please have a look.

Comment 7

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8719551 [details] [diff] [review]
Bug1219323_v2.diff

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

Thanks for the quick turnaround malayaleecoder.
LGTM

@Nick: can you please do a super review?

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8719551 [details] [diff] [review]
Bug1219323_v2.diff

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

lgtm.  Vivek, can you chart the next steps for malayaleecoder?

Comment 9

[Vishnu (:malayaleecoder)]

Nick, I guess Vivek has not been online for almost a week. I think you can tell me what to do next :)

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to malayaleecoder from comment #9)
> Nick, I guess Vivek has not been online for almost a week. I think you can
> tell me what to do next :)

Well, that's only sort of how this works.  You've shown your way around a few Fennec tickets now.  What kinds of things are you interested in?  I have some interesting Firefox Accounts and Sync tickets coming up.  Bug 1241627 is a nice small one -- perhaps not trivial to test, but high value for its size.  Much larger is Bug 1250782.  That's a non-trivial ticket, but I can break it down and work with you.  It can happen in stages (it has to -- I'm building Push for it as we speak!)

Comment 11

[Carsten Book [:Tomcat]]

has problems to apply:

Tomcats-MacBook-Pro-2:fx-team Tomcat$ hg qimport bz://1219323 && hg qpush && hg qrefresh -e
Fetching... done
Parsing... done
adding 1219323 to series file
renamed 1219323 -> Bug1219323_v2.diff
applying Bug1219323_v2.diff
patching file mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
Hunk #2 FAILED at 333
1 out of 8 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug1219323_v2.diff

Comment 12

[Vishnu (:malayaleecoder)]

Attachment: Bug1219323_v3.diff

Necessary changes have been made. Please check.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/89460f01ea5a

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to malayaleecoder from comment #12)
> Created attachment 8725370 [details] [diff] [review]
> Bug1219323_v3.diff
> 
> Necessary changes have been made. Please check.

Landed.  Thanks, mayaleecoder, Carsten.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89460f01ea5a