Closed Bug 1166385 Opened 9 years ago Closed 3 years ago

Make it possible to enable/disable quick share menu

Categories

(Firefox for Android Graveyard :: General, defect, P5)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: Mook, Unassigned, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(4 files, 4 obsolete files)

Added lock in ActivityChooserModel.removeAllHistoricalRecords
15.59 KB, patch
mcomella
: feedback+
Details | Diff | Splinter Review
Screenshot of quick share option in browser settings
112.83 KB, image/png
mcomella
: review+
Details
Quick share preference
4.24 KB, patch
Details | Diff | Splinter Review
Update the menu when quick share preference is changed.
13.41 KB, patch
Details | Diff | Splinter Review 
There is no obvious way to remove the quick share entry once you share something.  Sharing something else to bump it off isn't an option because I'd like the menu item gone, it's not that I particularly object to the sharing method listed.

Fennec 38 (beta).

See also https://support.mozilla.org/en-US/questions/1044116
(In reply to :Mook from comment #0)
> Sharing something else to bump it off isn't an option because
> I'd like the menu item gone

Why would you like the menu item gone?

If it's for space considerations as per the posted SUMO link, bug 1140048 will move the quick share menu back onto the same row as the "Share" button, potentially alleviating the waste of space of quick share (though it will still create and use that third header row at the top of the menu). However, we're waiting for our other platforms to implement certain workflows so it's unclear when that change will land.
Flags: needinfo?(mook.moz+mozbz)
(In reply to Michael Comella (:mcomella) from comment #1)
> Why would you like the menu item gone?


UI clutter; it's the only colored icon in the main menu, which means it draws my eye. That's annoying for an item I don't use often. Further, it introduces the chance of mis-clicks (again for something I don't use often).

Basically it introduces a (very slightly) elevation of alertness when I open the menu.
Flags: needinfo?(mook.moz+mozbz)
I'm not very convinced by the argument that "I don't use this feature often" -- you can make that argument, supported by statistics, about just about every piece of the UI.

Mobile app UIs are not particularly customizable because it's hard to do right and painful to implement, and I doubt this is a big enough win to justify spending precious time.

I can see a privacy/control angle -- e.g., you want to hide the fact that you frequently share to a particular app, or hide which apps you share to altogether -- that we should consider, but unless we hear a lot more negative feedback about QuickShare (and this is the first I've heard), I don't consider this bug to be high priority.
I agree from a pure privacy standpoint, we should probably keep this around. Clear private data is leaving data around that we could easily clear. Nick said he had a volunteer who might take this.

The code to get these (yes there are potentially multiple) filenames is here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java#59

I think I'd recommend a public static method like GeckoActionProvider.clearData() that would iterate through files with the correct name type in the directory and wipe them. There will probably be more work to update the menu as well (since it may cache some data?)... If someone wants to work on this, be prepared to do a little (super awesome) digging into some confusing code :)
(In reply to Wesley Johnston (:wesj) from comment #4)
> There will probably be more work to update the menu
> as well (since it may cache some data?)...

If you change ActivityChooserModel.mReloadActivities, it should *just work*. I believe the ActivityChooserModel instance can be pulled from the GeckoActionProvider associated with the menu.
Whiteboard: [lang=java]
via irc.
Assignee: nobody → dnirm.moz
Anthony, what's a good UX here? Will a long-press context menu work here?
Flags: needinfo?(alam)
Yeah, we can try popping up a simple dialog on long press. But it would really only show a "Hide" item which seems a bit unnecessary. 

Is there perhaps a way to pick another (more preferred item/app icon) to be placed there, from this context menu?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
(In reply to Anthony Lam (:antlam) from comment #8)
> Is there perhaps a way to pick another (more preferred item/app icon) to be
> placed there, from this context menu?

Currently, the only way to do that is to use the item more frequently, which is not intuitive or easy. NI antlam for further ideas.
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
Why not just add a Setting to control the Quick Share system? It could be a simple:
[ ] Quick share: Remember my choices when sharing to other applications
(In reply to Mark Finkle (:mfinkle) from comment #10)
> Why not just add a Setting to control the Quick Share system? It could be a
> simple:
> [ ] Quick share: Remember my choices when sharing to other applications

Unchecking this setting would also clear any existing cache.
I'm still going through the code and have few questions. Can you please help me in this -

1. What's the purpose of having two share references as declared in BrowserApp? Both of them set action provider at the same time. Is "share" the generic share button and "quickshare" for specific sharing options (like what's app)?

2. I see that the quick share icons/buttons are added here - 
            view.addActionButton(dataModel.getActivity(i).loadIcon(packageManager), 
                                 dataModel.getActivity(i).loadLabel(packageManager));
  
   I'm bit confused if the fix is to clear/replace these buttons only or remove them from history file (from Comment #4)?
Flags: needinfo?(michael.l.comella)
Without specific line references, it's hard to answer some of these questions, but I'll try. In the future, you can use mxr to link to code references, e.g. https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#158

(In reply to Dipti Nirmale [:dnirm] from comment #12)
> 1. What's the purpose of having two share references as declared in
> BrowserApp? Both of them set action provider at the same time. Is "share"
> the generic share button and "quickshare" for specific sharing options (like
> what's app)?

The quick share UI widget includes both the quick share items AND the share button. However, in a newer version, we decided to split up these items to two separate rows. A hack I made was to use two of these widgets, one to show the share button and one to show the quick share items, which is probably what you're seeing.

> 2. I see that the quick share icons/buttons are added here - 
>    I'm bit confused if the fix is to clear/replace these buttons only or
> remove them from history file (from Comment #4)?

If you don't remove them from the file, they'll be restored the next time the cached photos are used - you should remove them from the history file - note that I'm not sure how complicated this could be.
Flags: needinfo?(michael.l.comella)
(In reply to Mark Finkle (:mfinkle) from comment #11)
> > Why not just add a Setting to control the Quick Share system? It could be a
> > simple:
> > [ ] Quick share: Remember my choices when sharing to other applications
> 
> Unchecking this setting would also clear any existing cache.

This is an interesting way of looking at this feature though does anyone really want to turn this feature off? They probably just want to clear the list, or remove an item. I dislike the way this could split the number of "configurations" of the browser but it could be a simpler solution than adding the UI to remove a single item.
(In reply to Michael Comella (:mcomella) from comment #13)
> Without specific line references, it's hard to answer some of these
> questions, but I'll try. In the future, you can use mxr to link to code
> references, e.g.
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> BrowserApp.java#158

Thank you Michael. Going forward I'll ensure that.

> The quick share UI widget includes both the quick share items AND the share
> button. However, in a newer version, we decided to split up these items to
> two separate rows. A hack I made was to use two of these widgets, one to
> show the share button and one to show the quick share items, which is
> probably what you're seeing.

> If you don't remove them from the file, they'll be restored the next time
> the cached photos are used - you should remove them from the history file -
> note that I'm not sure how complicated this could be.

This makes sense.
(In reply to Michael Comella (:mcomella) from comment #14)
> (In reply to Mark Finkle (:mfinkle) from comment #11)
> > > Why not just add a Setting to control the Quick Share system? It could be a
> > > simple:
> > > [ ] Quick share: Remember my choices when sharing to other applications
> > 
> > Unchecking this setting would also clear any existing cache.
> 
> This is an interesting way of looking at this feature though does anyone
> really want to turn this feature off? They probably just want to clear the
> list, or remove an item. I dislike the way this could split the number of
> "configurations" of the browser but it could be a simpler solution than
> adding the UI to remove a single item.

Seems like the OP (:Mook) wanted to remove the items in comment 0 because they were being displayed. The SUMO post describes wanting to disable the Quick Share feature. Comment 3 and comment 4 talk about the privacy aspect of saving and displaying the recent share apps.

It seems like a simple "enable/disable" setting is better than trying to make a UI for removing individual share items.
(In reply to Mark Finkle (:mfinkle) from comment #16)
> It seems like a simple "enable/disable" setting is better than trying to
> make a UI for removing individual share items.

Okay, I agree.

Dipti, let me know if you have questions. :)
iirc, you might be able to just hide, remove, or never add the quick share header view.
Summary: Make it possible to remove quick share entry → Make it possible to enable/disable quick share menu
(In reply to Michael Comella (:mcomella) from comment #17)
> (In reply to Mark Finkle (:mfinkle) from comment #16)
> > It seems like a simple "enable/disable" setting is better than trying to
> > make a UI for removing individual share items.
> 
> Okay, I agree.
> 
> Dipti, let me know if you have questions. :)

Where should this setting for enable/disable be? I think it should be besides share option if it is to be noticed and used often by normal user. I'm still not clear on how it should look like.
Flags: needinfo?(michael.l.comella)
(In reply to Dipti Nirmale [:dnirm] from comment #19)
> Where should this setting for enable/disable be?

My initial thought is Settings -> Display, perhaps under "Full-screen browsing", e.g. "Allow toolbar menu quick share", or similar.

> I think it should be
> besides share option if it is to be noticed and used often by normal user.

By besides the share option, do you mean directly in the menu toolbar? Do you have a mock-up you can show me? The way I'm seeing it, I'm afraid that'll add too much to the menu for a lesser used option (and I imagine this isn't something a normal user would use often - it's likely more a privacy thing for advanced users).
Flags: needinfo?(michael.l.comella) → needinfo?(dnirm.moz)
(In reply to Michael Comella (:mcomella) from comment #20)
> (In reply to Dipti Nirmale [:dnirm] from comment #19)
> > Where should this setting for enable/disable be?
> 
> My initial thought is Settings -> Display, perhaps under "Full-screen
> browsing", e.g. "Allow toolbar menu quick share", or similar.

If this bug is mainly about protecting privacy, I would suggest considering the Privacy section. Honestly, our Settings are in grave need of a cleanup, so maybe it's not a big deal where it goes for now.
(In reply to Michael Comella (:mcomella) from comment #20)
> My initial thought is Settings -> Display, perhaps under "Full-screen
> browsing", e.g. "Allow toolbar menu quick share", or similar.
> 
> By besides the share option, do you mean directly in the menu toolbar? Do
> you have a mock-up you can show me? The way I'm seeing it, I'm afraid
> that'll add too much to the menu for a lesser used option (and I imagine
> this isn't something a normal user would use often - it's likely more a
> privacy thing for advanced users).

Yes, I meant adding directly in the menu toolbar but as you said, I agree that it'll add too much. Adding it under Settings would be better. But both options Display as well as Privacy (from Mark in comment #21) look good to me. Should I try adding in Privacy for now, in case it can be changed later?
Flags: needinfo?(dnirm.moz) → needinfo?(michael.l.comella)
(In reply to Dipti Nirmale [:dnirm] from comment #22)
> Should I try adding in Privacy for now, in case it can be changed later?

Sure, let's do that.
Flags: needinfo?(michael.l.comella)
I think the quick share should be enabled by default. The work is in progress, but need some input on the existing patch.
Attachment #8645539 - Flags: review?(michael.l.comella)
In retrospect, I'm trying to understand how this bug addresses the privacy concern (I agree with the UI clutter reasoning). There is generally only one user for each android device and the same user would use the option often. Does firefox or any other app use this data? Could you please help me understand this?
Flags: needinfo?(michael.l.comella)

        Attachment #8645539 -
        Attachment is obsolete: true

        Attachment #8645539 -
        Flags: review?(michael.l.comella)

        Attachment #8647106 -
        Flags: review?(michael.l.comella)

    
    

    
  
Sorry for the delayed response.

(In reply to Dipti Nirmale [:dnirm] from comment #25)
> In retrospect, I'm trying to understand how this bug addresses the privacy
> concern (I agree with the UI clutter reasoning). There is generally only one
> user for each android device and the same user would use the option often.

I think the privacy use case extends to sharing devices – e.g. why private browsing exists. If you're the only user on your device, someone can still borrow your device and look at your history.

> Does firefox or any other app use this data?

No other app should have access to this data, and we don't do anything special with it beyond quick share, afaik.

Let me know if that didn't answer your question.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Comment on attachment 8647106 [details] [diff] [review]
Added preference in settings to enable/disable quick share option

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

(Didn't get a chance to finish – I'll take a look again tomorrow)

::: mobile/android/base/BrowserApp.java
@@ +3309,5 @@
>  
>              // This provider also applies to the quick share menu item.
>              final GeckoActionProvider provider = ((GeckoMenuItem) share).getGeckoActionProvider();
> +
> +            ((GeckoMenuItem)quickShare).setActionProvider(provider);

This is done in BrowserApp.onCreateOptionsMenu - why did you add this here as well?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +198,5 @@
>  <!ENTITY pref_tracking_protection_summary2 "Actively block tracking elements in Private Browsing">
>  <!ENTITY pref_donottrack_title "Do not track">
>  <!ENTITY pref_donottrack_summary "&brandShortName; will tell sites that you do not want to be tracked">
> +<!ENTITY pref_quickshare_title "Quick share">
> +<!ENTITY pref_quickshare_summary "Display quick share menu">

Once we have a near-final build, let's get some screenshots and ask antlam for final copy.

::: mobile/android/base/resources/xml/preferences_privacy.xml
@@ +29,5 @@
>              android:title="@string/pref_learn_more"
>              android:persistent="false"
>              url="https://www.mozilla.org/firefox/dnt/" />
>  
> +    <CheckBoxPreference android:key="privacy.quickshare.enabled"

There is a convention we use where preferences not stored in Gecko are prefixed with "android.not_a_preference." so:

android.not_a_preference.privacy.quickshare.enabled

@@ +30,5 @@
>              android:persistent="false"
>              url="https://www.mozilla.org/firefox/dnt/" />
>  
> +    <CheckBoxPreference android:key="privacy.quickshare.enabled"
> +        android:defaultValue="@bool/pref_quick_share_enabled"

It seems there's a precedent for hardcoding the values directly in the xml [1] and I'm fine with continuing that to reduce overhead of looking up the values and to reduce the clutter in our various xml files.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=android%3AdefaultValue&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +859,5 @@
>  
>          return removed;
>      }
>  
> +    // Removes all history records from cache

Since this file is copy-pasted from the Android source, we comment any of our additions with, "Mozilla:". Look around the file for examples.

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #27)
> I think the privacy use case extends to sharing devices – e.g. why private
> browsing exists. 

This is helpful. Thanks

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #28)
> Comment on attachment 8647106 [details] [diff] [review]
> Added preference in settings to enable/disable quick share option
> -----------------------------------------------------------------
> ::: mobile/android/base/BrowserApp.java
> @@ +3309,5 @@
> > +            ((GeckoMenuItem)quickShare).setActionProvider(provider);
> 
> This is done in BrowserApp.onCreateOptionsMenu - why did you add this here
> as well?

To update the options menu. GeckoActionProvider.onCreateActionView needs to be called every time clicking on menu after setting is changed. I've now removed the same line from BrowserApp.onCreateOptionsMenu, to avoid redundancy. 
 
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +198,5 @@
> > +<!ENTITY pref_quickshare_title "Quick share">
> > +<!ENTITY pref_quickshare_summary "Display quick share menu">
> 
> Once we have a near-final build, let's get some screenshots and ask antlam
> for final copy.

Should I then remove this dtd and Strings.xml.in from the patch?
 
> ::: mobile/android/base/resources/xml/preferences_privacy.xml
> @@ +29,5 @@
> > +    <CheckBoxPreference android:key="privacy.quickshare.enabled"
> 
> There is a convention we use where preferences not stored in Gecko are
> prefixed with "android.not_a_preference." so:
> 
> android.not_a_preference.privacy.quickshare.enabled

I could see how it sets up the preference screen initially for gecko preferences but couldn't figure out why it is so. What is the difference between the two categories (Gecko vs non-Gecko)?

> ::: mobile/android/base/widget/ActivityChooserModel.java
> @@ +859,5 @@
> >  
> > +    // Removes all history records from cache
> 
> Since this file is copy-pasted from the Android source, we comment any of
> our additions with, "Mozilla:". Look around the file for examples.

It's not clear to me. Can you please elaborate a little? I referred to this code- https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ActivityChooserModel.java#836

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #30)
> To update the options menu. GeckoActionProvider.onCreateActionView needs to
> be called every time clicking on menu after setting is changed. I've now
> removed the same line from BrowserApp.onCreateOptionsMenu, to avoid
> redundancy.

Does the options menu not updated by itself? I was under the impression that the options menu will just poll the Provider for content. Perhaps we need to call a method on the provider to indicate it's been updated?

> Should I then remove this dtd and Strings.xml.in from the patch?

No, we'll leave it in and then show antlam as an example so he can figure out the final copy.

> I could see how it sets up the preference screen initially for gecko
> preferences but couldn't figure out why it is so. What is the difference
> between the two categories (Gecko vs non-Gecko)?

Gecko, our browser engine, has its own set of preferences that are independent from the Java UI. Sometimes we want to provide a Java UI to modify these preferences (thus the Gecko preferences). Rather than whitelist these preferences, I believe we just pick up anything that doesn't start with "android.not_a_preference" and sync it to Gecko.

You may have noticed the Preferences in Firefox is not instantaneous to load like other applications – this is because we're retrieving and syncing the Java UI with the preferences stored in Gecko.

> > Since this file is copy-pasted from the Android source, we comment any of
> > our additions with, "Mozilla:". Look around the file for examples.
> 
> It's not clear to me. Can you please elaborate a little? I referred to this
> code-
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/
> ActivityChooserModel.java#836

ActivityChooserModel is a private class shipped as a part of the Android framework (i.e. [1]) but we wanted to make our own modifications to it so we copied it into our own tree (i.e. [2]). We want to know which changes are ours (e.g. so when there's a bug, we know there's a higher probability it's in our code) so we make comments on our changes and prefix the comment with "Mozilla:", e.g. [3].

[1]: http://androidxref.com/4.4.4_r1/xref/frameworks/support/v7/appcompat/src/android/support/v7/internal/widget/ActivityChooserModel.java
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ActivityChooserModel.java
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ActivityChooserModel.java?rev=52763281c7b1#17

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #31)

> (In reply to Dipti Nirmale [:dnirm] from comment #30)
> > To update the options menu. GeckoActionProvider.onCreateActionView needs to
> > be called every time clicking on menu after setting is changed. I've now
> > removed the same line from BrowserApp.onCreateOptionsMenu, to avoid
> > redundancy.
> 
> Does the options menu not updated by itself? I was under the impression that
> the options menu will just poll the Provider for content. Perhaps we need to
> call a method on the provider to indicate it's been updated?
 
The current behavior (without calling quickShare.setActionProvider) is like this- 1. Quick share is enabled and if you share anything, the icon will be displayed onto options menu. 2. Now after disabling the quick share, click on the menu option and it still displays the icon. It goes off only if you click on "share" again. 
Perhaps we need to call only GeckoMenu.onShowAsActionChanged. I'll try with it.
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenuItem.java#273

> Gecko, our browser engine, has its own set of preferences that are
> independent from the Java UI. Sometimes we want to provide a Java UI to
> modify these preferences (thus the Gecko preferences). Rather than whitelist
> these preferences, I believe we just pick up anything that doesn't start
> with "android.not_a_preference" and sync it to Gecko.

I get it now. I was little confused before with the "preference" keyword.

    
    

    
  
Comment on attachment 8647106 [details] [diff] [review]
Added preference in settings to enable/disable quick share option

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

Looks like you're going in the right direction.

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +865,5 @@
> +        mHistoricalRecords.clear();
> +        mHistoricalRecordsChanged = true;
> +        notifyChanged();
> +        mReloadActivities = true;
> +        ensureConsistentState();

Did you consider if we need mInstanceLock for this? If so, why don't we need it?

Also, I believe notifyChanged will be called in ensureConsistentState() since mReloadActivities is true and the intent should not have been set to null.

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +50,4 @@
>  public class GeckoActionProvider {
>      private static final int MAX_HISTORY_SIZE_DEFAULT = 2;
>  
> +    private final static String LOG_TAG = "GeckoActionProvider";

nit: "Gecko" + GeckoActionProvider.class.getSimpleName();

@@ +108,5 @@
>      public GeckoActionProvider(Context context) {
>          mContext = context;
>      }
>  
> +    public static void updateQuickSharePref(boolean prefValue) {

I feel like the reason the view doesn't update is because this method should call out to the existing providers (or not be static and be called on all of the providers that way) to update the preference. I'd be surprised if there isn't a generic preference changed listener somewhere.

@@ +174,5 @@
>          return view;
>      }
>  
> +    //Clears all historical records in history file
> +    private void clearHistoryRecordsInFile() {

This looks really complicated – is there a better way to do this? Do you have an example of how this file is formatted? Can the file just be cleared (e.g. `rm file && touch file`)?

        Attachment #8647106 -
        Flags: review?(michael.l.comella) → feedback+

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #33)

> ::: mobile/android/base/widget/ActivityChooserModel.java
> @@ +865,5 @@
> > +        mHistoricalRecords.clear();
> > +        mHistoricalRecordsChanged = true;
> > +        notifyChanged();
> > +        mReloadActivities = true;
> > +        ensureConsistentState();
> 
> Did you consider if we need mInstanceLock for this? If so, why don't we need
> it?

I didn't see apparent behaviour that would need locking but might have missed on this. I'll recheck it. Can you point me to some doc if available for better understanding of Gecko threads?
 
> ::: mobile/android/base/widget/GeckoActionProvider.java
> @@ +108,5 @@
> >      public GeckoActionProvider(Context context) {
> >          mContext = context;
> >      }
> >  
> > +    public static void updateQuickSharePref(boolean prefValue) {
> 
> I feel like the reason the view doesn't update is because this method should
> call out to the existing providers (or not be static and be called on all of
> the providers that way) to update the preference. I'd be surprised if there
> isn't a generic preference changed listener somewhere.

Agree for the generic listener. I've replaced this static reference with preference from GeckoSharedPrefs.forApp. But it still doesn't affect the view as expected.

> @@ +174,5 @@
> >          return view;
> >      }
> >  
> > +    //Clears all historical records in history file
> > +    private void clearHistoryRecordsInFile() {
> 
> This looks really complicated – is there a better way to do this? Do you
> have an example of how this file is formatted? Can the file just be cleared
> (e.g. `rm file && touch file`)?

Example: history file => 
<?xml version='1.0' encoding='UTF-8' standalone='yes' ?>
<historical-records>
<historical-record activity="com.android.mms/com.android.mms.ui.ComposeMessageActivity" time="1436897520022" weight="1.0" />
<historical-record activity="com.whatsapp/com.whatsapp.ContactPicker" time="1437082800367" weight="1.0" />
</historical-records>

After formatting the file => 
<?xml version='1.0' encoding='UTF-8' standalone='yes' ?>
<historical-records>
</historical-records>

The other way is deleting the file but I'm not sure if this xml is supposed to hold any other data as well (in future). So deleting only the record nodes and rewriting the file sounded good option. What do you think?

    
    

    
  
Sorry, nearly missed this! You can use the "Need more information from" field below to give a user a notification (like me!) so this doesn't happen again!

(In reply to Dipti Nirmale [:dnirm] from comment #34)
> > Did you consider if we need mInstanceLock for this? If so, why don't we need
> > it?
> 
> I didn't see apparent behaviour that would need locking but might have
> missed on this. I'll recheck it. Can you point me to some doc if available
> for better understanding of Gecko threads?

afaik, there is no documentation regarding the use of Gecko threads. However, this is unrelated to Gecko threads.

I noticed that mInstanceLock is used by ActivityChooserModel, which comes from the Android code and makes assumptions that it can be used from any thread – it would be good to maintain those assumptions.

> > @@ +174,5 @@
> > This looks really complicated – is there a better way to do this? Do you
> > have an example of how this file is formatted? Can the file just be cleared
> > (e.g. `rm file && touch file`)?
> 
> The other way is deleting the file but I'm not sure if this xml is supposed
> to hold any other data as well (in future).

Good point.

> So deleting only the record
> nodes and rewriting the file sounded good option. What do you think?

I agree, that does sound more correct. Perhaps we can hide the ugly bits in a util class.

Do you want to take a look at the locking issue, possibly send me an updated patch, and I'll take another look?
Flags: needinfo?(dnirm.moz)

    
    

    
  


  
Modified code as per review comments on previous patch.

        Attachment #8647106 -
        Attachment is obsolete: true

        Attachment #8653809 -
        Flags: review?(michael.l.comella)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #35)
> (In reply to Dipti Nirmale [:dnirm] from comment #34)
> > > Did you consider if we need mInstanceLock for this? If so, why don't we need
> > > it? 
> I noticed that mInstanceLock is used by ActivityChooserModel, which comes
> from the Android code and makes assumptions that it can be used from any
> thread – it would be good to maintain those assumptions.
> 
Right. I missed it. After a little digging realized that ensureConsistentState can be called from multiple threads and access to mIntent (and perhaps mReloadActivities) are made thread safe for the same reason. Since the newly added method also use them, it should use mInstanceLock. I've made this change in updated patch. 
 
> > So deleting only the record
> > nodes and rewriting the file sounded good option. What do you think?
> I agree, that does sound more correct. Perhaps we can hide the ugly bits in
> a util class.

Is there any existing util class that can be used or should we have a new one here?
Flags: needinfo?(dnirm.moz)

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #37)
> Is there any existing util class that can be used or should we have a new
> one here?

Nothing that I can see off-hand but maybe you want to check out m/a/b/util [1] yourself.

[1]: http://mxr.mozilla.org/mozilla-central/find?text=&string=mobile%2Fandroid%2Fbase%2Futil

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #39)
> (In reply to Dipti Nirmale [:dnirm] from comment #37)
> > Is there any existing util class that can be used or should we have a new
> > one here?
> 
> Nothing that I can see off-hand but maybe you want to check out m/a/b/util
> [1] yourself.
> 
> [1]:
> http://mxr.mozilla.org/mozilla-central/
> find?text=&string=mobile%2Fandroid%2Fbase%2Futil

Thanks for pointing out. How about FileUtils.deleteChildNodesFromFile()? Other option is having new util class for xml handling. Since this function rewrites to the file, using existing 'FileUtils' looks good to me.
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #40)
> (In reply to Michael Comella (:mcomella) from comment #39)
> > (In reply to Dipti Nirmale [:dnirm] from comment #37)
> > > Is there any existing util class that can be used or should we have a new
> > > one here?
> > 
> > Nothing that I can see off-hand but maybe you want to check out m/a/b/util
> > [1] yourself.
> > 
> > [1]:
> > http://mxr.mozilla.org/mozilla-central/
> > find?text=&string=mobile%2Fandroid%2Fbase%2Futil
> 
> Thanks for pointing out. How about FileUtils.deleteChildNodesFromFile()?
> Other option is having new util class for xml handling. Since this function
> rewrites to the file, using existing 'FileUtils' looks good to me.

Additionally, should we have ClearHistoryFileAsyncTask (to clear the file in background) in order to avoid StrictMode policy violation? This might take a little time to update the menu.

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #40)
> Thanks for pointing out. How about FileUtils.deleteChildNodesFromFile()?

FileUtils seems to be more about doing file system operations (e.g. read, delete whole files).

> Other option is having new util class for xml handling.

Let's do that – XmlUtils.

(In reply to Dipti Nirmale [:dnirm] from comment #41)
> Additionally, should we have ClearHistoryFileAsyncTask (to clear the file in
> background) in order to avoid StrictMode policy violation? This might take a
> little time to update the menu.

Good find! Yes, this would be nice though if it's a lot of work, we can consider it to be a follow-up.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Comment on attachment 8653816 [details]
Screenshot of quick share option in browser settings

Let's bring in Anthony for final copy/UX. Provided I can get the patch working locally, I'll host a build for him to try out.

        Attachment #8653816 -
        Flags: review?(michael.l.comella) → review+

    
    

    
  
Comment on attachment 8653809 [details] [diff] [review]
Added lock in ActivityChooserModel.removeAllHistoricalRecords

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

Need to run for now – I'll finish this review later.

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +169,5 @@
>          return view;
>      }
>  
> +    //Clears all historical records in history file
> +    private void clearHistoryRecordsInFile() {

Actually, I just noticed the ActivityChooserModel.persistHistoricalDataIfNeeded method. Did you try using that? You might need to modify some internal variables though.

    
    

    
  
Comment on attachment 8653809 [details] [diff] [review]
Added lock in ActivityChooserModel.removeAllHistoricalRecords

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

This patch does not apply – do you mind rebasing it?  I fixed it locally to try it out though.

Every time I jump into our menu code, I get the biggest headache – good work finding a solution on this! I'm impressed. :)

I'm surprised you need to update the toolbar menu but not the context menu (if you haven't discovered it, long press on a link and there is a quick share menu there) – do you know why this is?

I'll pass a build to Anthony, and we'll see what he thinks.

::: mobile/android/base/BrowserApp.java
@@ +3307,5 @@
>              quickShare.setVisible(shareVisible);
>              quickShare.setEnabled(shareEnabled);
>  
> +            // To update the menu whenever quick share is disabled.
> +            ((GeckoMenuItem)quickShare).refreshMenuItem();

nit: ((GeckoMenuItem) quickShare)...

It sounds expensive to do this every time the menu is shown – can we only refresh the menu when the preference changes? Did you try using this listener?

http://developer.android.com/reference/android/content/SharedPreferences.OnSharedPreferenceChangeListener.html

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +119,5 @@
>      public View onCreateActionView(final int maxHistorySize, final ActionViewType viewType) {
>          // Create the view and set its data model.
>          ActivityChooserModel dataModel = ActivityChooserModel.get(mContext, mHistoryFileName);
> +
> +        if (!GeckoSharedPrefs.forApp(mContext).getBoolean(GeckoPreferences.PREFS_QUICKSHARE_ENABLED, true)) {

I feel this would be more relevant down next to the other dataModel code – any reason not to put it there?

        Attachment #8653809 -
        Flags: review?(michael.l.comella) → feedback+

    
    

    
  
Anthony, can you check out this build [1]? Dipti added an option to hide the quick share menu in Settings -> Privacy. Can you give us a final copy on the settings and ensure the preference works as expected? Thanks.

[1]: https://people.mozilla.com/~mcomella/apks/mcomella-1166385_01.apk
Flags: needinfo?(alam)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #44)
> Comment on attachment 8653809 [details] [diff] [review]
> Added lock in ActivityChooserModel.removeAllHistoricalRecords
> 
> ::: mobile/android/base/widget/GeckoActionProvider.java
> @@ +169,5 @@
> >          return view;
> >      }
> >  
> > +    //Clears all historical records in history file
> > +    private void clearHistoryRecordsInFile() {
> 
> Actually, I just noticed the
> ActivityChooserModel.persistHistoricalDataIfNeeded method. Did you try using
> that? You might need to modify some internal variables though.

I tried it before applying GeckoMenuItem.refreshMenuItem and mInstanceLock. I wasn't sure of the reason of failure to update the view with persisted empty records. But now it seems to be working without any modification of any existing variables in ActivityChooserModel.persistHistoricalDataIfNeeded.

    
    

    
  


  
Remove all historical Records only once after the quick share is disabled and then prevent the recording of history until quick share is enabled.

        Attachment #8655800 -
        Flags: review?(michael.l.comella)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #45)
> Comment on attachment 8653809 [details] [diff] [review]
> Added lock in ActivityChooserModel.removeAllHistoricalRecords
> 
> Review of attachment 8653809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch does not apply – do you mind rebasing it?  I fixed it locally to
> try it out though.

I'll upload the modified patch after rebasing. I tried to do it but got some issues because of which I had to build the whole project. I'm not sure if it is correct as I never tried it before. Could you please tell me if it is rebased properly?
 
> Every time I jump into our menu code, I get the biggest headache – good work
> finding a solution on this! I'm impressed. :)
 Thank you :) Initially there was bit confusion but later got through it with your help.

> I'm surprised you need to update the toolbar menu but not the context menu
> (if you haven't discovered it, long press on a link and there is a quick
> share menu there) – do you know why this is?

I see 'Share link' but didn't find quick share. Could you please share screenshot or code to know where am missing?
 
> ::: mobile/android/base/BrowserApp.java
> @@ +3307,5 @@
> >              quickShare.setVisible(shareVisible);
> >              quickShare.setEnabled(shareEnabled);
> >  
> > +            // To update the menu whenever quick share is disabled.
> > +            ((GeckoMenuItem)quickShare).refreshMenuItem();
> 
> nit: ((GeckoMenuItem) quickShare)...
> 
> It sounds expensive to do this every time the menu is shown – can we only
> refresh the menu when the preference changes? Did you try using this
> listener?

I've added a flag now which would help execute the above code only when setting is changed. It has to be done when the menu options are created/used. So far it seems that menu can't be accessed unless it is invoked.
  
> ::: mobile/android/base/widget/GeckoActionProvider.java
> @@ +119,5 @@
> >      public View onCreateActionView(final int maxHistorySize, final ActionViewType viewType) {
> >          // Create the view and set its data model.
> >          ActivityChooserModel dataModel = ActivityChooserModel.get(mContext, mHistoryFileName);
> > +
> > +        if (!GeckoSharedPrefs.forApp(mContext).getBoolean(GeckoPreferences.PREFS_QUICKSHARE_ENABLED, true)) {
> 
> I feel this would be more relevant down next to the other dataModel code –
> any reason not to put it there?

Refactored this piece of code.
Flags: needinfo?(michael.l.comella)

    
    

    
  


  
Remove all historical Records only once after the quick share is disabled and then prevent the recording of history until quick share is enabled.

        Attachment #8655800 -
        Attachment is obsolete: true

        Attachment #8655800 -
        Flags: review?(michael.l.comella)

        Attachment #8655810 -
        Flags: review?(michael.l.comella)

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #50)
> (In reply to Michael Comella (:mcomella) from comment #45)
> > Comment on attachment 8653809 [details] [diff] [review]
> > Added lock in ActivityChooserModel.removeAllHistoricalRecords

> > I'm surprised you need to update the toolbar menu but not the context menu
> > (if you haven't discovered it, long press on a link and there is a quick
> > share menu there) – do you know why this is?
> 
> I see 'Share link' but didn't find quick share. Could you please share
> screenshot or code to know where am missing?

I got the quick share context menu. The context menu is updated only after the toolbar menu is updated. Need to check more.

    
    

    
  


  
Unlike the toolbar menu, context menu need not be refreshed explicitly since floating context menu does not have an update callback (onPrepareOptionsMenu) function and initiates the menu every time the long-click event is occurred.

        Attachment #8655810 -
        Attachment is obsolete: true

        Attachment #8655810 -
        Flags: review?(michael.l.comella)
Flags: needinfo?(michael.l.comella)

        Attachment #8656790 -
        Flags: review?(michael.l.comella)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #46)
> Anthony, can you check out this build [1]? Dipti added an option to hide the
> quick share menu in Settings -> Privacy. Can you give us a final copy on the
> settings and ensure the preference works as expected? Thanks.
> 
> [1]: https://people.mozilla.com/~mcomella/apks/mcomella-1166385_01.apk

Sure, I can get some copy but since we are also in the midst of doing some clean up work of our Settings. But I'd like to take a step back here and see if another preference is indeed what we should be doing?

What are we trying to address here? Is it still privacy concerns? Or is it a "let me customize my menu" thing?

From comment 8, I see that we started talking about a dialog that offers an option on long-press. Later, a pref was suggested as a simpler way to address this bug. But, I'm hesitant to add another settings pref, especially for a feature like quick-search because I really believe in this feature and it makes the user experience of Firefox that much better.

tl;dr
 - What's the problem we're trying to solve?
 - for Privacy, maybe this should clear if the user clears their data? perhaps history? It feels like an awfully specific use-case though.
 - for Customizability, perhaps we should look at ways to customize the menu as a whole and provide more value that way.
Flags: needinfo?(alam) → needinfo?(michael.l.comella)

    
    

    
  
Others can weigh in but...

(In reply to Anthony Lam (:antlam) from comment #54)
> What are we trying to address here? Is it still privacy concerns? Or is it a
> "let me customize my menu" thing?

From my understanding, this change is being made for privacy concerns.

>  - for Privacy, maybe this should clear if the user clears their data?
> perhaps history? It feels like an awfully specific use-case though.

We could do that, but then users have to frequently go back and clear the setting (e.g. why the "always private browsing" bug is a thing).

NI antlam for follow-up.
Flags: needinfo?(michael.l.comella) → needinfo?(alam)

    
    

    
  
Dipti, I'll review this when antlam gets back to us.

    
    

    
  
Sorry for the delay here. I think all this work is great but I'm just still not convinced this needs another preference inside Settings. Especially since our Settings are undergoing some re-organization work at the moment. I'm a little uncomfortable just adding another one in there unless we really have to.

That being said, I'd like to hold off on this and step back to understand the problem space a bit more. I feel like this is a pretty core feature to the Firefox experience. And at the same time, I don't see a clear connection with Privacy concerns either. So, perhaps improvements can be made to this feature in general that will also address this issue more wholly.  

I also worry that just allowing users to disable it could cause issues in other parts of our app's experience.

I'm going to CC Barbara to get this on Product's radar too.
Flags: needinfo?(alam)

    
    

    
  
Sorry, Dipti, but I think we're going to have put this on hold until Anthony identifies the larger UX problem (which will likely be after the settings cleanup). I should have been more on top of this to bring Anthony back into the discussion before we moved ahead with the preference.

You've been doing great and I'd love for you to continue making Firefox awesome! If you'd like to work on another bug in the meantime, I found some high visibility ones: how about bug 1155860, bug 1164879, or bug 1056031?

    
    

    
  
Interesting timing I think for two reasons

1) We are in the midst of doing user research / card sorting on our menu settings items. Having said that, I'd also like to hold off in adding something under settings->privacy but I like the idea to have this cleared via clear data. However, I don't think will solve the issue that was raised in the first place.
2) I just had to pull some Telemetry numbers to compare frequency of Copy URL from URL address vs. Share URL, and was surprised and amazing by how much Sharing is being used by our users. 

- Copying URLs from URL bar: 28%
- Share URL (via quick share, menu-> share etc): 72%

And even more important, focusing on all the ways you can share an URL, either via menu->share (1), via home panel/top sites (2), or via selecting a link (and hold) on web content (3), the quick share button (4) is pressed and used way more often than any of the other ones:
(1) 1% 
(2) 3%
(3) 20%
(4) 76%

So having said that, I think this is such a useful feature that we don't want to change too much in a way that it removes the love it gets from our current users.

My 2 cents:
- Unless we hear more concerns or complaints, we won't add another disable/enable anywhere.
- If we all think this could be a good candidate for any privacy / user control, we could have it cleared via clear data (privacy) as a separate option (besides saved logins, cache, site settings etc.). That way we would be able to satisfy the concern that was raised here: https://support.mozilla.org/en-US/questions/1044116 "I don't want to clear my cache/data because I don't want to lose my login sessions.".


And I can only repeat what mcomella said, thanks Dipti for raising this and the work you've done but it just seems not something that many people seem to worry too much at the moment, and your time is probably better spent on the the bugs mcomella mentioned, if you want to help :)

Thanks

    
    

    
  
In reply to Michael Comella (:mcomella) from comment #58)
> Sorry, Dipti, but I think we're going to have put this on hold until Anthony
> identifies the larger UX problem (which will likely be after the settings
> cleanup). I should have been more on top of this to bring Anthony back into
> the discussion before we moved ahead with the preference.

It's okay. This is disappointing but I understand it happens when multiple teams or members are involved. Is there anything I can do to avoid this in future bugs I'll be working on?

> 
> You've been doing great and I'd love for you to continue making Firefox
> awesome! If you'd like to work on another bug in the meantime, I found some
> high visibility ones: how about bug 1155860, bug 1164879, or bug 1056031?

I'll go through these bugs. Could you please let me know if there is any feature request that I can work on? I'd like to work on more challenging bugs like adding a new component or performance improvement. Also my patches for Bug 1169419 are waiting for review by Richard. Is it possible for anyone else to review them in case he is very busy?

    
    

    
  
Thanks for all your help Dipti! On to the next one! :)

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #60)
> Is there anything I can do to avoid this in
> future bugs I'll be working on?

I see at least two approaches:
  * Double-check that the design is final. I think the best person to check with is Anthony because he (more-or-less) gets the final say on what's acceptable in the UI and is better connected to product than myself or most of the engineers. In this particular bug, we should have NI'd him again after we decided to move towards a setting – that was my fault, and I apologize.
  * Choose bugs that are higher priority (i.e. more eyes from different teams gets on them) or polish-related. Both of these styles of bugs are unlikely to be deemed undesirable or overturned – higher priority bugs because we've already thought a lot about them and polish-related bugs because nobody can be upset about making a more polished product (provided the added code is not too complex).

> I'll go through these bugs. Could you please let me know if there is any
> feature request that I can work on?

I should be able to dig up new UI features for you to work on.

bug 1140048 is a high priority feature that we wouldn't back out. That being said, it's on a timeline and it needs to be done for Firefox 44, which is November 2nd (but preferably sooner so it can be tested, we can find regressions, etc.). If you can finish it by then, you're welcome to have it!

If you don't want that pressure, I can try to find another bug – let me know.

> I'd like to work on more challenging
> bugs like adding a new component or performance improvement.

For perf improvement bugs, I'd recommend asking Richard.

> Also my patches
> for Bug 1169419 are waiting for review by Richard. Is it possible for anyone
> else to review them in case he is very busy?

Richard is also the best person to ask about who can do his reviews if he's busy – note that he's on GMT time this week and next.

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #62)
> bug 1140048 is a high priority feature that we wouldn't back out.

I forgot that Anthony wanted a few other bugs fixed first – bug 1147653 and bug 1183659. I'll get the latter done pretty sooner but let me know if bug 1147653 is interesting to you. If not, I can work on it and you can work on bug 1140048 concurrently.

    
    

    
  
Comment on attachment 8656790 [details] [diff] [review]
Update the menu when quick share preference is changed.

Clearing review as per comment 57.

        Attachment #8656790 -
        Flags: review?(michael.l.comella)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #62)
> 
> I see at least two approaches:
>   * Double-check that the design is final. I think the best person to check
> with is Anthony because he (more-or-less) gets the final say on what's
> acceptable in the UI and is better connected to product than myself or most
> of the engineers. In this particular bug, we should have NI'd him again
> after we decided to move towards a setting – that was my fault, and I
> apologize.
>   * Choose bugs that are higher priority (i.e. more eyes from different
> teams gets on them) or polish-related. Both of these styles of bugs are
> unlikely to be deemed undesirable or overturned – higher priority bugs
> because we've already thought a lot about them and polish-related bugs
> because nobody can be upset about making a more polished product (provided
> the added code is not too complex).

Thank you for the tips. I'll ensure these next time.
 
> bug 1140048 is a high priority feature that we wouldn't back out. That being
> said, it's on a timeline and it needs to be done for Firefox 44, which is
> November 2nd (but preferably sooner so it can be tested, we can find
> regressions, etc.). If you can finish it by then, you're welcome to have it!
> 
> If you don't want that pressure, I can try to find another bug – let me know.
> 

I'd like to work on the bug 1140048. Is "Send To device" the "Firefox sync" option?
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Dipti Nirmale [:dnirm] from comment #65)
> I'd like to work on the bug 1140048.

Great! You've been assigned!

> Is "Send To device" the "Firefox sync" option?

"Send to device" is connected with Firefox sync, but I wouldn't call it "the "Firefox sync" option". It's the paper airplane icon, without a title, that is inside the standard Android share menu. If you look at the mock, you'll notice this icon moves from within the standard Android share menu to the top level (i.e. the menu that appears when you initially hit the 3-dot menu). I'll give you some tips on how to test this in bug 1140048.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Reset assignee due to inactivity.
Assignee: dnirm.moz → nobody

    
    

    
  
For what it's worth, I'd like to add a vote for this. Many versions later, there's still a share menu, and it still adds quick buttons. Would be nice to be able to set what share options appear there, or at least clear history of them, for the sake of both privacy and appearance. I used a particular share option literally just once, and it didn't work anyway so I would never click it again, and now the icon is stuck at the top of my menu and it's the only bright colorful icon, distracting and useless. Android's share menu clutter is annoying enough without having to deal with more of the same in Firefox.

    
    

    
  
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: -- → P5

    
    

    
  
Any progress on this issue?


If Mozilla doesn't allow users to remove quick share, that won't stop me from soing it anyway. If your phone is rooted, you can delete /data/data/org.mozilla.firefox/files/mozilla/9r07sgg2.default/history.xml .



    
    

    
  
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: