Closed Bug 1325380 Opened 8 years ago Closed 8 years ago

Use 'provider name' metadata for AS panel

Categories

(Firefox for Android Graveyard :: General, defect, P1)

All
Android
defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

(Whiteboard: [MobileAS])

Attachments

(1 file)

Bug 1311434 introduced the rule for collecting the "provider name" from the website's metadata. Now let's use it for the AS panel.
Comment on attachment 8821188 [details] Bug 1325380 - Use provider name for AS highlights. https://reviewboard.mozilla.org/r/100530/#review101110 Looks good. I've added some thoughts on how we might evolve this pattern over time, which might be a good follow-up. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1201 (Diff revision 1) > + DBUtils.qualifyColumn(PageMetadata.TABLE_NAME, PageMetadata.JSON) + " AS " + Highlights.METADATA + " " + > "FROM " + Bookmarks.TABLE_NAME + " " + > "LEFT JOIN " + History.TABLE_NAME + " ON " + > DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.URL) + " = " + > DBUtils.qualifyColumn(History.TABLE_NAME, History.URL) + " " + > + "LEFT JOIN " + PageMetadata.TABLE_NAME + " ON " + It might be worth noting in a comment that the way we currently insert page metadata is via INSERT OR REPLACE, ensuring that it's a 1x1 relationship between history and pagemetadata. That is why a simple left join without a group by works here. ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamItem.java:292 (Diff revision 1) > + if (metadata != null && metadata.has("provider")) { > + vPageView.setText(metadata.getString("provider")); > + return; > + } > + } catch (JSONException e) { > + // Broken JSON? Continue with fallback. I wonder how clumsy dealing with json metadata will get once we start doing more of this. It might be helpful to think of this in terms of a model-view relationship, where we have a "highlight model", which is instantiated with various highlight data from the cursor, and, for example, provides a "displayName" method which encapsulates various ways to obtain a suitable display name: try provider, fallback to X, fallback to Y... We'll be doing this sort of a thing for highlight images very soon, and I would imagine for more and more bits of data as time goes on. This will be nicely testable and should help keep view logic free from data stuff.
Attachment #8821188 - Flags: review?(gkruglov) → review+
Yeah, I agree. In addition to that the whole StreamItem file gets more and more convoluted. It's time to break it up - I'll look at this in a follow-up bug.
Pushed by s.kaspari@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8702f15c7d2e Use provider name for AS highlights. r=Grisha
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: