Use Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

RESOLVED FIXED in Firefox 62

Status

()

RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: JanH, Assigned: constfilin, Mentored)

Tracking

Trunk
Firefox 62
All
Android
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
Since our minimum SDK level is now >= 11, we can drop our copies of those two methods from DBUtils [1] and just directly use the corresponding Android framework methods in DatabaseUtils.



[1] https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/mobile/android/base/java/org/mozilla/gecko/db/DBUtils.java#36-66

Comment 1

a year ago
I'm looking to make my first contribution here. To make sure I understand the problem, we just need to delete the two methods as linked in [1]?

Thank you and apologies for a [probably] silly question.

Comment 2

a year ago
:) got here 32 mins late.
I guess task is to completely replace their own copies with ones from SDK which means that we need 
-to replace all the appearences in sourses with SDKs ones and 
-to delete those two local methods

Gluck Justin
just in case Justin wont take the task - ill do it.
(Reporter)

Comment 3

a year ago
Exactly, first find all places that use those two methods using https://dxr.mozilla.org/, https://searchfox.org/ or Android Studio's code search, swap them over to the framework methods and then delete our own copies of those functions (and make sure Firefox still builds after all that).

To get you started, just follow the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

For submitting the patch later on, instructions are here:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html
Just scroll down and chose the applicable link (Mercurial or Git).

Comment 4

a year ago
(In reply to Jan Henning [:JanH] from comment #3)
> Exactly, first find all places that use those two methods using
> https://dxr.mozilla.org/, https://searchfox.org/ or Android Studio's code
> search, swap them over to the framework methods and then delete our own
> copies of those functions (and make sure Firefox still builds after all
> that).
> 
> To get you started, just follow the instructions here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_for_Android_build
> 
> For submitting the patch later on, instructions are here:
> http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> install.html
> Just scroll down and chose the applicable link (Mercurial or Git).

Jan, should i do it too, or should i left it to Justin? (coz he was first and it can be kinda impolite i guess)

Comment 5

a year ago
:Jan I'm looking to make my first contribution too btw ^^

Comment 6

a year ago
Thanks for the feedback, Jan. I will just wait until the bug is assigned.
(Reporter)

Comment 7

a year ago
@Justin: Feel free to go ahead and get a build of Firefox going. We normally sort out the assignment when you've got a patch.

@Gregory: Hmm, if you're looking specifically for Firefox on Android, it seems we're a little short on good first bugs marked as such. Maybe try bug 1411198 or bug 1286698 and ping Nevin Chen, or else browser around https://www.joshmatthews.net/bugsahoy/?internals-android=1&mobileandroid=1 and try finding something that looks interesting.

Comment 8

a year ago
@Jan: Okay! TY

Comment 9

a year ago
Thanks, will do!

Comment 10

a year ago
(In reply to Jan Henning [:JanH] from comment #7)
> @Justin: Feel free to go ahead and get a build of Firefox going. We normally
> sort out the assignment when you've got a patch.
> 

@Jan, It looks like I am having hardware incompatibilities. I am sorry for the inconvenience. I can hand this off to @Gregory and he'll hopefully fix it more quickly. Thanks for being a mentor and love the community.

Comment 11

a year ago
Ok. Takin it :)
Flags: needinfo?(jh+bugzilla)
(Reporter)

Comment 12

a year ago
Sure, go ahead.
Flags: needinfo?(jh+bugzilla)

Comment 13

a year ago
is this done? or can i work on it?
(Reporter)

Comment 14

a year ago
You can give it a try if you want (see comment 3 for some instructions).
Hi, I'm Issei.
Could I work on this issue if it's still open?
(Assignee)

Comment 16

11 months ago
Hi all. I already have a patch for the issue, will submit it to MozReview in an hour or so.
(Assignee)

Comment 17

11 months ago
@Jan: can you please assign the issue to me?
(Assignee)

Comment 19

11 months ago
@Jan: I am trying to push patch to MozReview and getting this error:

====
cf@cfdell:~/Work/Mozilla/android/mozilla-central$ hg bzexport
Refreshing configuration cache for https://bugzilla.mozilla.org/bzapi/
. uploaded as https://bugzilla.mozilla.org/attachment.cgi?id=8972480&action=edit
abort: REST error on PUT to https://bugzilla.mozilla.org/rest/bug/1428434: You tried to change the Assignee field from nobody@mozilla.org to constfilin@gmail.com , but only the assignee of the bug, or a user with the required permissions may change that field. If you are attempting to confirm an unconfirmed bug or edit the fields of a bug, find out how to get the necessary permissions.
cf@cfdell:~/Work/Mozilla/android/mozilla-central$
====

Sounds like I can't proceed until the issue is assigned to me.
Hope to hear from your soon.

Updated

11 months ago
Assignee: nobody → constfilin
Status: NEW → ASSIGNED
(Assignee)

Updated

11 months ago
Attachment #8972480 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8972504 - Flags: review?(jh+bugzilla)
Attachment #8972505 - Flags: review?(jh+bugzilla)
(Assignee)

Updated

11 months ago
Attachment #8972504 - Flags: review?(nalexander)

Comment 23

11 months ago
mozreview-review
Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

https://reviewboard.mozilla.org/r/241090/#review247022

Hi Constantine!

Thanks for picking this up.  Could you fold the two patches here into one using |hg histedit|, and push again to MozReview?  We won't land in two pieces like this.  I looked briefly at the details and everything looks okay; I'll leave JanH to check more closely.

r- only for us to look at this again after it's been folded down.

Thanks!
Nick
Attachment #8972504 - Flags: review?(nalexander) → review-
(Assignee)

Comment 24

11 months ago
MozReview-Commit-ID: JF3Wsx9IYfs
(Assignee)

Updated

11 months ago
Attachment #8972503 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8972505 - Attachment is obsolete: true
Attachment #8972505 - Flags: review?(jh+bugzilla)
(Assignee)

Comment 26

11 months ago
hi Nick. Thanks for taking a look.

I just rolled my tab-removal commit into the previous commit which had the real changes. I hope I did everything right wigh "hg histedit" and now https://reviewboard.mozilla.org/r/241090/ shows me just one commit. I a bit concerned that it also shows me that "Some jobs failed on try" in Treeherder (https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ccc66a00fef26211a9e535de454a43b279845b). I can't pretend that I understand what it means but judging by the timestamp of the failed jobs, they were before my folding and therefore not caused by the fold. 

Hope this changeset is now ready for review. Thank you!!
(Reporter)

Comment 27

11 months ago
mozreview-review
Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

https://reviewboard.mozilla.org/r/241090/#review247066

::: mobile/android/base/java/org/mozilla/gecko/db/HomeProvider.java:87
(Diff revision 2)
>              throw new IllegalArgumentException("All queries should contain a dataset ID parameter");
>          }
>  
> -        selection = DBUtils.concatenateWhere(selection, HomeItems.DATASET_ID + " = ?");
> -        selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +        selection = DatabaseUtils.concatenateWhere(selection, HomeItems.DATASET_ID + " = ?");
> +        selectionArgs = DatabaseUtils.appendSelectionArgs(selectionArgs,
> -                                                    new String[] { datasetId });
> +                                                          new String[] { datasetId });

