If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

(photon) Frequency search result visual refresh

VERIFIED FIXED in Firefox 56

Status

()

Firefox for Android
General
VERIFIED FIXED
4 months ago
4 days ago

People

(Reporter: wesley_huang, Assigned: jwu)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

3 months ago
Assignee: nobody → topwu.tw
Blocks: 1379652
Comment hidden (mozreview-request)

Comment 2

2 months ago
mozreview-review
Comment on attachment 8885594 [details]
Bug 1366678 - Highlight substrings in search suggestion/history if they match search term.

https://reviewboard.mozilla.org/r/156414/#review161902

::: mobile/android/app/src/photon/java/org/mozilla/gecko/home/SearchEngineRow.java:220
(Diff revision 1)
> -            int nextStartSpanIndex = 0;
> -            // Done to make sure that the stretch of text after the last occurrence, till the end of the suggestion, is made bold
> -            mOccurrences.add(suggestion.length());
>              for (int occurrence : mOccurrences) {
>                  // Even though they're the same style, SpannableStringBuilder will interpret there as being only one Span present if we re-use a StyleSpan
> -                StyleSpan boldSpan = new StyleSpan(Typeface.BOLD);
> +                final StyleSpan boldSpan = new StyleSpan(Typeface.BOLD);

just curious. Is it possible move this out of for-loop?

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:62
(Diff revision 1)
>      private String mPageUrl;
>  
>      private boolean mHasReaderCacheItem;
>  
> +    // Use this interface to decorate content in title view.
> +    interface TitleFormatter {

I think move the interface declaration to the bottom of file would be better.

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:63
(Diff revision 1)
>  
>      private boolean mHasReaderCacheItem;
>  
> +    // Use this interface to decorate content in title view.
> +    interface TitleFormatter {
> +        @Nullable Spannable format(@NonNull CharSequence title);

what if we return the original CharSequence if nothing be formatted, instead of returning null? This behavior is more like *decorator*
Attachment #8885594 - Flags: review?(walkingice0204) → review+
(Assignee)

Comment 3

2 months ago
mozreview-review-reply
Comment on attachment 8885594 [details]
Bug 1366678 - Highlight substrings in search suggestion/history if they match search term.

https://reviewboard.mozilla.org/r/156414/#review161902

> just curious. Is it possible move this out of for-loop?

SpannableStringBuilder uses a IdentityHashMap to keep the relationship between the span and its range(start and end position). The key value of the map is the span's hashCode().

If we re-use the span, the map would always return same range that cause only one match can be styled.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 5

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/967111e5be01
Highlight substrings in search suggestion/history if they match search term. r=walkingice
Keywords: checkin-needed

Comment 6

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/967111e5be01
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 7

2 months ago
Verified as fixed on Nightly 56.0a1, (2017-08-10).
Devices: 
Huawei Honor 8 (Android 6.0)
Motorola Nexus 6 (Android 7.0)
Prestigio Grace X5 (Android 4.4.2)
Huawei Honor 8 (Android 6.0)
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
QA Contact: oana.horvath
Depends on: 1401869
You need to log in before you can comment on or make changes to this bug.