Closed Bug 776950 Opened 9 years ago Closed 9 years ago

Long tap on titlebar can't create homescreen shortcut if page has no title

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 17

People

(Reporter: wesj, Assigned: wesj)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
We should fall back to the url in this case?
Attachment #645338 - Flags: review?(margaret.leibovic)
Comment on attachment 645338 [details] [diff] [review]
Patch

Wht is tab.getTitle() returning null? I think a better fix would be to make sure that doesn't happen in Tab. This method and shareCurrentURI are the only consumers of tab.getTitle, and I think both of them would like to default to the URL as a title, so let's put the change in there.
Attachment #645338 - Attachment is patch: true
Attachment #645338 - Flags: review?(margaret.leibovic) → review-
I think we want getTitle to return null if there is none. That gives us some way to know that the tab doesn't have a title.

How about I just switch both to using tab.getDisplayTitle()?
Attached patch Patch v2Splinter Review
?
Assignee: nobody → wjohnston
Attachment #645338 - Attachment is obsolete: true
Attachment #645494 - Flags: review?(margaret.leibovic)
Comment on attachment 645494 [details] [diff] [review]
Patch v2

That sounds good, considering we already have a method for that :)

It looks like the only place getTitle() is used then is instead Tab.java itself:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#422

Bonus points if you write a patch that kills getTitle and replaces that call with mTitle. We don't need the misdirection of a getter method inside the class itself, and if that method isn't used anymore we should kill it. We can always add it back if we need it for something.
Attachment #645494 - Flags: review?(margaret.leibovic) → review+
Attached patch Patch 2/2 (obsolete) — Splinter Review
Good idea. Cleans up the tabs stuff.
Attachment #645585 - Flags: review?(margaret.leibovic)
(In reply to Wesley Johnston (:wesj) from comment #5)
> Created attachment 645585 [details] [diff] [review]
> Patch 2/2
> 
> Good idea. Cleans up the tabs stuff.

This patch is empty...
Attached patch Patch 2/2 v2.Splinter Review
Grr.
Attachment #645585 - Attachment is obsolete: true
Attachment #645585 - Flags: review?(margaret.leibovic)
Attachment #645592 - Flags: review?(margaret.leibovic)
Attachment #645592 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/1dd57c1c57cf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Build ID: 17.0 (2012-10-08) Aurora Channel
          18.0 (2012-10-08) Nightly Channel
          19.0 (2012-10-09) Nightly Channel
Device: Samsung Galaxy Nexus
OS: Android 4.1

Tried with about:home and the page is added to home screen. 
Marking bug as Verify Fixed.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.