1188271 - "Import bookmarks and history" should be disabled or warn if no data is available

Status: RESOLVED INCOMPLETE

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

Description

[Gian-Carlo Pascutto [:gcp]]

https://bugzilla.mozilla.org/show_bug.cgi?id=963661
https://bugzilla.mozilla.org/show_bug.cgi?id=1186037#c3

Comment 1

[Gian-Carlo Pascutto [:gcp]]

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/AndroidImport.java#48
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/AndroidImport.java#96

Need to pull out those 2 into a separate function, rewrite the "SQL" so they only check if anything is available (rather than returning the entire result set).

Then call that function from here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/AndroidImportPreference.java

Either when showing the dialog (and graying out what isn't available) or just before running.

Comment 2

[Sergej Kravcenko]

Are you want to use this function in ui thread? Otherwise we need some sort of ui, to show check is in progress.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

I agree it's not ideal. I'm sortof hoping that formulating the SQL/ContentProvider query as a boolean "any data present or not" makes it return near-instantly so it doesn't cause any jank.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

If it does cause yank or too much StrictMode spam, then we can try displaying a message that "no bookmarks are available for import or the default browser is keeping them private" or something similar.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

To clarify, I mean doing the latter if the user has already pressed Import and we notice that it didn't do anything - that's basically only adding some UI and avoids the UI-thread<>DB interaction.

Comment 6

[Sergej Kravcenko]

ContentResolver doesn't provide method to check row count. It's possible to alter SQL statement to limit row count or inject count(*), but this is 'not documented'/hacky way to do it.
I'll make a patch to test it without row retrieval (with current SQL statements), but that means we will perform same SQL twice (check and actual import).

Comment 7

[Gian-Carlo Pascutto [:gcp]]

(In reply to Sergej Kravcenko from comment #6)
> It's possible to alter SQL statement to limit row count or inject count(*), but this is 'not
> documented'/hacky way to do it.

You mean adding a LIMIT like this?
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#509

So I guess the issue is that there's no hard guarantee the backing ContentProvider implements that? I think it's OK to assume this. We know it should work on most and we can test on the affected devices. If the device has a ContentProvider that is sufficiently different that it doesn't support that, I guess our odds of successfully importing have dropped anyway.

> I'll make a patch to test it without row retrieval (with current SQL
> statements), but that means we will perform same SQL twice (check and actual
> import).

I wouldn't do this without the LIMIT. If we take this approach then it's better to just notice nothing happened during the import and pop up a message afterwards. We need a good formulation for explaining to the user why we can't help them, though.

Comment 8

[Sergej Kravcenko]

I've done some research, at least on my device (5.0.2) limit does nothing, query returns full dataset. The other problem, firefox uses standard android preferences and disabling list item inside DialogPreference dialog will require quite large changes.
I think better solution is to show info dialog afterwards.

Comment 9

[Ahmed Abdi Ali]

How would you start on this bug and could you assign me this bug

Comment 10

[Ahmed Abdi Ali]

How would you start on this bug and could you assign me this bug

Comment 11

[Ivan]

How do you find the path of this bug in mozzila-central and I want to make a patch about it.

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

We don't assign bugs until there is a patch. Comments 0 and 1 deal with where the changes that need to be made are. You will need to be able to build Firefox for Android on OS X or Linux. See https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec for details. If you run into issues irc://irc.mozilla.org/#mobile is a place to ask for help with the build.

Comment 13

[taru.saumya]

How can i start working on this bug? Please let me know the steps.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

(In reply to taru.saumya from comment #13)
> How can i start working on this bug? Please let me know the steps.

You should be able to make a custom build of Fennec and get it running on a device or the emulator. See comment 12. Then read through this bug more carefully and ask specific questions about things you don't understand.

Comment 15

[varunnaganathan912]

Hey,I wanted to work on this.So finally should i aim at just displaying a message if nothing happens on clicking "Import Bookmarks" or do you want an implemented function as mentioned in comment #1 ?

Comment 16

[varunnaganathan912]

Also could someone helpme in finding the "Import Bookmarks and History" option in Fennec.I am unable to do so.Thanks!

Comment 17

[varunnaganathan912]

Also while i was enquiring about this option at the IRC channel, some developers told me that this option was not present in Android 6.0.Could someone also help me with that possibility?

Comment 18

[Gian-Carlo Pascutto [:gcp]]

(In reply to varunnaganathan912 from comment #15)
> Hey,I wanted to work on this.So finally should i aim at just displaying a
> message if nothing happens on clicking "Import Bookmarks" or do you want an
> implemented function as mentioned in comment #1 ?

I believe comment 7 and comment 8 address this: we probably only want to show the message, as it's more difficult than anticipated to quickly check if the import would return anything.

Comment 19

[varunnaganathan912]

So basically i  check the database for any data as metioned in comment #1 and if the dataset is empty ,display a message right?
Also the links provided in comment #1 return a 404.Could you redirect me to the right place to begin.Thanks a lot!

Comment 20

[varunnaganathan912]

Also about the android 6.0 not having an import option?

Comment 21

[Gian-Carlo Pascutto [:gcp]]

(In reply to varunnaganathan912 from comment #19)
> So basically i  check the database for any data as metioned in comment #1
> and if the dataset is empty ,display a message right?
> Also the links provided in comment #1 return a 404.Could you redirect me to
> the right place to begin.Thanks a lot!

It's because the layout of the Java files has been changed since then. Just look for the same filename in your local source tree.

https://dxr.mozilla.org/mozilla-central/rev/c2256ee8ae9a8ee0bf7ab49a8b1924720d846cc7/mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java#71

>Also about the android 6.0 not having an import option?

See this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1183559

Comment 22

[varunnaganathan912]

Hi ,I'm facing problems in understanding what you suggested in comment #1.Do you want me to actually merge both the functions(merge boookmarks and merge history) completely or just the part of the query to the database?
Also the conditions for the query are "bookmarks = 1".How is this condition able to get all the bookmarks.
Sorry I'm taking a while to get a hang of things.

Comment 23

[varunnaganathan912]

Attachment: Added a check for bookmarks and history before importing

So basically,I added a count query for bookmarks and history before the actual import begins.If the count is 0 ,rather than running the import i simply display a message based on what the user actually requested to be imported.
Please Review.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8714111 [details] [diff] [review]
Added a check for bookmarks and history before importing

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

Some remarks included.

Also, I pointed you to comment 7 and comment 8. The way you structured this means that we will do the query twice. In those comments it's already pointed out this is inefficient and probably not what we want. I think you can just "notice" if the existing query doesn't return anything and show the toast in that case.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java
@@ +35,5 @@
>      private final Context mContext;
>  
> +   //Uri and strings for bookmarks and history checks
> +    public static final Uri SAMSUNG_BOOKMARKS_URI = Uri.parse("content://com.sec.android.app.sbrowser.browser/bookmarks");
> +    public static final Uri SAMSUNG_HISTORY_URI = Uri.parse("content://com.sec.android.app.sbrowser.browser/history");

I'd rather not hardcode these twice, can you use the ones from AndroidImport.java?
Maybe all this new code belongs there in the first place?

@@ +89,5 @@
> +    public int checkBookmarksAndHistory(final boolean doBookmarks, final boolean doHistory)
> +    {
> +
> +        final ContentResolver mCr = mContext.getContentResolver();
> +        Cursor bookmarkCountCursor =  null;

nit: spurious whitespace

@@ +91,5 @@
> +
> +        final ContentResolver mCr = mContext.getContentResolver();
> +        Cursor bookmarkCountCursor =  null;
> +        Cursor historyCountCursor = null;
> +        bookmarkCountCursor = mCr.query(BOOKMARKS_URI,new String[] {"count(*) AS count "}, BOOKMARK + " = 1",null,null);

Can you make sure the general indentation and whitespace use is consistent with the existing code in the file?

@@ +92,5 @@
> +        final ContentResolver mCr = mContext.getContentResolver();
> +        Cursor bookmarkCountCursor =  null;
> +        Cursor historyCountCursor = null;
> +        bookmarkCountCursor = mCr.query(BOOKMARKS_URI,new String[] {"count(*) AS count "}, BOOKMARK + " = 1",null,null);
> +        bookmarkCountCursor.moveToFirst();

This needs a null check. Also probably check the result of the call?

@@ +119,5 @@
> +        else if(doHistory)
> +        {
> +            if(historycount <= 0)
> +            {
> +                Toast toast = Toast.makeText(getContext(), "No history available", Toast.LENGTH_LONG);

Why mContext before and getContext() now?

@@ +134,5 @@
>          if (!doBookmarks && !doHistory) {
>              return;
>          }
>  
> +

nit: bogus whitespace change

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Clearing the needinfo. This question was already answered in comment 18: we just want to show a toast if the existing code doesn't do anything.

Comment 26

[varunnaganathan912]

Hey.thanks for the review, will fix the code.just one question, when you're telling me to notice the results from the existing query, u mean I simply check the count of the results returned from the existing query ?

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Yes. You don't need an SQL COUNT() or anything, you can just see if the result cursor has getCount() == 0 and return a flag saying whether the mergeBookmarks and mergeHistory functions did anything.

Comment 28

[varunnaganathan912]

Attachment: Added a check for bookmarks and history before importing

As asked,I added a count check on the cursor to see if history and bookmarks is present or not and displayed the corresponding toast accordingly.Also moved the function to AndroidImport instead and removed unnecessary newlines and whitespaces.
Please review.

Comment 29

[varunnaganathan912]

Attachment: Added a check for bookmarks and history before importing

Hi,Could you please ignore the previous patch.
As asked,I added a count check on the cursor to see if history and bookmarks is present or not and displayed the corresponding toast accordingly.Also moved the function to AndroidImport instead and removed unnecessary newlines and whitespaces.
Please review.

Comment 30

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8714774 [details] [diff] [review]
Added a check for bookmarks and history before importing

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

Bunch of remarks, this needs at lease one more pass.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java
@@ +24,4 @@
>  
>  import java.util.ArrayList;
>  
> +import ch.boye.httpclientandroidlib.TruncatedChunkException;

Where does this come from?

@@ +53,5 @@
> +    public static final String NO_BOOKMARKS = "Bookmarks not available";
> +    public static final String NO_HISTORY = "History not available";
> +    public static final String NO_HISTORY_AND_BOOKMARKS = "No History or Bookmarks available";
> +    public boolean bookmarksFlag = true;
> +    public boolean historyFlag = true;

Move these down next to the other fields below, and prefix them with "m" like the other fields.

@@ +75,5 @@
>          mImportBookmarks = doBookmarks;
>          mImportHistory = doHistory;
>      }
>  
> +    public void BookmarksHistoryCheck(boolean doBookmarks, boolean doHistory, boolean isBookmarks, boolean isHistory) {

There's no need to pass instance fields into a method. You already have access to them.

@@ +77,5 @@
>      }
>  
> +    public void BookmarksHistoryCheck(boolean doBookmarks, boolean doHistory, boolean isBookmarks, boolean isHistory) {
> +        if (doBookmarks && doHistory) {
> +            if(!isBookmarks && !isHistory) {

nit: spaces around the if like in the rest of the file

@@ +79,5 @@
> +    public void BookmarksHistoryCheck(boolean doBookmarks, boolean doHistory, boolean isBookmarks, boolean isHistory) {
> +        if (doBookmarks && doHistory) {
> +            if(!isBookmarks && !isHistory) {
> +            Toast toast = Toast.makeText(mContext, NO_HISTORY_AND_BOOKMARKS, Toast.LENGTH_LONG);
> +            toast.show();

These two lines are repeated throughout with only the second argument changing. Just make a variable containing that, and write the code once at the end.

@@ +112,5 @@
>                                 SAMSUNG_BOOKMARKS_URI,
>                                 LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 1");
>  
>              if (cursor != null) {
> +                bookmarksFlag  = (cursor.getCount()!=0);

nit: spacing around operators, spurious whitespace

Comment 31

[varunnaganathan912]

Hi,
I have 2 questions.

> +    public static final String NO_BOOKMARKS = "Bookmarks not available";
> +    public static final String NO_HISTORY = "History not available";
> +    public static final String NO_HISTORY_AND_BOOKMARKS = "No History or Bookmarks available";
> +    public boolean bookmarksFlag = true;
> +    public boolean historyFlag = true;

Move these down next to the other fields below, and prefix them with "m" like the other fields.

As suggested above,where exactly are you telling me to move them?.i thought I positioned them consistent with the rest of the String declarations.




> +    public void BookmarksHistoryCheck(boolean doBookmarks, boolean doHistory, boolean isBookmarks, boolean isHistory) {
> +        if (doBookmarks && doHistory) {
> +            if(!isBookmarks && !isHistory) {
> +            Toast toast = Toast.makeText(mContext, NO_HISTORY_AND_BOOKMARKS, Toast.LENGTH_LONG);
> +            toast.show();

These two lines are repeated throughout with only the second argument changing. Just make a variable containing that, and write the code once at the end.

So there are some instances where the import is successful.Wanted to clarify if we also needed a Toast for a successful import?

Comment 32

[Gian-Carlo Pascutto [:gcp]]

(In reply to varunnaganathan912 from comment #31)
> As suggested above,where exactly are you telling me to move them?.i thought
> I positioned them consistent with the rest of the String declarations.

They are fields (instance variables) so they should go with the other fields. The Strings you are pointing to are class constants (static final).

> So there are some instances where the import is successful.Wanted to clarify
> if we also needed a Toast for a successful import?

No, the user will see his new bookmarks and know it worked.

Comment 33

[varunnaganathan912]

Attachment: Added a bookmarks and history check

Hi,
As asked,I changed the location of the strings and displayed the corresponding toasts.Also changed the toast string to a common variable.
Note:The flag used is to check if the import returned data as asked by user in which case no toast is displayed.
Please Review

Comment 34

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8716450 [details] [diff] [review]
Added a bookmarks and history check

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

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java
@@ +57,5 @@
>      private final LocalBrowserDB mDB;
>      private final boolean mImportBookmarks;
>      private final boolean mImportHistory;
> +    private boolean mIsHistory;
> +    private boolean mIsBookmarks;

This should probably read "mHasHistory" and "mHasBookmarks".

@@ +74,5 @@
>          mImportHistory = doHistory;
>      }
>  
> +    public void BookmarksHistoryCheck() {
> +        boolean mImportFlag = true;

You prefixed this with an "m", but it's actually a local variable, not a field/member var.

We can see it's a kind of flag because it's a boolean, but it's hard to infer from the name what about "importing" that it flags. Maybe name it "shouldShowToast" or something?

@@ +75,5 @@
>      }
>  
> +    public void BookmarksHistoryCheck() {
> +        boolean mImportFlag = true;
> +        String Toastmessage = "";

Do you need the empty string here?

@@ +79,5 @@
> +        String Toastmessage = "";
> +        if (mImportBookmarks && mImportHistory) {
> +            if(!mIsBookmarks && !mIsHistory) {
> +                Toastmessage = mNoHistoryAndBookmarks;
> +                mImportFlag = false;

"mImportFlag = false" is common in all branches of this if, so it can move up one level and you can write it once instead of 4 times.

Try to avoid duplicating code, it's usually a sign something is wrong style-wise.

Maybe simplify this all to:

boolean shouldShowToast = (mImportBookmarks && !mHasBookmarks) || (mImportHistory && !mHasHistory);

Comment 35

[varunnaganathan912]

Attachment: Added a bookmarks and history check

Hi,
made the changes suggested.
Few points:
The empty string declaration if not made gives me an error in SDK.
Secondly ,I names the flag as importSuccess because its primary aim is establishing if the import was a success.If the import failed,we display a toast.
Also in the expression you suggested to evaluate the import success, I also needed to add a check for the case when the user asks to import both history and bookmarks and any one of them fails.
Please review
Thanks!

Comment 36

[Gian-Carlo Pascutto [:gcp]]

(In reply to varunnaganathan912 from comment #35)
> Also in the expression you suggested to evaluate the import success, I also
> needed to add a check for the case when the user asks to import both history
> and bookmarks and any one of them fails.

I think it handles that fine? If mImportBookmarks and mImportHistory are both true, then either mHasBookmarks or mHasHistory being false will trigger the toast.

Comment 37

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8717173 [details] [diff] [review]
Added a bookmarks and history check

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

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java
@@ +74,5 @@
>          mImportHistory = doHistory;
>      }
>  
> +    public void BookmarksHistoryCheck() {
> +        boolean importSuccess = (mImportBookmarks && !mHasBookmarks) || (mImportHistory && !mHasHistory) || ((mImportBookmarks && mImportHistory) && (!mHasHistory || !mHasBookmarks));

As explained in the previous comment I think this is more complicated than it needs to be.

@@ +82,5 @@
> +        String Toastmessage = "";
> +        if (mImportBookmarks && mImportHistory) {
> +            if(!mHasBookmarks && !mHasHistory) {
> +                Toastmessage = mNoHistoryAndBookmarks;
> +                importSuccess = false;

What's the point of this variable now? If it were true, we'd never get here in the first place.

The whole idea of factoring it out was to avoid having to set it in every branch of this if (see previous review).

Comment 38

[varunnaganathan912]

Attachment: Added a bookmarks and history check

Hi ,made the changes suggested.
Thanks! 
And sorry for the complications caused.

Comment 39

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8717527 [details] [diff] [review]
Added a bookmarks and history check

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

Looks good except for the inverted condition. I'll r+ this because that seems an easy fix.

I'm going to hand off the review now to Chenxia who's a bit more hands on with Android-specific development these days. I think this patch will need a tweak to make the used strings suitable for translation (I think you've done this before on a previous bug you fixed for us?), and because I have 2 things that seem odd to me but I don't know the answer to offhand, so Chenxia should answer them:
1) stylewise, I think we put the } else if { on the same line?
2) The String foo = ""; seems really odd.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java
@@ +74,5 @@
>          mImportHistory = doHistory;
>      }
>  
> +    public void BookmarksHistoryCheck() {
> +        boolean importSuccess = (mImportBookmarks && !mHasBookmarks) || (mImportHistory && !mHasHistory);

This is wrong. Please reread my previous review comments, and note importSuccess is exactly the *opposite* condition of shouldShowToast.

You will set importSuccess as true when you are asked to import bookmarks and there are none. That's not right, is it?

Comment 40

[varunnaganathan912]

Attachment: Added a bookmarks and history check

ya,you were right about the condition.
Made the required changes in the condition and the position of else-if.

Comment 41

[Chenxia Liu [:liuche]]

Comment on attachment 8720170 [details] [diff] [review]
Added a bookmarks and history check

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

Thanks for the patch Varun! Your logic is sound, but I had a few comments on this patch to improve the coding practices.

In general, it's good practice to avoid using member variables unless you need them, and in this code it's just a little change to avoid using them - you can just return them from the mergeBookmarks and mergeHistory calls.

Please let me know if anything I said is unclear, or if you have any questions!

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java
@@ +57,5 @@
>      private final LocalBrowserDB mDB;
>      private final boolean mImportBookmarks;
>      private final boolean mImportHistory;
> +    private boolean mHasHistory;
> +    private boolean mHasBookmarks;

On second thought, let's not use member variables here because we can just calculate these values in the run() method. It doesn't matter for this class because we create a new instance each time, but this is better programming practice (and in a case where you reuse the object, you'd have to reset these values).

So basically, in the run() method, return a boolean value for whether any items were found from the mergeBookmarks and mergeHistory calls, and then pass those into the toast call.

(See my later comments inline for more detail.)

@@ +58,5 @@
>      private final boolean mImportBookmarks;
>      private final boolean mImportHistory;
> +    private boolean mHasHistory;
> +    private boolean mHasBookmarks;
> +    private final String mNoBookmarks = "Bookmarks not available";

Like gcp said, these will need to be pulled out into strings so they can be localized.

We should also change these strings:

"Bookmarks not available for import"
"History not available for import"
"No bookmarks or history available"

Take a look at the string files at mobile/android/base/locales/en-US/android_strings.dtd and mobile/android/base/strings.xml.in and add these strings there. Follow the style of the naming, so they're something like import_toast_no_bookmarks, import_toast_no_history_bookmarks, etc.

After that, you can reference the strings with their ids, like R.string.import_toast_no_bookmarks.

@@ +73,5 @@
>          mImportBookmarks = doBookmarks;
>          mImportHistory = doHistory;
>      }
>  
> +    public void BookmarksHistoryCheck() {

Methods should start with a lowercase, but let's make this more specific - this is for showing a toast, so rename this something like showErrorToast().

Also, since we don't need member variables what you can do instead of checking member variables is just pass in two booleans here, so this method would be:

showErrorToast(boolean hasBookmarks, boolean hasHistory)

@@ +78,5 @@
> +        boolean importSuccess = !((mImportBookmarks && !mHasBookmarks) || (mImportHistory && !mHasHistory));
> +        if (importSuccess) {
> +            return;
> +        }
> +        String Toastmessage = "";

You'll be using string resources (like R.string.toast_message) so you can set a default resource id here (like -1).

Also, variables should start with a lower case.

@@ +89,5 @@
> +            } else if (!mHasHistory) {
> +                Toastmessage = mNoHistory;
> +            }
> +        } else if (mImportBookmarks) {
> +            if (!mHasBookmarks) {

You can combine these two if statements:

} else if (mImportBookmarks && !hasBookmarks) {

so you don't have an extra nested if.

@@ +93,5 @@
> +            if (!mHasBookmarks) {
> +                Toastmessage = mNoBookmarks;
> +            }
> +        } else if (mImportHistory) {
> +            if (!mHasHistory) {

Same here.

@@ +97,5 @@
> +            if (!mHasHistory) {
> +                Toastmessage = mNoHistory;
> +            }
> +        }
> +        Toast toast = Toast.makeText(mContext, Toastmessage, Toast.LENGTH_LONG);

Before you make the toast, check to make sure the toastResource is not equal to your default (-1).

@@ +109,5 @@
>                                 SAMSUNG_BOOKMARKS_URI,
>                                 LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 1");
>  
>              if (cursor != null) {
> +                mHasBookmarks = (cursor.getCount() != 0);

Save this value as a boolean, and return it at the end of the method.

@@ +157,5 @@
>                                  LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 0 AND " +
>                                  LegacyBrowserProvider.BookmarkColumns.VISITS + " > 0");
>  
>              if (cursor != null) {
> +                mHasHistory = (cursor.getCount() != 0);

Same.

@@ +214,4 @@
>      @Override
>      public void run() {
>          if (mImportBookmarks) {
>              mergeBookmarks();

As I mentioned earlier, this method can return a boolean that means whether or not there were any bookmarks found. You can save it here to pass into the method to show a toast message below.

@@ +218,5 @@
>          }
>          if (mImportHistory) {
>              mergeHistory();
>          }
> +        BookmarksHistoryCheck();

Now here you can pass in the two booleans returned from mergeHistory and mergeBookmarks to this method.

Comment 42

[varunnaganathan912]

Hi,I'm pretty much clear on everything you suggested except the way of declaration of string in the 
mobile/android/base/strings.xml.in file.
The string declarations have the name which is fine.But outside the <string> tag , there is an '&'.What I'm confused is that where is the actual string declared? And how does the '&' notation work.
Because in the android apps I have  developed, generally the string declarations were like 
"<string name="view_members">VIEW MEMBERS</string>"

Comment 43

[Chenxia Liu [:liuche]]

That's a good question. We have two types of string files, and you're correct in guessing that the string referenced by the & is where the string is declared.

Did you look at both files that I linked you to? The .dtd file is something custom that we have on Firefox so that we can use the same localization system that we use for Desktop Firefox. Basically, the strings are declared in the .dtd file, and the entity name is used in strings.xml.in by "referencing" it using the &. (The *.in suffix means the file is one that needs to be preprocessed, and the end result with be a strings.xml file, just like what you'd find in other Android apps.)

So you should add the actual strings in the android_strings.dtd file (which is a Mozilla-specific type of file that we send to localizers to localize), and then reference those names in the strings.xml.in.

For example, if we look at the first string in the android_strings.dtd file no_space_to_start_error, the string itself is associated with an "entity" (like a variable), and then that entity is referenced in the strings.xml.in file as &no_space_to_start_error. Just follow the format of the other strings used in these files.

Comment 44

[varunnaganathan912]

Attachment: Added a bookmarks and history check

Added the changes you mentioned.
Please review.
Thanks.

Comment 45

[Chenxia Liu [:liuche]]

Comment on attachment 8721742 [details] [diff] [review]
Added a bookmarks and history check

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

I appreciate the changes, sorry for taking a while to get back to you. I ran through this flow, and while using it, I realized that our user interface feedback (the dialog, notifying the user of success/failure) should be better, so let's work through that. I apologize for increasing the scope of this bug, but I'm happy to work with you on it, and it will 1) be good software engineering, and 2) end up with a better and higher quality user experience for this feature.

Basically, good code design separates frontend and backend, and we should do that with this code. After running this code, I think we should move the "warning" into the code for the dialog that's presented in AndroidImportPreference.

Basically, we should improve two things:
- Show the progress dialog for long enough to read
- Show a success message or an error message

So I've included some suggestions in line.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java
@@ +74,5 @@
> +        boolean importSuccess = !((mImportBookmarks && !hasBookmarks) || (mImportHistory && !hasHistory));
> +        if (importSuccess) {
> +            return;
> +        }
> +        String toastmessage = "";

This declaration isn't needed anymore since it's only being set once.

@@ +224,2 @@
>  
>          mOnDoneRunnable.run();

For good UI and code separation, we actually want the Runnable to handle the success or failure case, so instead of handling the error in showErrorToast, we should tell mOnDoneRunnable what the result was, and let it handle that. This import code is basically responsible for the "backend" and the AndroidImportPreference code is responsible for "frontend".

So the possible states are:
SUCCESS
FAIL_BOOKMARKS
FAIL_HISTORY
FAIL_BOOKMARKS_HISTORY

This should eventually look something like:

mOnDoneRunnable.run(result);

where the callback runnable has the responsibility for displaying a message based on which state is involved. (Each state corresponds to one of the strings you've already added in the strings section.)

What you'll need to do is the following:

1) Create an enum called ImportResult that consists of the 4 states mentioned before in this file.

2) Move the logic you currently have in showErrorToast to the run() method right here to determine which result to pass to the runnable.

3) Change the mOnDoneRunnable (which is created in AndroidImportPreference) to take this ImportState as an argument by making an interface (ImportResultRunnable) in AndroidImportPreference.java.

4) Change what stopRunnable does - update the text of the dialog to match the state (using the strings you've already added!) and delay the running by 1 second. 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java#89

Thanks again for your work so far!

Comment 46

[varunnaganathan912]

Hi,I'll be happy to implement the feature the way you suggested.
I have a few doubts.
Firstly,If I make the AndroidImport class to implement an alternate interface other than runnable(Which is a java.lang interface),Are you sure the code will not break?I mean won't there be any alternate files where the AnroidImport class would be used or expected to be as an implementation of Runnable interface.I mean the Runnable interface is a quite standard java interface.Are we sure we want to change it?
If you're confident about the code not breaking,then do you want me to make the AndroidImport class implement an alternate interface other than runnable?

Comment 47

[Chenxia Liu [:liuche]]

Good to have concerns about the usage of code! However, just because code is used elsewhere doesn't mean that it shouldn't be changed - this change will allow the callers of the class to handle the UI for success or failure (which is the real reason we need to separate this  change). When you make changes like this, you do need to check to see if this code is used elsewhere, and you can do that using using mxr.mozilla.org or dxr.mozilla.org , and you should also be able to verify this by building and running the code.

One thing I'd like to amend for comment #45 - ImportResultRunnable should actually be declared in AndroidImport.java, not AndroidImportPreference.

Comment 48

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

Unassigning due to inactivity. varunnaganathan912, please let us know if you'd like to continue to work on this.

Comment 49

[Andrew Adams]

I'd like to work on this however I'm new and not quite sure how to assign this to myself so it doesn't get taken. Could someone help me out?

Comment 50

[Gian-Carlo Pascutto [:gcp]]

(In reply to Andrew Adams from comment #49)
> I'd like to work on this however I'm new and not quite sure how to assign
> this to myself so it doesn't get taken. Could someone help me out?

You can just start working on it. Assigning doesn't really do anything. It won't stop anyone from providing a working patch even though someone else is assigned, and (unfortunately) won't automatically cause the person who was originally assigned to produce a working patch.

Not any more than the message you just posted, anyway :-)

Comment 51

[Dan Huang[:danhuang]]

Attachment: Bug1188271.patch

Hi Chenxia,

This patch is follow the description in Comment 45, I'm not sure if this patch completes all your suggestion. Please help review. Thanks.

Comment 52

[ameya]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #0)
> https://bugzilla.mozilla.org/show_bug.cgi?id=963661
> https://bugzilla.mozilla.org/show_bug.cgi?id=1186037#c3

Hi,

  I am new to Firefox development community and would like to start contributing for Firefox Android. I am interested in working on this user story. I read the comments and bugs related to this user story and need some clarifications regarding the same. I could not find import history/bookmark menu in Fennec build version 51.0a1. How do I import the History/Bookmarks ? Is the menu item removed from the menu ? 

Thanks!

Comment 53

[Gian-Carlo Pascutto [:gcp]]

I haven't worked on Fennec lately and I'm not sure what the plans are for this. Forwarding to liuche.

Comment 54

[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 55

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