Our boundary conditions for matching search groups to visits was wrong.
This change switches the boundary buffer to only be applied to history
items, not the metadata items.
In other words, when checking if any of the metadata items match the
current "page" of history, we'll be looking to see if the item falls
within this time window:
buffer - oldest history item <= metadata item <= newest history item +
buffer
There's a separate problem with buffer though: it's reset to 0 when requested
offset is >0, but that requires a larger refactor of this code, for a
separate PR.
When you want to look at one of these markers, you usually want to look
at all three so I found that having them on a single track was easier to
follow. Since they run in sequence, they should never overlap and that
should minimize confusion.
* For #21313: Product telemetry renewals for December
* For #21313: Data review for december product telemetry renewals
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* For #22075 - Added event to track the count of recent bookmarks
* For #22075 - Added data review issue number
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* For #22107 - Added probe to track if the Recent tabs / jump back in section is visible
* For #22107 - Fixed lint errors
* For #22107 - added data review number to metric
* For #22166 - fixed expiration date
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
A quantity probe in the metrics ping means we'll loose the granularity events
provided but it will be easier to extract the values.
For reporting whether the inactive tabs feature is enabled or not we already
have the "preferences.inactive_tabs_enabled" probe so I didn't duplicate this.
When deciding if we should include a history group within the "page of
history" results on the History View UI, we used to look at the most
recent timestamp of the metadata items within the group, and see if that
falls within the range of the timestamps of the history page, +/- some
buffer.
This assumes that each metadata entry will have a corresponding history
item. However, that's not true - when restarting the app, the selected
tab will be restored, and when opening History View right after we'll
record some metadata for it. However, we won't record a history visit
during the app restore for the selected tab.
That's all correct, but the assumption around group matching to history is now incorrect.
This patch changes the logic to instead look at every item within the
group, and see if any of them match the time window of the current
history page. This has a side-effect of also displaying search groups
multiple times on diffenent pages of history, if it makes sense to do so chronologically.
I think that's fine, it reflects reality at least (e.g. items within the
group may have been visited at very different points in time).
Co-authored-by: Christian Sadilek <christian.sadilek@gmail.com>
* For #21313: Renew metrics for December - never expire updates
* For #21313: data review
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Set long term business-critical metrics as non-expiring.
* Remove quotes around "never"
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
The onLayout marker may be redundant to onGlobalLayout marker but I'm not
sure yet so let's leave them both in and observe if that's the case.
Here's a profile with the markers: https://share.firefox.dev/3lZaOQb
Assuming the InlineAutocompleteEditText is not being recreated (and I
did not verify this), it's unnecessary to traverse the view hierarchy
to find it more than once so this patch removes the unnecessary
traversals.
* For #21903 - Added telemetry for interacting with inactive tabs
* For #21903 - Added missing inactive tab delete count event to delete all event
* For #21903 - Added PR numbers to metrics
* For #21903 - Updated broken unit tests. Resolved critical lint warning.
* For #21903 - Fixed inactive tabs setting toggle metric
* For #21903 - Updated FenixApp unit test
* For #21903 - Updated newline character in Metrics. Set inactive tab metrics' lifetime to default. Updated expiration to Nov 2022. Refactored inactive tabs metric to be a single metric.
* PR: addendum for last commit that missed a file
* For #21903 - Changed logic check for reporting inactive tab count
* PR: fixed merge conflict
* For #21903 - Removed tab close tracking when the user closes ALL inactive tabs
* For #21903 - Removed individual tab close metric verify from CLOSE ALL test
* For #21903 - Updated inactive tabs toggle setting expiration to match the expiration of the other events
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Remove redundant calls to setHasOptionsMenu(false)
Fix memory leaks for credit card and login fragments
* Fixes:
Add link to issue tracker
Use activity?.invalidateOptionsMenu() instead of setHasOptionsMenu(false)
Move it inside of 'if' statement to avoid unintended issues when called improperly
Revert changes to AddLoginFragment.kt
* Fix call invocation to redirectToReAuth() from AddLoginFragment.kt
Fix 'when' statement in redirectToReAuth() to use AddLoginFragment
Co-authored-by: Vitaly V. Pinchuk <vetal.978@gmail.com>
Fetching a set of logins from the store is quite expensive. This commit
avoids doing that while navigating back and forth between the list and
detail views:
- retain processes logins state when navigating into detail view
- use the `get` storage api to obtain specific login, instead of
`list().filter {...}`
- avoid re-sorting retained logins when navigating back into the list
view
Switched to always using `Login` instead of the `SavedPassword` alias.
Made `MasterPasswordTipProvider.saveLogins()` call
`importLoginsAsync()`. This is needed because it's the only method that
inputs a `Login` rather than a `LoginEntry`.
Moved the `SavedLoginsStorageController.kt.syncAndUpdateList` call
to inside `add()` and `update()`. This simplifies the error handling a
bit.
Refactored dupe-checking code to use findLoginToUpdate()
Refactored `AddLoginFragment` / `EditLoginFragment` to put the username
error handling code all in 1 method. I think it's easier to follow the
logic of showing/hiding the error labels when it's all in one place.
This fixes issues #24103 and #24104. I would love to address #24102,
but I'm not sure what the correct behavior is there so I just kept that
the same.
At this moment, we have two extension methods that have duplicate
functionality to construct search term groupings. One on `List<Tab>` and
one on `List<TabSessionState>`. The former is used for everything
related to tabs piped through the `TabsFeature` and the latter is for
consumers of `BrowserState` directly.
The bug occurs because our implementation of search groupings was
updated only on the former extension, but the `HeaderBinding`, that
observes the BrowserState and updates the title visibility, was using
the latter.
Ideally, we remove this duplication when we no longer have separate data
classes for consumers of `TabsFeature`, but this intermediary fix should
suffice.
Before this patch, this was the behavior - 'remove' button is clicked, we'd ask
the storage to remove metadata (on its IO thread), then navigate to Home
Screen.
This resulted in a race we could end-up on the Home Screen before delete
finishes, so the search groups do not appear to be removed (but,
refreshing the Home Screen again shows that they are removed).
This also resulted in an unnecessary navigation which felt very janky
(screen will "scroll" to the top) and was way more work than necessary.
After this patch, we:
- dispatch two actions (on browserstore, on homefragmentstore) which
remove the search groups from any relevant in-memory state; any UI bound to
this state will be automatically "refreshed"
- no longer navigate as part of the remove action, so the UI doesn't
move and removal happens "in-place"
It seems like we no longer need to use rotation for the chevron, since
we are now using two different icons within the `ic_chevon` that change
depending on the `state_activated`.
This helps identify file IO. Unfortunately, with this marker, it's
difficult to separate code we own from code we don't own. However, I
wasn't sure what the best implementation would be to address that
(e.g. ideally, we would ignore violations in code we don't own rather than
annotate the markers) so I thought we can land it this simple way and
improve it incrementally.
* For #21437 - Relocated Home-related settings to its dedicated sub screen
* For #21437 - Updated show top sites toggle text
* PR: Fixed lint warning. Reverted preference keys
* PR: added ignore for UI test
* PR: Added ignore for UI test
The bottom gray border of the header item from the Inactive Tabs section was correctly set when collapsing or expanding said section, but not on init. So if the section was initialized collapsed the gray border would not be present.
This regressed in our previous fix that made sure child tabs don't
mistakenly get moved out of the group if their parent is navigated
away, or in case the child tabs are redirected.
However, when a subsequent load occurs in any tab in the group the
search terms need to be cleared and the tab removed from the group
to prevent false positives.
* Fix description for `home_screen_displayed` metric
The current description appears to be incorrect by my reading of the source.
* Update app/metrics.yaml
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
Co-authored-by: Will Lachance <wlachance@mozilla.com>
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
* Docs only: Add extra context to `events.browser_menu_action`
This came up as a potential point of confusion in a discussion with DS. This is a docs only change.
* Update metrics.yaml
Co-authored-by: Will Lachance <wrlach@gmail.com>
* For #21360 - Added toggle for search term tab groups
* For #21360 - Lint cleanup
* PR: Added missing licenses and possibly fixed UI test
* PR: Added a "scrollTo" to potentially fix a UI test
* PR: Added potential fix for alwaysStartOnHomeTest
* PR: Added temporary ignore to alwaysStartOnHomeTest
* PR: added missing ignore comment
* For #21360 - Added missing feature flag driven visibility logic
Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Home screen isn't actually visible in case we're displaying awesomebar
search results. The navigation is thus unnecessary and actually causes visual
jankiness as we display home for a moment before covering it up with
search results.
The latest geckoview-omni package correctly declares its capabilities,
including the `glean-native` one.
Additionally it is able to pick geckoview-omni over glean-native in all
configurations.
* Issue mozilla-mobile#21319 - Moved inactive tabs to the top of the normal tabs tray.
* Issue mozilla-mobile#21319 - Added a delete icon to delete ALL inactive tabs.
* Issue mozilla-mobile#21319 - Changed default inactive time period to 14 days
* Issue mozilla-mobile#21319 - Hooked inactive tabs setting to UI code
Inactive tabs setting is also disabled when the user has selected the one day or week auto-close tab setting.
* Issue mozilla-mobile#21319 - File and Lint cleanup
* PR: Fixed bug causing grouped tabs to also show in "Other" when marked as inactive but inactive is OFF in Settings
* PR: Fixed lint warnings
* PR: Removed redundant feature check
* PR - Ignore test until search term tab groups switch is done
These double comparisons are easier to read and see the pattern of on one line
so I'd rather keep them on one line. Additionally, it's difficult to
test each change individually so I'd rather not make additional changes.
To do this, I suppressed the max line length warning.
This commit was generated primarily by a macro that:
- appends `== component &&`
- appends `== item`
- (if applicable) Skips to the ending brace
- Go down one line and move cursor to the front of the line to prep for repeat
My only intervention was to skip extra lines to line it up to run again
and specify how many times in a row it should run.
---
The `to` in this code is an infix function that calls instantiates a
Pair under the hood. Subjectively observed, when this method is called
it generally hits the else case so 35 Pairs are instantiated each call -
that's 560 bytes. This method is called frequently - for example, an estimated
4 times each time a letter is typed on the homescreen and a measured 116 times
in a simple navigation (see the issue). The latter generates an estimated
63.4 KiB.
It was straightforward to remove these allocations so that's what this
change does.
The primary risk from this change is that it's difficult to test each
case to ensure it's working.