Closed Bug 912523 Opened 11 years ago Closed 11 years ago

Give bookmark thumbnails a context menu header


(Firefox for Android Graveyard :: Awesomescreen, defect)

Not set


(Not tracked)

Firefox 26


(Reporter: pretzer, Assigned: capella)



(Whiteboard: [mentor=sriram][lang=java])


(4 files)

The six bookmark thumbnails on about:home deserve a header for their context menu showing the title/URL of the bookmark, just like all the normal bookmarks below.
Sriram, want to make this into a mentor bug?
Flags: needinfo?(sriram)
I can fix it right away. But happy to mentor :)
Flags: needinfo?(sriram)
(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> I can fix it right away. But happy to mentor :)

Let's make this a mentor bug, but you can fix it if you run out of things to do and nobody else fixes it first :)
Whiteboard: [mentor=sriram][lang=java]
Attached patch bug912523Splinter Review
I'll snag this small one while you guys are working on polishing the big about:home finishing touches :D
Assignee: nobody → markcapella
Attachment #799955 - Flags: review?(sriram)
Comment on attachment 799955 [details] [diff] [review]

Review of attachment 799955 [details] [diff] [review]:

++ r ++

::: mobile/android/base/home/
@@ +286,5 @@
>          MenuInflater inflater = new MenuInflater(view.getContext());
>          inflater.inflate(, menu);
>          TopBookmarksContextMenuInfo info = (TopBookmarksContextMenuInfo) menuInfo;

No new line needed before this.
Attachment #799955 - Flags: review?(sriram) → review+
Comment on attachment 799955 [details] [diff] [review]

Review of attachment 799955 [details] [diff] [review]:

::: mobile/android/base/home/
@@ +243,5 @@
>          }
> +
> +        public String getDisplayTitle() {
> +            return TextUtils.isEmpty(title) ? url.replaceAll(REGEX_URL_TO_TITLE, "") : title;
> +        }

Drive by. This will be the third implementation of stripping the scheme/subdomain from urls that we have in our Java code. Please use the ones in StringUtils (and if you want, combine those two so that there's only one implementation of this).
Attached patch bug912523 v2Splinter Review
Something like this might be better?
Attachment #800020 - Flags: review?(sriram)
Attached patch bug912523 v3Splinter Review
Or we could go further like this?

I wasn't sure by wesj comments if he thought we could combine both stripScheme() and stripCommonSubdomains() into one method (versus the two) in ... there's a unique call to stripCommonSubdomains() here:
Stripping common subdomains for bookmarks is not what we want I think. There are places where it makes sense (like in the addressbar or reader mode) because either space is very limited or it doesn't add any useful information in that particular context. Both is not the case here though, in my eyes. Therefore calling stripScheme() should be sufficient here. 

Offering a combined method for consumers that do both anyways does probably make sense. What I *think* Wes meant though was that there are still other places in the code that implement their own stripping mechanism and you could convert them to the StringUtils methods while your at it if you want. 

Just my two cents!
Actually after thinking about it again, it really doesn't matter much if the common subdomains get stripped away here. Patch v3 is therefore probably the way to go, I think. Sorry if I caused any confusion.
Comment on attachment 800020 [details] [diff] [review]
bug912523 v2

Review of attachment 800020 [details] [diff] [review]:

v1 is better to me.
Attachment #800020 - Flags: review?(sriram) → review-
shipping v1 ... planning a followup patch re: sriram and wesj via irc
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.