For #11404 - Add 'Open all' options in bookmarks #27138

rocketsroger · 2022-09-23T16:22:15Z

This is to re-land #21212

Add "Open all in new tabs" options in bookmarks
Add "Open all in private tabs" options in bookmarks
Add metrics tracking if the usage of "Open all in..." in bookmarks

Co-authored-by: Pg pg.developper.fr@gmail.com

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-debug task.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #11404

rocketsroger · 2022-09-23T23:42:08Z

app/src/main/res/values/strings.xml

+    <string name="open_all_warning_title">Confirm open</string>
+    <!-- Message to warn users that a large number of tabs will be opened
+    %d is a placeholder for the number of tabs that will be opened. -->
+    <string name="open_all_warning_message">You are about to open %d tabs. This might slow down Firefox while the pages are loading. Are you sure you want to continue?</string>


These strings are the same that's used in desktop. These will need to be reviewed.

rocketsroger · 2022-09-23T23:42:42Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt

@@ -83,6 +87,10 @@ class DefaultBookmarkController(

    private val resources: Resources = activity.resources

+    @Suppress("MagicNumber")
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal val maxOpenBeforeWarn = 15


This is a number that's from desktop.

rocketsroger · 2022-09-23T23:44:09Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt

@@ -58,7 +63,10 @@ class BookmarkNodeViewHolder(

        containerView.urlView.isVisible = item.type == BookmarkNodeType.ITEM
        containerView.setSelectionInteractor(item, mode, interactor)
-        menu.updateMenu(item.type)
+
+        CoroutineScope(Dispatchers.Default).launch {


I'm not sure if we need to update the menu with Dispatchers.Default. This was not discussed in the original PR.

@Mugurell thoughts here?

rocketsroger · 2022-10-04T18:02:33Z

When the bookmark folder contains more than 15 bookmarks:

rocketsroger · 2022-10-11T15:16:05Z

device-2022-10-11-111519.mp4

rocketsroger · 2022-10-11T20:05:48Z

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

What questions will you answer with this data?

This data will help us understand how often user open all bookmarks in a folder.

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?

We are adding new features in bookmarks to make users' interactions more efficient.

What alternative methods did you consider to answer these questions? Why were they not sufficient?

There are no other alternatives.

Can current instrumentation answer these questions?

No.

List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki. 
Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.

Measurement Description	Data Collection Category	Tracking Bug #
Open all in new tabs: A user opened all bookmarks in a folder in new tabs.	Category 2 - interaction data	#11404
Open all in private tabs: A user opened all bookmarks in a folder in private tabs.	Category 2 - interaction data	#11404

Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.

https://dictionary.telemetry.mozilla.org/apps/fenix

How long will this data be collected? Choose one of the following:

Up until version 120, with the option to renew at that point.

What populations will you measure?

All channels, all locales, all countries

If this data collection is default on, what is the opt-out mechanism for users?

Default Glean SDK opt-out mechanism.

Please provide a general description of how you will analyze this data.

Glean and Glam.

Where do you intend to share the results of your analysis?

 Only on Glean, Glam, and with mobile teams.

Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection?

No third-party tools.

eliserichards · 2022-10-11T20:53:17Z

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

1. What questions will you answer with this data?


* This data will help us understand how often user open all bookmarks in a folder.


2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?


* We are adding new features in bookmarks to make users' interactions more efficient.


3. What alternative methods did you consider to answer these questions? Why were they not sufficient?


* There are no other alternatives.


4. Can current instrumentation answer these questions?


* No.


5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.    
   Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.

Measurement Description Data Collection Category Tracking Bug #
Open all in new tabs: A user opened all bookmarks in a folder in new tabs. Category 2 - interaction data #11404
Open all in private tabs: A user opened all bookmarks in a folder in private tabs. Category 2 - interaction data #11404

6. Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.


* [https://dictionary.telemetry.mozilla.org/apps/fenix   ](https://dictionary.telemetry.mozilla.org/apps/fenix%E2%80%A8)


7. How long will this data be collected? Choose one of the following:


* Up until version 120, with the option to renew at that point.


8. What populations will you measure?


* All channels, all locales, all countries


9. If this data collection is default on, what is the opt-out mechanism for users?


* Default Glean SDK opt-out mechanism.


10. Please provide a general description of how you will analyze this data.


* Glean and Glam.


11. Where do you intend to share the results of your analysis?


* Only on Glean, Glam, and with mobile teams.


12. Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection?


* No third-party tools.

Data Review Form (to be filled by Data Stewards)

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?
- Yes, through the metrics.yaml, the Glean Dictionary, and GLAM.
Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.
- Yes, through the "Send Usage Data" preference in the application settings.
If the request is for permanent data collection, is there someone who will monitor the data over time?
- No, the data will be collected for one year (until version 120) with the option to remove/renew.
Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
- Category 2 - interaction data
Is the data collection request for default-on or default-off?
- Default-on
Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?
- No
Is the data collection covered by the existing Firefox privacy notice?
- Yes
Does the data collection use a third-party collection tool? If yes, escalate to legal.
- No

Result

data-review+

topotropic · 2022-10-13T17:12:58Z

Thanks! LGTM

ewachowiak · 2022-10-14T15:13:12Z

@rocketsroger I have a suggested edit to the strings in this reminder for clarity:

HEADLINE: Open 16 tabs?
BODY: Opening this many tabs may slow down Firefox while the pages are loading. Are you sure you want to continue?

Note that the "16" would be a variable to show the correct # of tabs. The word "Firefox" should also be a variable so the correct product name (E.g. Firefox Nightly) appears.

rocketsroger · 2022-10-14T15:36:09Z

@rocketsroger I have a suggested edit to the strings in this reminder for clarity:

HEADLINE: Open 16 tabs? BODY: Opening this many tabs may slow down Firefox while the pages are loading. Are you sure you want to continue?

Note that the "16" would be a variable to show the correct # of tabs. The word "Firefox" should also be a variable so the correct product name (E.g. Firefox Nightly) appears.

Thanks! I'll update to reflect this change.

rocketsroger · 2022-10-14T16:59:55Z

Updated. On local builds the app name is "Firefox Fenix". Thanks

gabrielluong · 2022-10-14T19:07:49Z

app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt

+        }
+
+        browserScreen {
+            // out of folder and should not be opened


I am not sure I really follow this comment. Could this be something that we can reword for better clarity?

Agreed. I'll update and move this below to where we verify that the tab is not opened.

gabrielluong · 2022-10-14T19:08:26Z

app/src/androidTest/java/org/mozilla/fenix/ui/robots/BrowserRobot.kt

        navigationToolbar {
        }.enterURLAndEnterToBrowser(url) {
            // needs to wait for the right url to load before saving a bookmark
            verifyUrl(url.toString())
        }.openThreeDotMenu {
-        }.bookmarkPage { }
+        }.bookmarkPage {
+            // continue only if a folder is defined


Suggested change

// continue only if a folder is defined

// continue only if a folder exists

gabrielluong · 2022-10-14T19:09:37Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt

+
+        when (node.type) {
+            BookmarkNodeType.FOLDER -> {
+                // if not leaf (empty) folder


Could we capitalize all of these comments?

gabrielluong · 2022-10-14T19:15:22Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt

+    override fun handleOpeningFolderBookmarks(folder: BookmarkNode, mode: BrowsingMode) {
+        // potentially heavy function with a lot of bookmarks to open => use a coroutine
+        scope.launch {
+            // if more than maximum elements


I find myself thinking that a lot of these comments in the PR could be improved upon or some might not be necessary since they're just repeating what the code will tell you.

This comment unfortunately doesn't make sense in relation with the next line of code which is returning early when the bookmark folder cannot be loaded. Rather I think this comment was trying to say that we warn users if the number of bookmarks are bigger than WARN_OPEN_ALL_SIZE, but that is already stated further down.

Agreed. I'll remove most of the comments and leave the one that says "Warn user if more than maximum number of bookmarks are being opened"

gabrielluong · 2022-10-14T19:16:20Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt

+            if (hasAtLeastOneChild) {
+                if (itemType == BookmarkNodeType.FOLDER) {


I think these 2 if statements can be joined to avoid the nesting

Good catch, I'll join them. Thanks

gabrielluong · 2022-10-14T19:16:37Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt

+            if (hasAtLeastOneChild) {
+                if (itemType == BookmarkNodeType.FOLDER) {


Same as above

gabrielluong · 2022-10-14T19:17:47Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt

-    internal fun menuItems(itemType: BookmarkNodeType): List<TextMenuCandidate> {
+    @SuppressWarnings("LongMethod")
+    internal suspend fun menuItems(itemType: BookmarkNodeType, itemId: String): List<TextMenuCandidate> {
+        // if have at least one child


This is an example where I really don't think the comment is adding much for us. So, I would ask that we audit the comments and try to add clarity to them in the PR.

gabrielluong · 2022-10-25T19:15:29Z

app/metrics.yaml

+  open_all_in_private_tabs:
+    type: event
+    description: |
+      A user opened a folder of bookmarks at once in new private tabs.


Suggested change

A user opened a folder of bookmarks at once in new private tabs.

A user opened all the bookmarks in a folder in new private tabs.

gabrielluong · 2022-10-25T19:15:50Z

app/metrics.yaml

+  open_all_in_new_tabs:
+    type: event
+    description: |
+      A user opened a folder of bookmarks at once in new tabs.


Suggested change

A user opened a folder of bookmarks at once in new tabs.

A user opened all the bookmarks in a folder in new tabs.

gabrielluong · 2022-10-25T19:30:24Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt

@@ -25,14 +26,19 @@ class BookmarkItemMenu(
        Share,
        OpenInNewTab,
        OpenInPrivateTab,
+        OpenAllInTabs,


Suggested change

OpenAllInTabs,

OpenAllInNewTabs,

Sounds good I'll update

gabrielluong · 2022-10-25T19:31:31Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt

@@ -80,6 +80,16 @@ class BookmarkFragmentInteractor(
        }
    }

+    override fun onOpenAllInTabs(folder: BookmarkNode) {


Suggested change

override fun onOpenAllInTabs(folder: BookmarkNode) {

override fun onOpenAllInNewTabs(folder: BookmarkNode) {

Thoughts?

gabrielluong · 2022-10-25T19:33:02Z

app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt

@@ -58,7 +63,10 @@ class BookmarkNodeViewHolder(

        containerView.urlView.isVisible = item.type == BookmarkNodeType.ITEM
        containerView.setSelectionInteractor(item, mode, interactor)
-        menu.updateMenu(item.type)
+
+        CoroutineScope(Dispatchers.Default).launch {


@Mugurell thoughts here?

gabrielluong · 2022-10-25T19:33:53Z

app/src/main/res/values/strings.xml

@@ -622,6 +622,17 @@
    <!-- Content description (not visible, for screen readers etc.): "Close button for library settings" -->
    <string name="content_description_close_button">Close</string>

+    <!-- Title to show in alert when a lot of tabs are to be opened


I am assuming these strings already got OK'd

Yes, these strings were reviewed and approved by @ewachowiak

* Add "Open all in new tabs" options in bookmarks * Add "Open all in private tabs" options in bookmarks * Add metrics tracking if the usage of "Open all in..." in bookmarks Co-authored-by: Pg <pg.developper.fr@gmail.com>

rocketsroger force-pushed the FB_11404 branch from 45752c9 to 1e3fb01 Compare September 23, 2022 23:27

rocketsroger changed the title ~~For #11404 - Add 'Open all' optionst in bookmarks~~ For #11404 - Add 'Open all' options in bookmarks Sep 23, 2022

rocketsroger force-pushed the FB_11404 branch 2 times, most recently from 0da10f1 to 469e4b8 Compare September 23, 2022 23:38

rocketsroger commented Sep 23, 2022

View reviewed changes

rocketsroger force-pushed the FB_11404 branch from 469e4b8 to 9d91f69 Compare September 23, 2022 23:48

rocketsroger force-pushed the FB_11404 branch from 9d91f69 to ea9a415 Compare October 4, 2022 18:01

rocketsroger added the needs:UX-feedback Needs UX Feedback label Oct 4, 2022

rocketsroger force-pushed the FB_11404 branch 5 times, most recently from 51508b8 to 6976d1e Compare October 11, 2022 19:50

rocketsroger added the needs:strings Needs strings label Oct 11, 2022

rocketsroger marked this pull request as ready for review October 11, 2022 19:59

rocketsroger requested review from a team as code owners October 11, 2022 19:59

rocketsroger added the needs:data-review PR is awaiting a data review label Oct 11, 2022

rocketsroger force-pushed the FB_11404 branch from 6976d1e to be7dd1d Compare October 11, 2022 20:16

eliserichards removed the needs:data-review PR is awaiting a data review label Oct 11, 2022

rocketsroger added the needs:review PRs that need to be reviewed label Oct 12, 2022

topotropic removed the needs:UX-feedback Needs UX Feedback label Oct 13, 2022

rocketsroger force-pushed the FB_11404 branch from be7dd1d to 168f95f Compare October 14, 2022 16:57

rocketsroger removed the needs:strings Needs strings label Oct 14, 2022

gabrielluong reviewed Oct 14, 2022

View reviewed changes

rocketsroger force-pushed the FB_11404 branch from 168f95f to a588f61 Compare October 17, 2022 18:29

rocketsroger requested a review from gabrielluong October 17, 2022 18:32

gabrielluong approved these changes Oct 25, 2022

View reviewed changes

gabrielluong removed the needs:review PRs that need to be reviewed label Oct 25, 2022

For mozilla-mobile#11404 - Add 'Open all' options in bookmarks

869b014

* Add "Open all in new tabs" options in bookmarks * Add "Open all in private tabs" options in bookmarks * Add metrics tracking if the usage of "Open all in..." in bookmarks Co-authored-by: Pg <pg.developper.fr@gmail.com>

rocketsroger force-pushed the FB_11404 branch from a588f61 to 869b014 Compare October 25, 2022 21:00

rocketsroger added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Oct 26, 2022

Merge branch 'main' into FB_11404

48e3819

mergify bot merged commit 489548b into mozilla-mobile:main Oct 26, 2022

rocketsroger deleted the FB_11404 branch October 27, 2022 13:38

rocketsroger mentioned this pull request Aug 17, 2023

Bug 1849184 - Renew Fenix telemetry probes expiring in v120 mozilla-mobile/firefox-android#3319

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #11404 - Add 'Open all' options in bookmarks #27138

For #11404 - Add 'Open all' options in bookmarks #27138

rocketsroger commented Sep 23, 2022 •

edited by github-actions bot

Loading

rocketsroger Sep 23, 2022 •

edited

Loading

rocketsroger Sep 23, 2022

rocketsroger Sep 23, 2022 •

edited

Loading

gabrielluong Oct 25, 2022

rocketsroger commented Oct 4, 2022

rocketsroger commented Oct 11, 2022

rocketsroger commented Oct 11, 2022

eliserichards commented Oct 11, 2022

Request for data collection review form

topotropic commented Oct 13, 2022

ewachowiak commented Oct 14, 2022

rocketsroger commented Oct 14, 2022

rocketsroger commented Oct 14, 2022

gabrielluong Oct 14, 2022

rocketsroger Oct 17, 2022

gabrielluong Oct 14, 2022

gabrielluong Oct 14, 2022 •

edited

Loading

gabrielluong Oct 14, 2022

rocketsroger Oct 17, 2022

gabrielluong Oct 14, 2022 •

edited

Loading

rocketsroger Oct 17, 2022

gabrielluong Oct 14, 2022

gabrielluong Oct 14, 2022

gabrielluong Oct 25, 2022

gabrielluong Oct 25, 2022

gabrielluong Oct 25, 2022

rocketsroger Oct 25, 2022

gabrielluong Oct 25, 2022

gabrielluong Oct 25, 2022

gabrielluong Oct 25, 2022

rocketsroger Oct 25, 2022 •

edited

Loading

	// continue only if a folder is defined
	// continue only if a folder exists

		if (hasAtLeastOneChild) {
		if (itemType == BookmarkNodeType.FOLDER) {

	A user opened a folder of bookmarks at once in new private tabs.
	A user opened all the bookmarks in a folder in new private tabs.

	override fun onOpenAllInTabs(folder: BookmarkNode) {
	override fun onOpenAllInNewTabs(folder: BookmarkNode) {

For #11404 - Add 'Open all' options in bookmarks #27138

For #11404 - Add 'Open all' options in bookmarks #27138

Conversation

rocketsroger commented Sep 23, 2022 • edited by github-actions bot Loading

Pull Request checklist

QA

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

rocketsroger Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketsroger Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketsroger commented Oct 4, 2022

rocketsroger commented Oct 11, 2022

rocketsroger commented Oct 11, 2022

Request for data collection review form

eliserichards commented Oct 11, 2022

Request for data collection review form

Data Review Form (to be filled by Data Stewards)

Result

topotropic commented Oct 13, 2022

ewachowiak commented Oct 14, 2022

rocketsroger commented Oct 14, 2022

rocketsroger commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketsroger Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

rocketsroger commented Sep 23, 2022 •

edited by github-actions bot

Loading

rocketsroger Sep 23, 2022 •

edited

Loading

rocketsroger Sep 23, 2022 •

edited

Loading

gabrielluong Oct 14, 2022 •

edited

Loading

gabrielluong Oct 14, 2022 •

edited

Loading

rocketsroger Oct 25, 2022 •

edited

Loading