926234 - Eliminate combined_with_favicons view

Status: RESOLVED INCOMPLETE

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

Description

[Richard Newman [:rnewman]]

It's no longer necessary, and we should in any case discourage its use!

Comment 1

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

We have a taker - thanks!

Comment 2

[Richard Newman [:rnewman]]

nivvedan came to find me on IRC to ask for more details.

We no longer need combined_with_favicons, because we now asynchronously load favicons when needed, so we simply don't need this view any longer. Last I checked it had no consumers.

We should discourage its use because it's inefficient.

Comment 3

[nivvedan]

How about the history_with_favicons and the the bookmarks_with_favicons views?
Should they be eliminated too? (In general, even if not in this patch)

Comment 4

[nivvedan]

Attachment: v1

Comment 5

[Chris Kitching [:ckitching]]

(In reply to nivvedan from comment #3)
> How about the history_with_favicons and the the bookmarks_with_favicons
> views?
> Should they be eliminated too? (In general, even if not in this patch)

They're sort of sensible, but combined_with_Favicons has dreadful performance, requiring evil table scans to do anything (as it builds evilly atop the already-evil combined view).

Plus, those two are definitely still used (at least, as of a couple of weeks ago).

(In reply to Richard Newman [:rnewman] from comment #2)
> nivvedan came to find me on IRC to ask for more details.
> 
> We no longer need combined_with_favicons, because we now asynchronously load
> favicons when needed, so we simply don't need this view any longer. Last I
> checked it had no consumers.

You sure? I can't check conveniently now because I'm on a phone but I thought there was some crazy thing sriram wrote a while ago to do with search suggestions that used it. (that's why I didn't already resolve this bug).

If that's gone away... woot! Kill the thing.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #5)

> thought there was some crazy thing sriram wrote a while ago to do with
> search suggestions that used it. (that's why I didn't already resolve this
> bug).

You're right; it's still used by SearchManager (and SearchManager alone).

We should see if that integration point is still used by recent Android versions. 

(mfinkle, do you know how this stuff works? I can't even find a way on KitKat of triggering search suggestions!)

Even if it is still used, we should decide whether we want to do something smarter and more efficient here, or just throw away the complexity altogether.

I'm inclined to assert that we care very little about returning icons for these suggestions, so we should just rip this view out and return NULL for that column.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8535965 [details] [diff] [review]
v1

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

Assuming that we really are killing this view, you'll also need to:

* Add a migration to remove the view.
* Do the right thing when a caller includes the favicon column in the projection.

::: mobile/android/base/db/BrowserProvider.java
@@ -760,5 @@
>  
>                  qb.setProjectionMap(COMBINED_PROJECTION_MAP);
>  
> -                if (hasFaviconsInProjection(projection))
> -                    qb.setTables(VIEW_COMBINED_WITH_FAVICONS);

Removing this will cause an error something like "no such column: favicon". This will need a little bit of finesse to ensure that we return a NULL value for the icon column.

@@ +780,5 @@
>                  if (TextUtils.isEmpty(sortOrder))
>                      sortOrder = DEFAULT_HISTORY_SORT_ORDER;
>  
>                  qb.setProjectionMap(SEARCH_SUGGEST_PROJECTION_MAP);
> +                qb.setTables(Combined.VIEW_NAME);

setProjectionMap:
"If a projection map is set it must contain all column names the user may request, even if the key and value are the same."

And S_S_P_M:

        map = new HashMap<String, String>();
        map.put(SearchManager.SUGGEST_COLUMN_TEXT_1,
                Combined.TITLE + " AS " + SearchManager.SUGGEST_COLUMN_TEXT_1);
        map.put(SearchManager.SUGGEST_COLUMN_TEXT_2_URL,
                Combined.URL + " AS " + SearchManager.SUGGEST_COLUMN_TEXT_2_URL);
        map.put(SearchManager.SUGGEST_COLUMN_INTENT_DATA,
                Combined.URL + " AS " + SearchManager.SUGGEST_COLUMN_INTENT_DATA);
        SEARCH_SUGGEST_PROJECTION_MAP = Collections.unmodifiableMap(map);


That implies that we could make this change right now with no ill-effects, or the docs are lying.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #6)
> (In reply to Chris Kitching [:ckitching] from comment #5)
> 
> > thought there was some crazy thing sriram wrote a while ago to do with
> > search suggestions that used it. (that's why I didn't already resolve this
> > bug).
> 
> You're right; it's still used by SearchManager (and SearchManager alone).
> 
> We should see if that integration point is still used by recent Android
> versions. 
> 
> (mfinkle, do you know how this stuff works? I can't even find a way on
> KitKat of triggering search suggestions!)

We discovered this in bug 966491

> Even if it is still used, we should decide whether we want to do something
> smarter and more efficient here, or just throw away the complexity
> altogether.

I think we should file a bug to remove the code that was used to support the feature. It would be nice to have telemetry on how much it's used before removing, but we can get the process started. File a bug?

> I'm inclined to assert that we care very little about returning icons for
> these suggestions, so we should just rip this view out and return NULL for
> that column.

Testing with an Android 4.3 phone I have, the history displayed in the Google Search app from Firefox/Beta/Aurora/Nightly does not include the favicon of the page, only the icon of the application (Firefox). I think we could safely remove the favicon from the results in the short term with no ill effects.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #8)

> I think we should file a bug to remove the code that was used to support the
> feature. It would be nice to have telemetry on how much it's used before
> removing, but we can get the process started. File a bug?

Filed Bug 1111221 to get telemetry, and Bug 1111220 to remove SearchManager. Those block this bug.

nivvedan, do you want to try tackling those before coming back to this bug?

Comment 10

[nivvedan]

 
> nivvedan, do you want to try tackling those before coming back to this bug?

Sure.

Comment 11

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

Unassigning due to inactivity. Let me know if you're still interested in working on this!

Comment 12

[Sheefeni Hauwanga]

I'd like to try my hand at this bug.

Comment 13

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

Sure - I've assigned you, please let us know if you have any questions!

Comment 14

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

Reset assignee due to inactivity.

Comment 15

[Tushar Saini (:shatur)]

Attachment: v1.patch

Hey!
   I would like to work on this. Based on previous comment, made some changes.
 Please review and let me know if any further modification is needed.

Thank You!

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8821728 [details] [diff] [review]
v1.patch

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

You can't just alter old DB migrations; you're changing the database schema, so you need to define a new schema version, add a migration to it that drops the view if it already exists, and alter the creation code for a new DB to not create the view.

You need to make sure that older versions of Firefox successfully update to the new schema, dropping the view, and that new installs create the DB without the view.

You should also double-check that there are no current callers that request this view.

Please send the updated review request to :ahunt, who I think has been working on cleaning up some of this code lately.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
@@ -1982,4 @@
>  
>      private void createV33CombinedView(final SQLiteDatabase db) {
>          db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED);
> -        db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED_WITH_FAVICONS);

You'll still need to keep these around to make sure that the view is discarded at the appropriate time.

Comment 17

[Richard Newman [:rnewman]]

Tushar: you should also read my review comment in Comment 7 about "no such column: favicon".

Comment 18

[Tushar Saini (:shatur)]

Attachment: raw.patch

Hey!
     Until ahunt is on vacation, can you please provide feedback on this raw patch. This does not resolve all issues, but it migrates to new schema where combine_with_favicon view is dropped.
    Am I in right direction??

Thanks!!

Comment 19

[Richard Newman [:rnewman]]

I'm also on vacation!

Comment 20

[Tushar Saini (:shatur)]

Hey! 

(In reply to Richard Newman [:rnewman] from comment #19)
> I'm also on vacation!

Sorry to bother you in your vacation. 

Still needed some help. I am not able to understand how can I handle "no such column: favicon" referred in Comment 7. Please, can you elaborate a little more?. It will be really helpful.

Regards.

Comment 21

[Richard Newman [:rnewman]]

(In reply to Tushar Saini (:shatur) from comment #20)

> Still needed some help. I am not able to understand how can I handle "no
> such column: favicon" referred in Comment 7. Please, can you elaborate a
> little more?. It will be really helpful.

I'm sorry, I don't have the time available to walk you through the amount of SQL knowledge available to do this. If :ahunt is available, I hope he can help!

Comment 22

[Andrzej Hunt :ahunt]

(In reply to Tushar Saini (:shatur) from comment #20)
> Hey! 
> 
> (In reply to Richard Newman [:rnewman] from comment #19)
> > I'm also on vacation!
> 
> Sorry to bother you in your vacation. 
> 
> Still needed some help. I am not able to understand how can I handle "no
> such column: favicon" referred in Comment 7. Please, can you elaborate a
> little more?. It will be really helpful.
> 
> Regards.

As far as I can tell, the issue is that the projection could still contain the Favicons column (i.e. we could have a query which requests the favicons column), in which case the caller would expect the favicons columns to be present/could query them. VIEW_COMBINED doesn't provide any favicon columns, so the client code would crash when trying to access those columns - instead we could just provide "fake" data, i.e. use NULL in a generated column when querying against VIEW_COMBINED.

However I'm not too sure why we'd still need to provide a favicons column at all - as far as I can tell all of our queries on the Combined view don't request any of those columns, so we should be able to just remove them - I'm still trying to figure out if there's any other code that would somehow end up requesting favicons that I'm not aware of.
 
Note: we still access Combined.FAVICON* in testBrowserProvider.java, that test will need to be updated to reflect any changes we make. E.g. getFaviconsByUrl() queries the combined view in order to access favicons, that will need to be modified to access favicons via history/bookmarks instead.

Comment 23

[Tushar Saini (:shatur)]

Attachment: Bug 926234 - Eliminate combined_with_favicons view.

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

Comment 24

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

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