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)
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 hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 3•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8702f15c7d2e
Use provider name for AS highlights. r=Grisha
Comment 6•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•