1118971 - Extract fragile parameter appending code into DBUtils

Status: RESOLVED INCOMPLETE

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

Description

[Richard Newman [:rnewman]]

There are some methods that we share (or can share) across our DB implementations, like appending PARAM_SHOW_DELETED, PARAM_LIMIT, or PARAM_PROFILE.

These are currently spread across LocalBrowserDB, TabsAccessor, URLMetadata, and so on. It'd be nice to clean these up.

Comment 1

[M Hussain]

are we suppose to move these below methods to DBUtils.java ?

combinedUriWithLimit(int limit)
bookmarksUriWithLimit(int limit) 
removeBookmarksWithURL(ContentResolver cr, String uri)
removeReadingListItemWithURL(ContentResolver cr, String uri)

Comment 2

[Richard Newman [:rnewman]]

Not quite.

There's shared logic in *WithLimit, but those operate on private fields of LocalBrowserDB.

There's shared logic in methods that append PARAM_SHOW_DELETED.

This bug is to lift the shared logic into shared methods in DBUtils.

Comment 3

[MunKeat]

If no one is working on it, may I be assigned this bug?

Comment 4

[MunKeat]

Attachment: Bug1118971.patch

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8557582 [details] [diff] [review]
Bug1118971.patch

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

Sorry for the delayed response!

This is the right direction, but needs some tweaking and terminology clarification. It's the Uri being updated, not the profile; the "appendProfile" methods are called such because they're appending a profile parameter to the provided URI.

::: mobile/android/base/db/DBUtils.java
@@ +176,5 @@
> +
> +    /**
> +     * Note that Uri uri is not set with the modifier final for 
> +     * method modifyProfile, modifyProfileWithLimit, and modifyProfileWithInsert, 
> +     * unlike method appendProfile, and appendProfileWithDefault.

Why not? I don't see any reason why these couldn't be final.

@@ +179,5 @@
> +     * method modifyProfile, modifyProfileWithLimit, and modifyProfileWithInsert, 
> +     * unlike method appendProfile, and appendProfileWithDefault.
> +     */
> +    public static Uri modifyProfile(final String profile, Uri uri, int parameter){
> +        return uri.buildUpon().appendQueryParameter(parameter, profile).build();

I don't think "profile" is the right word here. You're not appending a profile, you're appending some value.

I think you want something like:

  public static Uri appendIntegerParameter(final Uri uri, final String param, final int value) {
      return uri.buildUpon()
                .appendQueryParameter(param, "" + value)
                .build();
  }

then

  public static Uri appendLimit(final Uri uri, final int limit) {
      return appendIntegerParameter(uri, BrowserContract.PARAM_LIMIT, limit);
  }

etc.

Comment 6

[MunKeat]

I read through the code and was under the initial impression that buildUpon() actually modifies the original Uri input in. But I guess I should have read a bit more slowly, since it states that "buildUpon() Constructs a new builder, copying the attributes from this Uri."

Oops.

Thanks for the advice; I'll make the necessary changes and upload it asap!

Comment 7

[MunKeat]

Attachment: Bug1118971_V2.patch

Comment 8

[MunKeat]

Attachment: Bug1118971_V3.patch

Prefix 'DBUtils.' was accidentally omitted in the previous patch (Bug1118971_V2). This patch contains the rectification necessary.

Please ignore Bug1118971_V2.patch.

Apologies for the mistake.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8570849 [details] [diff] [review]
Bug1118971_V3.patch

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

Nit: always a space between the arg list and the opening {.

::: mobile/android/base/db/DBUtils.java
@@ +169,5 @@
>          builder.append(")");
>          return builder.toString();
>      }
>  
> +    public static Uri appendIntegerParameter(final Uri uri, final int value, final String parameter){

parameter first, then value.

@@ +170,5 @@
>          return builder.toString();
>      }
>  
> +    public static Uri appendIntegerParameter(final Uri uri, final int value, final String parameter){
> +        return uri.buildUpon().appendQueryParameter(value, parameter).build();

You have the arguments the wrong way around.

@@ +177,1 @@
>      public static Uri appendProfile(final String profile, final Uri uri) {

Make the Uri the first argument.

@@ +179,3 @@
>      }
>  
>      public static Uri appendProfileWithDefault(final String profile, final Uri uri) {

Make the Uri the first argument.

@@ +184,5 @@
>          }
>          return appendProfile(profile, uri);
>      }
>  
> +    public static Uri appendLimit(final int limit, final Uri uri){

Make the Uri the first argument.

@@ +188,5 @@
> +    public static Uri appendLimit(final int limit, final Uri uri){
> +        return appendIntegerParameter(uri, BrowserContract.PARAM_LIMIT, String.valueOf(limit));
> +    }
> +
> +    public static Uri appendTrueWithInsert(final Uri uri){

"appendInsertIfNeeded"?

::: mobile/android/base/db/LocalBrowserDB.java
@@ +698,5 @@
>  
>      @Override
>      public void expireHistory(ContentResolver cr, ExpirePriority priority) {
>          Uri url = mHistoryExpireUriWithProfile;
> +        url = DBUtils.appendIntegerParameter(url, BrowserContract.PARAM_EXPIRE_PRIORITY, priority.toString());

This isn't an integer parameter, is it?

@@ +1079,5 @@
>          values.put(Favicons.PAGE_URL, pageUri);
>          values.put(Favicons.DATA, encodedFavicon);
>  
>          // Update or insert
> +        Uri faviconsUri = DBUtils.appendTrueWithInsert(withDeleted(mFaviconsUriWithProfile));

Let's buy a small efficiency here. withDeleted builds a new URI, then we do the same again, which is silly.

    private static Uri appendInsertIfNeededAndWithDeleted(final Uri uri) {
        return uri.buildUpon()
                  .appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true")
                  .appendQueryParameter(BrowserContract.PARAM_SHOW_DELETED, "1")
                  .build();
    }

@@ +1270,5 @@
>                  values.put(History.TITLE, title);
>              }
>              values.put(History.URL, url);
>  
> +            Uri historyUri = DBUtils.appendTrueWithInsert(withDeleted(mHistoryUriWithProfile));

... same

@@ +1326,5 @@
>          }
>          values.put(Bookmarks.PARENT, parent);
>          values.put(Bookmarks.TYPE, type);
>  
> +        Uri bookmarkUri = DBUtils.appendTrueWithInsert(withDeleted(mBookmarksUriWithProfile));

... same

@@ +1373,5 @@
>              values.put(Favicons.URL, faviconUrl);
>          }
>  
>          // Update or insert
> +        Uri faviconsUri = DBUtils.appendTrueWithInsert(withDeleted(mFaviconsUriWithProfile));

... same

Comment 10

[MunKeat]

Attachment: Bug1118971_V4.patch

Comment 11

[Jalpreet Singh Nanda [:imjalpreet]]

Hey! If this bug is not being solved by someone else, I want to take this up and try to fix this.

Comment 12

[Richard Newman [:rnewman]]

Go ahead, Jalpreet! Perhaps MunKeat's old patch from last year can be cleaned up and used as a starting point.

Comment 13

[Akshat Goel]

Attachment: B1118971_v5.patch

Modified MunKeat's patch a little bit but it is essentially similar. Need review on the same.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8737514 [details] [diff] [review]
B1118971_v5.patch

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

The diff looks good to me! There are still a few cases where we call `appendQueryParameter` outside of DBUtils but that's probably okay and this is incrementally better than we had before.

However, the patch does not apply cleanly – Akshat, can you pull the latest changes and rebase your patch? After that and the one nit, I think we should be good to push it to our test servers and land it! :)