So adjusting the indentation somewhere like here, where we were already aligned with the line above, is fine...

::: mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java:239
(Diff revision 2)
>          switch (match) {
>              case CLIENTS_ID:
>                  trace("Update on CLIENTS_ID: " + uri);
> -                selection = DBUtils.concatenateWhere(selection, selectColumn(TABLE_CLIENTS, Clients._ID));
> -                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                selection = DatabaseUtils.concatenateWhere(selection, selectColumn(TABLE_CLIENTS, Clients._ID));
> +                selectionArgs = DatabaseUtils.appendSelectionArgs(selectionArgs,
> -                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                                                                  new String[] { Long.toString(ContentUris.parseId(uri)) });

... but for cases like this I'd either just keep the previous indentation in order to keep the line length down, or else move the part after the "=" to a separate line.
(Reporter)

Comment 28

11 months ago
Other than the possibly-slightly-too-long-lines after the indentation changes this looks fine.

I'm not sure why the first Try run failed, either, but I've pushed your patch again.
Comment hidden (mozreview-request)
(Assignee)

Comment 30

11 months ago
Thanks, Jan.

I just made another commit shortening the long lines you pointed out to (see https://reviewboard.mozilla.org/r/241088/diff/2-3/)
If there is anything else I need to do to get this merged, then please let me know. Otherwise we can wait till Nick takes another look at it or - perhaps - we can already change the status of this bug to RESOLVED???

Comment 31

11 months ago
mozreview-review
Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

https://reviewboard.mozilla.org/r/241090/#review247132

The whitespace still looks wonky here, but I'll let JanH take over and get this across the line.

As for setting RESOLVED, that happens only after the patch lands in our integration branches.  JanH will set checkin-needed, and then you can see the magic happen :)
Attachment #8972504 - Flags: review?(nalexander)
(Reporter)

Comment 32

11 months ago
mozreview-review
Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

https://reviewboard.mozilla.org/r/241090/#review247316

Hmm, as Nick said, the whitespace is still somwhat over the place. Thinking some more, I'd really just adjust the indentation of following lines where there's already some pre-existing manual indentation, i.e. in the cases I've marked below.

Everywhere else, I'd just simply swap DBUtils for DatabaseUtils and not touch anything else, since the indentation of the line below is already the default indentation you'd get if you were using Android Studio.

One nice way to make dropping all those unneeded whitespace changes easier would be using the hg evolve extension:
1. Install it as per https://www.mercurial-scm.org/wiki/EvolveExtension#Setup
2. Run "hg uncommit --all" to make your current changeset editable again
3. Run "hg amend -i" and uncheck all whitespace changes that you want to drop again, then commit all the other changes that you want to keep.
4. Throw away the remaining unnecessary whitespace changes that you haven't commited via "hg revert -aC".
5. (If necessary, make any remaining additional changes and simply amend the current changeset - the evolve extension introduces "hg amend" as a shorthand for "hg commit --amend".

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1068
(Diff revision 3)
>                                           " ? AS " + Bookmarks._ID + "," +
>                                           " ? AS " + Bookmarks.URL + "," +
>                                           " ? AS " + Bookmarks.TITLE);
>  
> -            suggestedSiteArgs = DBUtils.appendSelectionArgs(suggestedSiteArgs,
> +            suggestedSiteArgs = DatabaseUtils.appendSelectionArgs(suggestedSiteArgs,
>                                                              new String[] {

Maintain the pre-existing indentation here... (don't forget the following lines directly below)

::: mobile/android/base/java/org/mozilla/gecko/db/HomeProvider.java:88
(Diff revision 3)
>          }
>  
> -        selection = DBUtils.concatenateWhere(selection, HomeItems.DATASET_ID + " = ?");
> -        selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +        selection = DatabaseUtils.concatenateWhere(selection, HomeItems.DATASET_ID + " = ?");
> +        selectionArgs = DatabaseUtils.appendSelectionArgs(
> +                            selectionArgs,
> -                                                    new String[] { datasetId });
> +                            new String[] { datasetId });

...here (you can keep `selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,` as one line)

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:667
(Diff revision 3)
>  
>              // Only create a filter query with a maximum of 10 constraint words.
>              final int constraintCount = Math.min(constraintWords.length, 10);
>              for (int i = 0; i < constraintCount; i++) {
> -                selection = DBUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " +
> +                selection = DatabaseUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " +
>                                                                        Combined.TITLE + " LIKE ?)");

... here

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:670
(Diff revision 3)
>              for (int i = 0; i < constraintCount; i++) {
> -                selection = DBUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " +
> +                selection = DatabaseUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " +
>                                                                        Combined.TITLE + " LIKE ?)");
>                  String constraintWord =  "%" + constraintWords[i] + "%";
> -                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                selectionArgs = DatabaseUtils.appendSelectionArgs(selectionArgs,
> -                                                            new String[] { constraintWord, constraintWord });
> +                                                                  new String[] { constraintWord, constraintWord });

... and here (but here you've changed it already, so it's fine).
Attachment #8972504 - Flags: review?(jh+bugzilla)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

11 months ago
MozReview-Commit-ID: JF3Wsx9IYfs
(Assignee)

Updated

11 months ago
Attachment #8972620 - Attachment is obsolete: true
(Assignee)

Comment 35

11 months ago
Jan and Nick: thanks for your feedback..

I cleaned up all my changes creating a diff where I just do s/DBUtils\./DatabaseUtils\./g except in the places
where there was pre-existing custom/manual indentation which I tried to preserve. 

Please do one more review at https://reviewboard.mozilla.org/r/241088/ and let me know if there is anything else I need to do to bring this across the finish line.

Thanks!
(Reporter)

Comment 36

11 months ago
mozreview-review
Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

https://reviewboard.mozilla.org/r/241090/#review247586

Looks fine - thanks for taking this bug, even if it's not the most glamarous work.

While this patch probably isn't particularly risky, it's not really urgent either and since there's a soft code freeze in place until the merge for the next Firefox version next Monday, I suggest waiting until then.
So after the weekend you can edit the bug and set the "checkin-needed" keyword to get your changes into our repository.
Attachment #8972504 - Flags: review?(jh+bugzilla) → review+
(Assignee)

Comment 37

11 months ago
Thanks, Jan. Ok, I set "checkin-needed" keyword on the bug. Please let me know if there are any other steps you would like me to take to resolve this issue.
Keywords: checkin-needed

Comment 38

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5805bf8899b9
Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs r=JanH
Keywords: checkin-needed
(Reporter)

Comment 39

11 months ago
(In reply to constfilin from comment #37)
> Thanks, Jan. Ok, I set "checkin-needed" keyword on the bug. Please let me
> know if there are any other steps you would like me to take to resolve this
> issue.

That's all. If your patch (series) has a r+ and no open issues showing in Mozreview, one of the code sherrifs will come along and land your patch. In this case RyanVM has already done that.
After the patch has landed in one of the two integration repositories (autoland, or mozilla-inbound for manually pushed patches), automated tests are run. If no unexpected issues crop up, your patch will eventually be merged to mozilla-central (at which point it will start being included in our Nightly builds) and automatically marked as resolved.

So if you want, you can try finding some other bug to work on.

Comment 40

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5805bf8899b9
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Thanks for driving this across, JanH!
Attachment #8972504 - Flags: review?(nalexander)
You need to log in before you can comment on or make changes to this bug.