1428434 - Use Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Status: RESOLVED FIXED

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

Description

[Jan Henning [:JanH]]

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

[Justin Clark]

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

[Gregory]

:) 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.

Comment 3

[Jan Henning [:JanH]]

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

[Gregory]

(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

[Gregory]

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

Comment 6

[Justin Clark]

Thanks for the feedback, Jan. I will just wait until the bug is assigned.

Comment 7

[Jan Henning [:JanH]]

@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

[Gregory]

@Jan: Okay! TY

Comment 9

[Justin Clark]

Thanks, will do!

Comment 10

[Justin Clark]

(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

[Gregory]

Ok. Takin it :)

Comment 12

[Jan Henning [:JanH]]

Sure, go ahead.

Comment 13

[cmaheshster]

is this done? or can i work on it?

Comment 14

[Jan Henning [:JanH]]

You can give it a try if you want (see comment 3 for some instructions).

Comment 15

[Issei Horie [:is2ei]]

Hi, I'm Issei.
Could I work on this issue if it's still open?

Comment 16

[constfilin]

Hi all. I already have a patch for the issue, will submit it to MozReview in an hour or so.

Comment 17

[constfilin]

@Jan: can you please assign the issue to me?

Comment 18

[constfilin]

Attachment: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Comment 19

[constfilin]

@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 [e-mail] to [e-mail] , 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.

Comment 20

[constfilin]

Attachment: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Comment 21

[constfilin]

Attachment: Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Review commit: https://reviewboard.mozilla.org/r/241090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241090/

Comment 22

[constfilin]

Attachment: Bug 1428434: making sure that indentation uses spaces instead of tabs

Review commit: https://reviewboard.mozilla.org/r/241092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241092/

Comment 23

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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

Comment 24

[constfilin]

Attachment: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

MozReview-Commit-ID: JF3Wsx9IYfs

Comment 25

[constfilin]

Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241090/diff/1-2/

Comment 26

[constfilin]

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!!

Comment 27

[Jan Henning [:JanH]]

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.

Comment 28

[Jan Henning [:JanH]]

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 29

[constfilin]

Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241090/diff/2-3/

Comment 30

[constfilin]

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

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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 :)

Comment 32

[Jan Henning [:JanH]]

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).

Comment 33

[constfilin]

Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241090/diff/3-4/

Comment 34

[constfilin]

Attachment: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

MozReview-Commit-ID: JF3Wsx9IYfs

Comment 35

[constfilin]

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!

Comment 36

[Jan Henning [:JanH]]

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.

Comment 37

[constfilin]

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.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5805bf8899b9
Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs r=JanH

Comment 39

[Jan Henning [:JanH]]

(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

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/5805bf8899b9

Comment 41

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

Comment on attachment 8972504 [details]
Bug 1428434: Using Framework methods instead of DBUtils for concatenateWhere/appendSelectionArgs

Thanks for driving this across, JanH!