Closed Bug 952310 Opened 6 years ago Closed 6 years ago

Create database schema for HomeProvider

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 2 open bugs)

Details

(Whiteboard: shovel-ready)

Attachments

(2 files, 1 obsolete file)

Currently database use is disabled in HomeListsContentProvider until we can come up with a v1 schema that we're happy with.

There's some discussion of schema in bug 941318, let's continue that discussion here.
Hardware: ARM → All
Assignee: nobody → margaret.leibovic
As part of this, we should make the current HomeListItems columns more generic. E.g., instead of "title" and "url", we should have "primary_text" and "secondary_text" (or something like that).
There's a few different moving parts here, but we can use this bug to make sure we're representing the same schema in JS and Java.
Depends on: 942288, 961092
Summary: Create database schema for HomeListsContentProvider → Create database schema for HomeProvider
Also building this on top of my patch for bug 949039, which should be ready to land soon. We'll want the fake items to match the same schema anyway.
Depends on: 949039
This is built on top of patches in the dependent bugs. If this is too annoying to review right now, you can also wait until those patches land (hopefully soon!).
Attachment #8364785 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8364785 [details] [diff] [review]
(Part 1) Rename HomeListsProvider -> HomeProvider

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

::: mobile/android/base/tests/robocop.ini
@@ +37,5 @@
>  skip-if = processor == "x86"
>  [testInputUrlBar]
>  [testJarReader]
>  [testLinkContextMenu]
> +# [testHomeListsProvider] # see bug 952310

I decided to punt on updating testHomeListsProvider for now, since I want to figure out how to test querying the content provider from Java, while populating the database from JS... but maybe that's too ambitious and we should just insert test data from Java as well. I can address this in a later patch in this bug.
This patch has some more renaming, but it's main objective is to get things into the same state everywhere.

Once again, this might be annoying to review before the dependent patches land :/
Attachment #8364788 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8364788 [details] [diff] [review]
(Part 2) Unify JS and Java representations of HomeProvider schema

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

::: mobile/android/base/db/HomeProvider.java
@@ +116,3 @@
>                      item.getString("url"),
> +                    item.getString("primary_text"),
> +                    item.getString("secondary_text")

I'm sorta coming back around to calling these title/description, to make it clear that one should be short and one can be long. Sorta matches how Android settings look...

Meh. I also hate getting distracted on naming.
(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 8364788 [details] [diff] [review]
> (Part 2) Unify JS and Java representations of HomeProvider schema
> 
> Review of attachment 8364788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/HomeProvider.java
> @@ +116,3 @@
> >                      item.getString("url"),
> > +                    item.getString("primary_text"),
> > +                    item.getString("secondary_text")
> 
> I'm sorta coming back around to calling these title/description, to make it
> clear that one should be short and one can be long. Sorta matches how
> Android settings look...
> 
> Meh. I also hate getting distracted on naming.

I don't think it's a distraction, since this is an API that will be exposed to developers.

Calling these things title/description to match Android conventions sounds like a reasonable proposal. I think we just need to make sure we don't call them title/url, like we currently do, since right now our UI always assumes list items correspond to pages. (However, we should still have a url field that corresponds to an action, just not a user-visible thing.)
This updates the schema to match what we talked about this morning.

I also verified locally that testHomeProvider still passes.
Attachment #8364788 - Attachment is obsolete: true
Attachment #8364788 - Flags: review?(lucasr.at.mozilla)
Attachment #8366129 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8364785 [details] [diff] [review]
(Part 1) Rename HomeListsProvider -> HomeProvider

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

Nice.
Attachment #8364785 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8366129 [details] [diff] [review]
(Part 2) Schema update

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

Cool.
Attachment #8366129 - Flags: review?(lucasr.at.mozilla) → review+
Blocks: 964464
https://hg.mozilla.org/mozilla-central/rev/f88683073c1e
https://hg.mozilla.org/mozilla-central/rev/629b52808ae1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 964926
Duplicate of this bug: 956954
You need to log in before you can comment on or make changes to this bug.