Closed Bug 952582 Opened 6 years ago Closed 6 years ago

Bookmark item is sole item in row of menu of guest-browsing

Categories

(Firefox for Android :: Theme and Visual Design, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified
fennec 29+ ---

People

(Reporter: aaronmt, Assigned: wesj)

References

Details

(Keywords: uiwanted)

Attachments

(3 files)

01:58  AaronMT:	sriram: ibarlow: lonely bookmark button http://cl.ly/image/0t0e3f1g3F17
01:58  sriram:	 yup
[...]
01:58  ibarlow:	 you're in guest browsing
01:58  ibarlow:	 different bug

Not sure if something is to be done here or not, but filing because Ian recognized it as a bug.
tracking-fennec: --- → ?
Keywords: uiwanted
guests probably shouldn't have bookmarks. Ian, what do you want to do?
Assignee: nobody → ibarlow
Now that we've made that whole bar about "This is how you save or share things", the most sensible thing to do would probably be to turn off the whole thing -- sharing *and* bookmarking (I feel like that may have even been part of some early design sketches for Guest Browsing). 

So let's remove that row altogether.
Assignee: ibarlow → wjohnston
tracking-fennec: ? → 29+
Attached image tap-target.png
It's also half tap-target for bookmark ....
Attached patch PatchSplinter Review
This removes bookmark from the main menu and context menus when in guest mode.
Attachment #8377756 - Flags: review?(michael.l.comella)
Comment on attachment 8377756 [details] [diff] [review]
Patch

Review of attachment 8377756 [details] [diff] [review]:
-----------------------------------------------------------------

Can addons still be used in guest mode? If so, does the bookmarking functionality need to be disabled there?

::: mobile/android/base/BrowserApp.java
@@ +2145,5 @@
>              return true;
>          }
>  
>          bookmark.setEnabled(!AboutPages.isAboutReader(tab.getURL()));
> +        bookmark.setVisible(!GeckoProfile.get(this).inGuestMode());

It seems there's a precedent for `mProfile.inGuestMode()` [1]. Any reason not to use that?

I see your method has already been used (in the same function, no less!). We might as well change that to `mProfile.inGuestMode` while we're at it.
Attachment #8377756 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #5)
> Comment on attachment 8377756 [details] [diff] [review]
> Patch
> 
> Review of attachment 8377756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can addons still be used in guest mode? If so, does the bookmarking
> functionality need to be disabled there?

Add-ons can't be used in Guest Browsing. We create a new, temporary, profile. The profile has no add-ons installed into it. Add-ons from the main profile are not copied into the Guest profile.
(In reply to Michael Comella (:mcomella) from comment #5)
> It seems there's a precedent for `mProfile.inGuestMode()` [1]. Any reason
> not to use that?

That reference to the profile should probably not exist. i.e. The activity lifecycle doesn't match the Gecko lifecycle, hence the Activity shouldn't hold a reference to the Gecko profile dir either. We've tried (unsuccessfully) to disentangle this at times.
(In reply to Wesley Johnston (:wesj) from comment #7)
> That reference to the profile should probably not exist.

Should we file a followup to remove the reference then?
I filed bug 902060 about that awhile ago.
https://hg.mozilla.org/mozilla-central/rev/dcdf818049ee
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Do we want to uplift this to aurora?
Flags: needinfo?(wjohnston)
Comment on attachment 8377756 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Always been. The new quickshare made it more obvious (because this exposes a bug)
User impact if declined: Confusing single star menuitem in the UI with no label.
Testing completed (on m-c, etc.): Landed a few weeks ago. 
Risk to taking this patch (and alternatives if risky): Pretty low risk. We had this code in place before. We're just adding bookmarks to it.
String or IDL/UUID changes made by this patch: None.
Attachment #8377756 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wjohnston)
Attachment #8377756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- 29.0a2 (2014-03-06);
- 30.0a2 (2014-03-06);
Tested on Lenovo Yoga Tab 10 (Android 4.2.2).
Status: RESOLVED → VERIFIED
Depends on: 1030770
You need to log in before you can comment on or make changes to this bug.