Closed Bug 1234295 Opened 4 years ago Closed 4 years ago

MasterPassword snackbar is not visible

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox45 --- verified
firefox46 --- verified
fennec 45+ ---

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(3 files, 2 obsolete files)

If you try to remove the master password and enter a wrong password we show a snackbar:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/MasterPassword.js#64

This is done in GeckoPreferences but snackbars are currently only shown inside BrowserApp: The snackbar is not visible.
tracking-fennec: --- → ?
Sebastian, would switching the root_layout here https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1375 with R.android.id.content fix this?
Flags: needinfo?(s.kaspari)
(In reply to Alex Johnson (:alex_johnson) from comment #1)
> Sebastian, would switching the root_layout here
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/GeckoApp.java#1375 with R.android.id.content fix this?

I don't think so: The event will still be received in BrowserApp and we will add the snackbar to the content layout of BrowserApp. Even right now: If you click back very fast you see the snackbar we created from within GekoPreferences in BrowserApp.

We probably need to move the Snackbar code to some helper that gets called from BrowserApp and GeckoPreferences. GeckoPreferences could register for Snackbar:Show events too.
Flags: needinfo?(s.kaspari)
Assignee: nobody → s.kaspari
tracking-fennec: ? → 45+
Duplicate of this bug: 1233422
See Also: → 1237538
Status: NEW → ASSIGNED
Comment on attachment 8705697 [details]
MozReview Request: Bug 1234295 - Add SnackbarHelper for creating and dismissing snackbars. r=mcomella

https://reviewboard.mozilla.org/r/30141/#review26877

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:40
(Diff revision 1)
> +        public synchronized void onClick(View view) {

Do we need to call `super` here?

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:46
(Diff revision 1)
> +            callback = null;

Why do we null the reference after the callback is fired? Add a comment.

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:50
(Diff revision 1)
> +        public synchronized void onDismissed(Snackbar snackbar, int event) {

Do we need to call `super`?

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:56
(Diff revision 1)
> +            callback = null;

Same – add a comment.

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:60
(Diff revision 1)
> +    private static final Object lock = new Object();

nit: -> `currentSnackbarLock`

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:61
(Diff revision 1)
> +    private static WeakReference<Snackbar> currentSnackbar = new WeakReference<>(null); // Guarded by 'lock'

Do you initialize this so that we don't get NPE later? If so, can you add a comment?

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:83
(Diff revision 1)
> +    public static void showSnackbar(Activity activity, String message, int duration, String action, SnackbarCallback callback) {

nit: I'd prefer this name to be more specific, e.g. `showSnackbarWithActionButton`.

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:105
(Diff revision 1)
> +            Snackbar snackbar = currentSnackbar.get();

nit: `final`

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:113
(Diff revision 1)
> +     * Find best parent view to hold Snackbar's view. Later the Snackbar implementation will walk up the view tree to

"Later the Snackbar implementation will" – can you list a bug number?

::: mobile/android/base/moz.build:514
(Diff revision 1)
> +    'SnackbarHelper.java',

I'd prefer this to not be at the top-level to avoid additional clutter – is there any relevant package we can put it in? Maybe put it in the `util` package and re-name it to `SnackbarUtil`?
Attachment #8705697 - Flags: review?(michael.l.comella) → review+
https://reviewboard.mozilla.org/r/30141/#review27175

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:85
(Diff revision 1)
> +        final Snackbar snackbar = Snackbar.make(parentView, message, duration);

I saw in one of the methods you changed that this method was called from the UI thread – does this need to be called from the UI thread?
Comment on attachment 8705698 [details]
MozReview Request: Bug 1234295 - Use SnackbarHelper everywhere to create snackbars. r=mcomella

https://reviewboard.mozilla.org/r/30143/#review27165

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
(Diff revision 1)
> -import org.mozilla.gecko.AppConstants;

No need to do it now but it'd be good to keep this in a separate commit – it makes it easier to uplift, if necessary.

::: mobile/android/base/java/org/mozilla/gecko/preferences/SearchEnginePreference.java:95
(Diff revision 1)
> -                public void run() {
> +            SnackbarHelper.showSnackbar(activity,

I made an issue in the other commit but this may no longer be running on the UI thread – intentional?
Attachment #8705698 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8705699 [details]
MozReview Request: Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r=mcomella

https://reviewboard.mozilla.org/r/30145/#review27181

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:79
(Diff revision 1)
> +        final String msg = message.getString("message");

nit: `msg` coming from `message` is ambiguous. Maybe `msg` -> `snackbarActionString`?
Attachment #8705699 - Flags: review?(michael.l.comella) → review+
https://reviewboard.mozilla.org/r/30141/#review26877

> Do we need to call `super` here?

I don't think so. The super class is an interface and doesn't do anything.

> Why do we null the reference after the callback is fired? Add a comment.

To avoid executing the callback more than once (exception). I added a comment.

> Do we need to call `super`?

The super class doesn't do anything.

> Do you initialize this so that we don't get NPE later? If so, can you add a comment?

Yes, that correct. I feel like this comment would add more clutter than being helpful?

> nit: I'd prefer this name to be more specific, e.g. `showSnackbarWithActionButton`.

Good call! I updated the name.

> "Later the Snackbar implementation will" – can you list a bug number?

The way I wrote it is confusing: What I wanted to say is that that's what the Snackbar implementation (of the support library) alreday does. I updated the comment.

> I'd prefer this to not be at the top-level to avoid additional clutter – is there any relevant package we can put it in? Maybe put it in the `util` package and re-name it to `SnackbarUtil`?

The util package is somewhat isolated and compiled separately. SnackbarHelper references GeckoApp and R.id.root_layout. I agree moving is a good idea. Maybe creating a new package? With a dedicated package I'd move the SnackbarCallback classes to own files too.
https://reviewboard.mozilla.org/r/30141/#review27175

> I saw in one of the methods you changed that this method was called from the UI thread – does this need to be called from the UI thread?

The Snackbar implementation already takes care of that and executes code on the UI thread if needed.
https://reviewboard.mozilla.org/r/30145/#review27181

> nit: `msg` coming from `message` is ambiguous. Maybe `msg` -> `snackbarActionString`?

I changed "message" to "object" (It's a NativeObject and not necessarily a "message") and then "msg" to "message".
Comment on attachment 8705697 [details]
MozReview Request: Bug 1234295 - Add SnackbarHelper for creating and dismissing snackbars. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30141/diff/1-2/
Attachment #8705697 - Attachment description: MozReview Request: Bug 1234295 - Add SnackbarHelper for creating and dismissing snackbars. r?mcomella → MozReview Request: Bug 1234295 - Add SnackbarHelper for creating and dismissing snackbars. r=mcomella
Comment on attachment 8705698 [details]
MozReview Request: Bug 1234295 - Use SnackbarHelper everywhere to create snackbars. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30143/diff/1-2/
Attachment #8705698 - Attachment description: MozReview Request: Bug 1234295 - Use SnackbarHelper everywhere to create snackbars. r?mcomella → MozReview Request: Bug 1234295 - Use SnackbarHelper everywhere to create snackbars. r=mcomella
Comment on attachment 8705699 [details]
MozReview Request: Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30145/diff/1-2/
Attachment #8705699 - Attachment description: MozReview Request: Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r?mcomella → MozReview Request: Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r=mcomella
Attachment #8705697 - Attachment is obsolete: true
Attachment #8705698 - Attachment is obsolete: true
Attachment #8706861 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8706862 [details]
MozReview Request: Bug 1234295 - Use SnackbarHelper everywhere to create snackbars. r=mcomella

I messed up the review on reviewboard and lost the r+
Attachment #8706862 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/integration/fx-team/rev/9bc0476728bb9ee31d80d59c02c60dfe18e0e330
Bug 1234295 - Add SnackbarHelper for creating and dismissing snackbars. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/8997deb26c3426864a625b9119f4387a4020d1a2
Bug 1234295 - Use SnackbarHelper everywhere to create snackbars. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/62d40b2d384c44dd650707fcc48dbc9870dbcb1c
Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r=mcomella
Attachment #8706862 - Flags: approval-mozilla-aurora?
Attachment #8706861 - Flags: approval-mozilla-aurora?
Comment on attachment 8705699 [details]
MozReview Request: Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r=mcomella

(Request for all three patches)

Approval Request Comment

[Feature/regressing bug #]: Snackbars have been introduced app-wide in Firefox 45 - see meta bug 1157526.

[User impact if declined]: Snackbars in settings screen might not show up. Additionally some snackbars are not dismissed whenever the user clicks outside.

[Describe test coverage new/current, TreeHerder]: Local testing and try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3b88a6cf94a

[Risks and why]: I tested all code paths that create a snackbar; So I think the risk is low.

[String/UUID change made/needed]: -
Attachment #8705699 - Flags: approval-mozilla-aurora?
Comment on attachment 8705699 [details]
MozReview Request: Bug 1234295 - GeckoPreferences: Listen for snackbar events and display snackbars. r=mcomella

Taking it in 45.
Attachment #8705699 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8706861 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8706862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on Firefox 45 Beta 1
Trying to remove the master password and enter a wrong password , "Incorrect password" snackbar is displayed, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0) 
Build: Firefox for Android 46.0a2 (2016-02-16)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.