::: mobile/android/base/java/org/mozilla/gecko/db/DBUtils.java
@@ +258,5 @@
> +        return appendProfile(uri, profile);
> +    }
> +
> +    public static Uri appendLimit(final Uri uri, final int limit) {
> +        return appendParameterValue(uri, BrowserContract.PARAM_LIMIT, "" + limit);

nit: "" + limit -> String.valueOf(limit)

I think it's slightly more efficient because we don't have to concatenate strings

Comment 15

[Akshat Goel]

Attachment: B1118971_v6.patch

Updated with review comments.

One query.

File : /mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
Line : 484

We are using multi-catch which is not available before API 19 and we are using current min API as 15. Should we fix this?

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Is this patch ready for review? If so, please flag me for review?

(In reply to Akshat Goel from comment #15)
> We are using multi-catch which is not available before API 19 and we are
> using current min API as 15. Should we fix this?

Multi-catch requires the build SDK to be 19+ but it's backwards compatible – see http://stackoverflow.com/a/24574892 for details.

Thanks for being thorough!

Comment 17

[Akshat Goel]

I don't see any option to modify flag for existing attachment. Do I need to upload another patch or can it be reviewed without a flag?

(In reply to Michael Comella (:mcomella) from comment #16)
> Is this patch ready for review? If so, please flag me for review?
> 
> (In reply to Akshat Goel from comment #15)
> > We are using multi-catch which is not available before API 19 and we are
> > using current min API as 15. Should we fix this?
> 
> Multi-catch requires the build SDK to be 19+ but it's backwards compatible –
> see http://stackoverflow.com/a/24574892 for details.
> 
> Thanks for being thorough!

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

(a11y-review flag is used to get attention for the accessibility (a11y) team)

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Akshat Goel from comment #17)
> I don't see any option to modify flag for existing attachment. Do I need to
> upload another patch or can it be reviewed without a flag?

You can click the, "Details" button next to the attachment, and there should be a "review" item in there next to a drop-down. You flip that to "?" and a textbox should appear to type in your reviewer's name. You should be able to auto-complete me with, ":mcomella"

Comment 20

[Akshat Goel]

Thanks! I have marked the attachment for review. I have also marked previous patch as obsolete.

(In reply to Michael Comella (:mcomella) from comment #19)
> (In reply to Akshat Goel from comment #17)
> > I don't see any option to modify flag for existing attachment. Do I need to
> > upload another patch or can it be reviewed without a flag?
> 
> You can click the, "Details" button next to the attachment, and there should
> be a "review" item in there next to a drop-down. You flip that to "?" and a
> textbox should appear to type in your reviewer's name. You should be able to
> auto-complete me with, ":mcomella"

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8747513 [details] [diff] [review]
B1118971_v6.patch

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

This still doesn't appear to apply locally, but I was able to manually merge it and I've pushed it to our test servers:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6658d2a8f16

Akshat, normally we'd use the "checkin-needed" keyword to land this on green push but given that I'm doing a manual merge, I'll have to land it and we can't. Thanks for your help!

NI self to land on green push.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
@@ +1664,2 @@
>          cr.update(uri,
> +                values,

It looks like your editor may have reformatted some lines – we generally don't accept patches with unnecessary reformatting but to save time, I'll accept it this time.

To prevent this in the future, you can run `hg log -fp` to ensure only your intended changes are in your patch or you can check your upload on Bugzilla.

Comment 22

[Akshat Goel]

Thanks Michael. I will keep the points you mentioned in future changes. 

If it is ok, I will like to pick another first bug so that I can work on the complete fix->iterate->checkin cycle. Is it allowed to do so?

(In reply to Michael Comella (:mcomella) from comment #21)
> Comment on attachment 8747513 [details] [diff] [review]
> B1118971_v6.patch
> 
> Review of attachment 8747513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This still doesn't appear to apply locally, but I was able to manually merge
> it and I've pushed it to our test servers:
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6658d2a8f16
> 
> Akshat, normally we'd use the "checkin-needed" keyword to land this on green
> push but given that I'm doing a manual merge, I'll have to land it and we
> can't. Thanks for your help!
> 
> NI self to land on green push.
> 
> ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
> @@ +1664,2 @@
> >          cr.update(uri,
> > +                values,
> 
> It looks like your editor may have reformatted some lines – we generally
> don't accept patches with unnecessary reformatting but to save time, I'll
> accept it this time.
> 
> To prevent this in the future, you can run `hg log -fp` to ensure only your
> intended changes are in your patch or you can check your upload on Bugzilla.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Akhshat, it actually looks like we have some test failures in the push from comment 21 – can you take a look?

(In reply to Akshat Goel from comment #22)
> If it is ok, I will like to pick another first bug so that I can work on the
> complete fix->iterate->checkin cycle. Is it allowed to do so?

Sure. If you think it'd help to do this before you fix the issues here as well, feel free to do so.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning due to inactivity. Let me know if you'd still like to work on this Akshat.

Comment 25

[mddrill]

Can I work on this bug? I've never worked on a bug before so I might need someone to show me where to start.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey mddrill – welcome to Bugzilla!

(In reply to mddrill from comment #25)
> Can I work on this bug? I've never worked on a bug before so I might need
> someone to show me where to start.

Sure! We'll assign you to the bug once you post a patch.

As for where to start, set up a build environment - you can see the instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Once your build is set up, the most recent contributor got a mostly working patch that had test failures – I'd look at that patch to get an idea of the changes we're looking at and rebase it onto the latest tree. Then make sure no new code was added that might affect the patch and then post it to the bug. Once posted, I can push it against our test servers and make sure it passes everything.
-

To create a patch to upload - see
https://wiki.mozilla.org/Mobile/Fennec/Android/Suggested_workflow#Creating_commits_and_submitting_patches

You can find more miscellaneous information at: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message us on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Looking at this briefly, to clarify after a question from irc, the name of this bug may be out of date – the with* methods may have already been renamed.

The general idea here is that there is some functionality similar to:

      return uri.buildUpon()
                .appendQueryParameter(param, "" + value)
                .build();

that we can place into shared helper methods to make them less error prone. See the attached patch for an example and the obsolete patches (under "Show obsolete attachments") should give further insight into how this bug should work.

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To respond to IRC, there are two general ways (that I know of) to add changes to the main line of code:
 * Rebasing: to "rebase" is to change the parent of a series of commits to another parent – think of taking your stack of commits of one commit and placing that stack on the top of the tree.
 * Merging: to "merge" is to take a diff between two sets of files (the topmost commit you're merging from to the commit you're merging to) and then insert that diff onto the commit you're merging to – think of interleaving your changes to the code with the code at the top of the tree

They have trade-offs:
 * When merging goes right, it's an easier process – just say where you want your changes and then they're there. Rebasing requires you to be more specific (which commits do you want to move where?).
 * When things go wrong, I find merging to be confusing – it's unclear what the state of the code was that you're merging into, which lines came from where. etc. Rebasing applies each commit one at a time so there are less changes to handle at once and, given each commit is made to solve one problem, it's more clear why these changes are being made.
 * With revision history, having the original commits on the top of the tree (via rebase) makes it clear what the intentions were behind the code change but a merge becomes a large blob of a code change (via merge) – you have to take more steps to find the original commits.
 * Merge creates a hard-to-follow non-linear history (see [1] for a non-extreme example) whereas rebase creates a linear revision history

Disclaimer: I personally advocate for rebasing but others might disagree (e.g. github allows you to merge through their UI, probably for the simplicity).

At Mozilla, we rebase our changes onto the top of the tree before landing them.

I'm not sure why the internet says you may not want to do this with mercurial – maybe they're referring to commits that have already been pushed to the public?

My typical workflow is: `hg rebase -b <commit-I-want-to-put-on-tree> -d <destination-commit>` where b=base and d=destination. See more with `hg help rebase`.

For more information, you can also check out Mozilla's hg guide: http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[1]: https://duckduckgo.com/?q=confusing+merging+trees+mercurial&t=ffab&iar=images&iax=1&ia=images&iai=http%3A%2F%2Faras-p.info%2Fblog%2Fwp-content%2Fuploads%2F2011%2F04%2Fkiln-merge-spiderweb.png

Comment 30

[mddrill]

(In reply to Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] from comment #28)
> To respond to IRC, there are two general ways (that I know of) to add
> changes to the main line of code:
>  * Rebasing: to "rebase" is to change the parent of a series of commits to
> another parent – think of taking your stack of commits of one commit and
> placing that stack on the top of the tree.
>  * Merging: to "merge" is to take a diff between two sets of files (the
> topmost commit you're merging from to the commit you're merging to) and then
> insert that diff onto the commit you're merging to – think of interleaving
> your changes to the code with the code at the top of the tree
> 
> They have trade-offs:
>  * When merging goes right, it's an easier process – just say where you want
> your changes and then they're there. Rebasing requires you to be more
> specific (which commits do you want to move where?).
>  * When things go wrong, I find merging to be confusing – it's unclear what
> the state of the code was that you're merging into, which lines came from
> where. etc. Rebasing applies each commit one at a time so there are less
> changes to handle at once and, given each commit is made to solve one
> problem, it's more clear why these changes are being made.
>  * With revision history, having the original commits on the top of the tree
> (via rebase) makes it clear what the intentions were behind the code change
> but a merge becomes a large blob of a code change (via merge) – you have to
> take more steps to find the original commits.
>  * Merge creates a hard-to-follow non-linear history (see [1] for a
> non-extreme example) whereas rebase creates a linear revision history
> 
> Disclaimer: I personally advocate for rebasing but others might disagree
> (e.g. github allows you to merge through their UI, probably for the
> simplicity).
> 
> At Mozilla, we rebase our changes onto the top of the tree before landing
> them.
> 
> I'm not sure why the internet says you may not want to do this with
> mercurial – maybe they're referring to commits that have already been pushed
> to the public?
> 
> My typical workflow is: `hg rebase -b <commit-I-want-to-put-on-tree> -d
> <destination-commit>` where b=base and d=destination. See more with `hg help
> rebase`.
> 
> For more information, you can also check out Mozilla's hg guide:
> http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/
> index.html

So I'm assuming what I'm supposed to do is apply the patch to the copy of the source code that I have and rebase that code to the main line of code, right?

I'm having problems applying the patch, i keep getting "Hunk #<number> FAILED at <line-number>" for several of the hunks.  

Also, where is the destination-commit you are refering to in `hg rebase -b <commit-I-want-to-put-on-tree> -d`?
> <destination-commit>`

Comment 31

[mddrill]

Attachment: Bug1118971_v7.patch

There were hunks in patch six which tried to edit methods that do not exist in the current source code. I just ignored these hunks as I couldn't figure out what they were supposed to fix.

Comment 32

[mddrill]

Attachment: Bug1118971_v7.patch

Fixed typo

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8799872 [details] [diff] [review]
Bug1118971_v7.patch

Grisha, can you take a look at this?

Comment 34

[:Grisha Kruglov]

(In reply to mddrill from comment #30)
> Also, where is the destination-commit you are refering to in `hg rebase -b
> <commit-I-want-to-put-on-tree> -d`?
> > <destination-commit>`

It's a good idea to rebase on top of "central". If you've got firefoxtree extension setup (http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxtree.html), you can just do `hg rebase -s your-commit -d central`.

Comment 35

[:Grisha Kruglov]

Comment on attachment 8799872 [details] [diff] [review]
Bug1118971_v7.patch

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

Thanks, mddrill! Patch seems to apply just fine on top of central, everything builds and checkstyle passes. I think with one more functional change this should be OK to land. I'll push this to our try servers later today (don't have my ssh keys handy at the moment).

Re-flag me for a review once you've updated the patch, and we can land it afterwards.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
@@ +551,4 @@
>      }
>  
>      private static Uri withDeleted(final Uri uri) {
> +        return DBUtils.appendParameterValue(uri, BrowserContract.PARAM_SHOW_DELETED, "1");

I think one of the goals of this exercise is to ensure we don't have code like this floating around. Specifically, specifying some "magic" value for some parameter, remembering which value to specify for which param, etc., all spread out through different files.

As such, this would make another good method, "appendShowDeleted" perhaps, which covers this specific case.

Also, let's use "true" for this value. We simply check if value is present or not, "1", "true" or "blah" are all equivalent, but "true" is consistent with other code.

Comment 36

[:Grisha Kruglov]

(In reply to mddrill from comment #32)
> Created attachment 8799872 [details] [diff] [review]
> Bug1118971_v7.patch
> 
> Fixed typo

Also, if you could push your work to MozReview, that would make reviewing, triggering try pushes (=running automated tests) and landing code easier :-) It is the suggested workflow. It also makes life easier for you - no need to export patches, etc - just ensure you have a proper commit message and run `hg push review`.

For example, to flag me for a review and attach review to this bug, your message should be something like this: "Bug 1118971 - Lift append* methods into DBUtils r=grisha". If you don't know who should review you patch, you can omit "r=" part of the commit message.

Setting up your machine should be easy - all you need to get the basics working (`hg push review`) is a bugzilla account, which you already have.

Here are the instructions: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html

Comment 37

[mddrill]

(In reply to :Grisha Kruglov from comment #36)
> (In reply to mddrill from comment #32)
> > Created attachment 8799872 [details] [diff] [review]
> > Bug1118971_v7.patch
> > 
> > Fixed typo
> 
> Also, if you could push your work to MozReview, that would make reviewing,
> triggering try pushes (=running automated tests) and landing code easier :-)
> It is the suggested workflow. It also makes life easier for you - no need to
> export patches, etc - just ensure you have a proper commit message and run
> `hg push review`.
> 
> For example, to flag me for a review and attach review to this bug, your
> message should be something like this: "Bug 1118971 - Lift append* methods
> into DBUtils r=grisha". If you don't know who should review you patch, you
> can omit "r=" part of the commit message.
> 
> Setting up your machine should be easy - all you need to get the basics
> working (`hg push review`) is a bugzilla account, which you already have.
> 
> Here are the instructions:
> http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> install.html

So do I use MozReview instead of firefoxtree? or do I use both? The link you gave me for MozReview doesn't work, and do I need to download firefoxtree? There's no download link in the link you posted. Sorry, I'm new to all this version control stuff, it's a little confusing. I updated the withDeleted() method, I think I did what you were asking me to do.

Comment 38

[:Grisha Kruglov]

(In reply to mddrill from comment #37)
> (In reply to :Grisha Kruglov from comment #36)
> > (In reply to mddrill from comment #32)
> > > Created attachment 8799872 [details] [diff] [review]
> > > Bug1118971_v7.patch
> > > 
> > > Fixed typo
> > 
> > Also, if you could push your work to MozReview, that would make reviewing,
> > triggering try pushes (=running automated tests) and landing code easier :-)
> > It is the suggested workflow. It also makes life easier for you - no need to
> > export patches, etc - just ensure you have a proper commit message and run
> > `hg push review`.
> > 
> > For example, to flag me for a review and attach review to this bug, your
> > message should be something like this: "Bug 1118971 - Lift append* methods
> > into DBUtils r=grisha". If you don't know who should review you patch, you
> > can omit "r=" part of the commit message.
> > 
> > Setting up your machine should be easy - all you need to get the basics
> > working (`hg push review`) is a bugzilla account, which you already have.
> > 
> > Here are the instructions:
> > http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> > install.html
> 
> So do I use MozReview instead of firefoxtree? or do I use both? The link you
> gave me for MozReview doesn't work, and do I need to download firefoxtree?
> There's no download link in the link you posted. Sorry, I'm new to all this
> version control stuff, it's a little confusing. I updated the withDeleted()
> method, I think I did what you were asking me to do.

You use both MozReview and firefoxtree (a mercurial extension). MozReview is a review tool we use (see sample review: https://reviewboard.mozilla.org/r/85056/diff/3/).

I believe running |mach bootstrap| will guide you through getting a good local Mercurial setup going (including extensions such as firefoxtree, etc), so give that a go. More information: http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html

I'm not sure what you mean about a broken link for MozReview - it works for me. Let's try this again though: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html

Comment 39

[mddrill]

Attachment: Bug 1118971 - Lift append* methods into DBUtils

> into DBUtils r=grisha

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

Comment 40

[:Grisha Kruglov]

I think you've pushed up the wrong thing? I see a one-line change in the patch you pushed: https://reviewboard.mozilla.org/r/86656/diff/1#index_header

Also, the commit message seems to have been split up into multiple lines.

Comment 41

[mddrill]

(In reply to :Grisha Kruglov from comment #40)
> I think you've pushed up the wrong thing? I see a one-line change in the
> patch you pushed: https://reviewboard.mozilla.org/r/86656/diff/1#index_header
> 
> Also, the commit message seems to have been split up into multiple lines.

What happened was I committed my changes in mercurial, then realized i didn't flag you in the commit message. I tried to recommit it, but it wouldn't let me saying that there had been no changes since the last update. So I added that asterisk you saw in order to be able to recommit with a new commit message, not realizing it would only show the asterisk.  Do you know how I can change the commit message of the original commit?

Comment 42

[:Grisha Kruglov]

You can use |hg histedit| to modify your previous commits. I believe flagging commit as "mess" in histedit will let you rewrite commit message without modifying any code changes itself. Then you'll be able to re-push.

Also, if it's just the reviewer you want to specify/change, you can do that in reviewboard itself (on top of the page, in the "commit" table). Adding a reviewer in the commit message is good, but if you didn't do that, you can specify reviewer (or reviewers) in the reviewboard interface. I believe that once the patch lands via autoland, correct reviewers will be automatically added to the commit message.

If for some reason you can't edit the reviewers column, you can just ask a specific reviewer to edit it themselves.

But yeah, histedit should be all you need in the case.

Comment 43

[mddrill]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

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

Comment 44

[mddrill]

(In reply to :Grisha Kruglov from comment #42)
> You can use |hg histedit| to modify your previous commits. I believe
> flagging commit as "mess" in histedit will let you rewrite commit message
> without modifying any code changes itself. Then you'll be able to re-push.
> 
> Also, if it's just the reviewer you want to specify/change, you can do that
> in reviewboard itself (on top of the page, in the "commit" table). Adding a
> reviewer in the commit message is good, but if you didn't do that, you can
> specify reviewer (or reviewers) in the reviewboard interface. I believe that
> once the patch lands via autoland, correct reviewers will be automatically
> added to the commit message.
> 
> If for some reason you can't edit the reviewers column, you can just ask a
> specific reviewer to edit it themselves.
> 
> But yeah, histedit should be all you need in the case.

ok, I submitted it.

Comment 45

[:Grisha Kruglov]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/86656/#review88602

Looks good to me. Thanks, mddrill! I'll push it to our try servers to run this through tests and once everything's green it can auto-land.

Comment 46

[:Grisha Kruglov]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/86656/#review88652

You have a few spelling and copy/paste errors in the code. Before re-submitting for a review, please ensure project compiles locally for you.

::: mobile/android/base/java/org/mozilla/gecko/db/DBUtils.java:252
(Diff revision 2)
>  
> -    public static Uri appendProfile(final String profile, final Uri uri) {
> -        return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE, profile).build();
> +    public static Uri appendParameterValue(final Uri uri, final String parameter, final String value) {
> +        return uri.buildUpon().appendQueryParameter(parameter, value).build();
> +    }
> +    public static Uri appendShowDeleted(final Uri uri){
> +	return appendParameterValue(uri, BrowserConrtract.PARAM_SHOW_DELETED, "true");

Spelling error in `BrowserContract`

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1444
(Diff revision 2)
>              if (title != null) {
>                  values.put(History.TITLE, title);
>              }
>              values.put(History.URL, url);
>  
> -            final Uri historyUri = withDeleted(mHistoryUriWithProfile).buildUpon().
> +            final Uri historyUri = DBUtils.appendInsertIfNeededAndDBUtils.appendShowDeleted(mHistoryUriWithProfile);

Incorrect method name

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1566
(Diff revision 2)
>              parent = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
>          }
>          values.put(Bookmarks.PARENT, parent);
>          values.put(Bookmarks.TYPE, type);
>  
> -        Uri bookmarkUri = withDeleted(mBookmarksUriWithProfile).buildUpon().
> +        final Uri bookmarkUri = DBUtils.appendInsertIfNeededAndDBUtils.appendShowDeleted(mBookmarksUriWithProfile);

Incorrect method name.

Comment 47

[mddrill]

(In reply to :Grisha Kruglov from comment #46)
> Comment on attachment 8802162 [details]
> Bug 1118971 - Lift append* methods into DBUtils
> 
> https://reviewboard.mozilla.org/r/86656/#review88652
> 
> You have a few spelling and copy/paste errors in the code. Before
> re-submitting for a review, please ensure project compiles locally for you.
> 
> ::: mobile/android/base/java/org/mozilla/gecko/db/DBUtils.java:252
> (Diff revision 2)
> >  
> > -    public static Uri appendProfile(final String profile, final Uri uri) {
> > -        return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE, profile).build();
> > +    public static Uri appendParameterValue(final Uri uri, final String parameter, final String value) {
> > +        return uri.buildUpon().appendQueryParameter(parameter, value).build();
> > +    }
> > +    public static Uri appendShowDeleted(final Uri uri){
> > +	return appendParameterValue(uri, BrowserConrtract.PARAM_SHOW_DELETED, "true");
> 
> Spelling error in `BrowserContract`
> 
> ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1444
> (Diff revision 2)
> >              if (title != null) {
> >                  values.put(History.TITLE, title);
> >              }
> >              values.put(History.URL, url);
> >  
> > -            final Uri historyUri = withDeleted(mHistoryUriWithProfile).buildUpon().
> > +            final Uri historyUri = DBUtils.appendInsertIfNeededAndDBUtils.appendShowDeleted(mHistoryUriWithProfile);
> 
> Incorrect method name
> 
> ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1566
> (Diff revision 2)
> >              parent = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
> >          }
> >          values.put(Bookmarks.PARENT, parent);
> >          values.put(Bookmarks.TYPE, type);
> >  
> > -        Uri bookmarkUri = withDeleted(mBookmarksUriWithProfile).buildUpon().
> > +        final Uri bookmarkUri = DBUtils.appendInsertIfNeededAndDBUtils.appendShowDeleted(mBookmarksUriWithProfile);
> 
> Incorrect method name.

Should I redo the commit or make a new commit with only these changes?

Comment 48

[:Grisha Kruglov]

Use the same commit, augment it with changes, and re-push.

Two options to do that:
1) Make the changes locally, commit them, and them, then run |hg histedit| and roll new commit into original commit. Then push for review.
2) Or, using |hg histedit|, mark your commit for edit, make your changes, commit them with a proper message (same you've used originally), and then run |hg histedit --continue| to finish. The push for review.

Comment 49

[mddrill]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

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

Comment 50

[:Grisha Kruglov]

As discussed on IRC, there are test and checkstyle failures (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=520146f92350&selectedJob=30251901).

Let's give this another pass, and try again.

Comment 51

[mddrill]

(In reply to :Grisha Kruglov from comment #50)
> As discussed on IRC, there are test and checkstyle failures (see
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=520146f92350&selectedJob=30251901).
> 
> Let's give this another pass, and try again.

I fixed the checkstyle failure, but to be honest I don't really understand what these test failure messages mean. It doesn't say which part of the code the error occurs in either.

TEST-UNEXPECTED-FAIL | testBrowserProvider - TestInsertUrlMetadata | URL metadata inserted via UrlMetadata not found - got false, expected true
0 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testBrowserProvider - TestInsertUrlMetadata | URL metadata inserted via UrlMetadata not found - got false, expected true
TEST-UNEXPECTED-FAIL | testBrowserProvider - TestInsertUrlMetadata | Exception caught - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testBrowserProvider - TestInsertUrlMetadata | URL metadata inserted via UrlMetadata not found - got false, expected true
TEST-UNEXPECTED-FAIL | testFilterOpenTab - TestInsertLocalTabs | 1 tabs entries found - got 0, expected 1
 1207879 Intermittent testFilterOpenTab - TestInsertLocalTabs | application crashed [None]
 1247469 Permafailing testFilterOpenTab - TestInsertLocalTabs | 1 tabs entries found - got 0, expected 1
1 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testFilterOpenTab - TestInsertLocalTabs | 1 tabs entries found - got 0, expected 1
 1207879 Intermittent testFilterOpenTab - TestInsertLocalTabs | application crashed [None]
 1247469 Permafailing testFilterOpenTab - TestInsertLocalTabs | 1 tabs entries found - got 0, expected 1
TEST-UNEXPECTED-FAIL | testFilterOpenTab - TestInsertLocalTabs | Exception caught - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testFilterOpenTab - TestInsertLocalTabs | 1 tabs entries found - got 0, expected 1
 1207879 Intermittent testFilterOpenTab - TestInsertLocalTabs | application crashed [None]
 1247469 Permafailing testFilterOpenTab - TestInsertLocalTabs | 1 tabs entries found - got 0, expected 1
Return code: 1

Comment 52

[:Grisha Kruglov]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/86656/#review91658

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1444
(Diff revision 3)
>              if (title != null) {
>                  values.put(History.TITLE, title);
>              }
>              values.put(History.URL, url);
>  
> -            final Uri historyUri = withDeleted(mHistoryUriWithProfile).buildUpon().
> +            final Uri historyUri = DBUtils.appendShowDeleted(mHistoryUriWithProfile);

This is missing PARAM_INSERT_IF_NEEDED.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1566
(Diff revision 3)
>              parent = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
>          }
>          values.put(Bookmarks.PARENT, parent);
>          values.put(Bookmarks.TYPE, type);
>  
> -        Uri bookmarkUri = withDeleted(mBookmarksUriWithProfile).buildUpon().
> +        final Uri bookmarkUri = DBUtils.appendShowDeleted(mBookmarksUriWithProfile);

This is also missing PARAM_INSERT_IF_NEEDED.

Comment 53

[:Grisha Kruglov]

Try fixing problems in Comment 52, and let's see if tests pass then.

Comment 54

[mddrill]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

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

Comment 55

[mddrill]

OK, I fixed them, event though I don't really understand the errors. `DBUtils.appendShowDeleted()` only takes one argument.

Comment 56

[:Grisha Kruglov]

(In reply to mddrill from comment #55)
> OK, I fixed them, event though I don't really understand the errors.
> `DBUtils.appendShowDeleted()` only takes one argument.

Perhaps looking at the broader picture will help you understand what's going on.

These helper methods serve one purpose: construct URIs. In the context of this bug, we use URIs internally to tell our ContentProviders to perform certain actions. Within Fennec, you can think of a ContentProvider as a "model" layer, the layer which performs various actions on the database, CRUD operations such as insert, update, delete, but also compound operations such as "insert into table X, while also updating table Y".
An example of a CRUD action is "insert a history record". An example of a compound action might be "record a visit for a history record, update a 'visits' count on the history record". Another simple operation might be "return up to 50 latest history records, including those which are marked as 'deleted'".

To perform these actions, we first need to figure out which ContentProvider can handle them, and second we need to tell the ContentProvider exactly what needs to be done. This is where URIs come in, they are both a mapping to a ContentProvider, and a way to pass in additional parameters to its high level methods (insert, update, delete, etc).

Here's a hypothetical URI: content://org.mozilla.firefox/history?with_deleted=true&limit=50
Here we're specifying "org.mozilla.firefox" as our "authority", setting "history" as path signifying that we'd like to work with history records, and setting two additional parameters: "with_deleted" and "limit".

With this URI in hand, we may now query our ContentProvider for records. A "query" call will be first routed to BrowserProvider's query method, which will figure out that it needs to run a query on the history table, which might go something like this: "select * from history where is_deleted=1 or is_deleted=1 limit 50".

Getting back to failing tests. In the two places I pointed out, original code constructed a URI of the form:
"content://stuff?with_deleted=true&insert_if_needed=true", essentially passing in two additional arguments to our ContentProvider: "with_deleted" and "insert_if_needed". After your change, we would get something like this: "content://stuff?with_deleted=true". As you can see, one of the parameters is now omitted, and as a result the code which is talking to our ContentProvider is no longer doing the right thing. Thankfully, some tests caught this problem.

To fix this, you need to construct the same URI as before, but using your new helper methods. If you look closely, they all take a Uri object as a parameter, and return another Uri object. This allows you to compound your calls to these methods.

Pseudo-example: Uri properUri = appendInsertIfNeeded(appendShowDeleted(uri));

You also have a `appendInsertIfNeededAndWithDeleted` method, which seems to be a good option in the case as well.

Generally, this type of code might be better off following a Builder pattern, but it's probably not worth the hassle in our case.

Comment 57

[mddrill]

I changed the methods to `appendInsertIfNeededAndWithDeleted` Would it have been better if I had done `appendWithDeleted(appendInsertIfNeeded(mHistoryUriWithProfile)` ?

Comment 58

[mddrill]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

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

Comment 59

[:Grisha Kruglov]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/86656/#review92746

Code doesn't compile, it seems you've deleted a variable by accident.

Minimal check list of things to do before you push for a review:
- ensure code compiles and works as you intended (build+package+run in an emulator/device, tap around, etc...)
- ensure checkstyle passes (./mach gradle app:checkstyle)
- ensure unit tests pass (./mach gradle app:test or just ./gradlew test)

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1569
(Diff revision 5)
>  
> -        Uri bookmarkUri = withDeleted(mBookmarksUriWithProfile).buildUpon().
> +
> -            appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
>          // Update or insert
>          ContentProviderOperation.Builder builder =
>              ContentProviderOperation.newUpdate(bookmarkUri);

You've deleted `bookmarkUri`.

Comment 60

[mddrill]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86656/diff/5-6/

Comment 61

[mddrill]

I pushed another review. I was getting a testDSAgeneration error when running gradle app:test, but I saw this post https://bug635044.bugzilla.mozilla.org/show_bug.cgi?id=1275423 which says that that's just a bug and we don't need to worry about it.

Comment 62

[:Grisha Kruglov]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/86656/#review96868

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1571
(Diff revision 6)
> -        Uri bookmarkUri = withDeleted(mBookmarksUriWithProfile).buildUpon().
> -            appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
> +
> +        final Uri historyUri = DBUtils.appendInsertIfNeededAndWithDeleted(mHistoryUriWithProfile);
> +
>          // Update or insert
>          ContentProviderOperation.Builder builder =
> -            ContentProviderOperation.newUpdate(bookmarkUri);
> +            ContentProviderOperation.newUpdate(mBookmarksUriWithProfile);

This does not look correct. mbookmarksUriWithProfile does not have the insert_if_needed flag. Also, notice that there's a comment right above, stating "Update or insert" - which this wouldn't achieve. 

Some tests are hopefully going to catch this.

Comment 63

[:Grisha Kruglov]

I triggered a try build, let's see if tests catch the bug I think you currently have in the code.

Comment 64

[mddrill]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86656/diff/6-7/

Comment 65

[mddrill]

I push a new commit that I think fixes this

Comment 66

[:Grisha Kruglov]

Tests are still failing. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b49577cd817

Comment 67

[:Grisha Kruglov]

Unassigned for inactivity. The latest patch is nearly there, just needs minor changes to land it. This is still a great first bug.

Comment 68

[:Grisha Kruglov]

Comment on attachment 8802162 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/86656/#review128794

Failing tests. See other comments in the bug.

Comment 69

[friedger]

Attachment: Bug 1118971 - Lift append* methods into DBUtils

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

Comment 70

[friedger]

Comment on attachment 8862839 [details]
Bug 1118971 - Lift append* methods into DBUtils

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

Comment 71

[:Grisha Kruglov]

Comment on attachment 8862839 [details]
Bug 1118971 - Lift append* methods into DBUtils

https://reviewboard.mozilla.org/r/134762/#review148904

My apologies for a late review! This looks good. Let's wait for the tests to pass before landing.

::: mobile/android/base/java/org/mozilla/gecko/db/DBUtils.java:254
(Diff revision 2)
> -        return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE, profile).build();
> +        return uri.buildUpon().appendQueryParameter(parameter, value).build();
> +    }
> +    public static Uri appendShowDeleted(final Uri uri) {
> +        return appendParameterValue(uri, BrowserContract.PARAM_SHOW_DELETED, "true");
> +    }
> +    public static Uri appendProfile(final Uri uri, final String profile) {

It's fine here, but generally try to avoid unnecessary "churny" chanages like this one.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:144
(Diff revision 2)
>  
>      public LocalBrowserDB(String profile) {
>          mProfile = profile;
>          mFolderIdMap = new HashMap<String, Long>();
>  
> -        mBookmarksUriWithProfile = DBUtils.appendProfile(profile, Bookmarks.CONTENT_URI);
> +        mBookmarksUriWithProfile = DBUtils.appendProfile(Bookmarks.CONTENT_URI, profile);

See! Very churny and wins us nothing.

Comment 72

[friedger]

Thanks for the review.

I see two failing tests, but I don't see how this is related to the changes of the patch. I don't have a running test environment yet. Can you give me some pointers?

(I like the way that DBUtils now have a more streamlined API with URI first and parameter values afterwards)

Comment 73

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 74

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.