Remove unnecessary table created check from BrowserDatabaseHelper

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
Data Providers
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: vivek, Assigned: malayaleecoder)

Tracking

unspecified
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [lang=java])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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.
Flags: needinfo?(vivekb.balakrishnan)
(Reporter)

Comment 2

2 years ago
Hi malayaleecoder,

Thanks for picking this up
Yes, you are right. so please fix the nits and remove the table creation boolean check.
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 3

2 years ago
Created attachment 8718998 [details] [diff] [review]
Bug1219323_v1.diff

Hello Vivek,

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

Thank You.
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8718998 - Flags: review?(vivekb.balakrishnan)
(Assignee)

Comment 4

2 years ago
Created attachment 8719008 [details] [diff] [review]
Bug1219323_v1.diff

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

Thank You.
Attachment #8718998 - Attachment is obsolete: true
Attachment #8718998 - Flags: review?(vivekb.balakrishnan)
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8719008 - Flags: review?(vivekb.balakrishnan)
(Assignee)

Updated

2 years ago
Flags: needinfo?(vivekb.balakrishnan)
(Reporter)

Comment 5

2 years ago
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
Attachment #8719008 - Flags: review?(vivekb.balakrishnan) → feedback+
(Reporter)

Updated

2 years ago
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 6

2 years ago
Created attachment 8719551 [details] [diff] [review]
Bug1219323_v2.diff

Hello Vivek,

I have made the said changes and uploaded a new patch. Please have a look.
Attachment #8719008 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8719551 - Flags: review?(vivekb.balakrishnan)
(Reporter)

Comment 7

2 years ago
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?
Attachment #8719551 - Flags: review?(vivekb.balakrishnan)
Attachment #8719551 - Flags: review?(nalexander)
Attachment #8719551 - Flags: review+
(Reporter)

Updated

2 years ago
Flags: needinfo?(vivekb.balakrishnan)
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?
Attachment #8719551 - Flags: review?(nalexander) → review+
(Assignee)

Updated

2 years ago
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 9

2 years ago
Nick, I guess Vivek has not been online for almost a week. I think you can tell me what to do next :)
Flags: needinfo?(nalexander)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(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!)
Flags: needinfo?(nalexander)
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
Flags: needinfo?(malayaleecoder)
(Assignee)

Comment 12

2 years ago
Created attachment 8725370 [details] [diff] [review]
Bug1219323_v3.diff

Necessary changes have been made. Please check.
Attachment #8719551 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(malayaleecoder)
Flags: needinfo?(cbook)

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/89460f01ea5a
Keywords: checkin-needed
(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.

Updated

2 years ago
Flags: needinfo?(cbook)

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89460f01ea5a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

2 years ago
Blocks: 1261907
You need to log in before you can comment on or make changes to this bug.