949216 - Replace BrowserApp.dismissEditingMode() calls with BrowserToolbar.cancelEdit()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

This way we can move some of cancelEdit's logic behing BrowserToolbar.onBackPressed() too. Right now, cancelEdit() returns a URL which is never used in BrowserApp. It should return a boolean.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Change cancelEdit() to return a boolean (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Simplify dismissEditingMode() to only call cancelEdit() (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Handle toolbar's back press all in onBackPressed() (r=wesj)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8357142 [details] [diff] [review]
Handle toolbar's back press all in onBackPressed() (r=wesj)

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

Drive-by nits: I'm so low!

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +495,5 @@
> +            return true;
> +        }
> +
> +        if (dismissSiteIdentityPopup()) {
> +            return true;

nit: Why not just `return dismissSiteIdentityPopup();` here?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #4)
> Comment on attachment 8357142 [details] [diff] [review]
> Handle toolbar's back press all in onBackPressed() (r=wesj)
> 
> Review of attachment 8357142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by nits: I'm so low!
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +495,5 @@
> > +            return true;
> > +        }
> > +
> > +        if (dismissSiteIdentityPopup()) {
> > +            return true;
> 
> nit: Why not just `return dismissSiteIdentityPopup();` here?

Fair enough, done :-)

Comment 6

[Lucas Rocha (:lucasr)]

Ping?

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8357141 [details] [diff] [review]
Simplify dismissEditingMode() to only call cancelEdit() (r=wesj)

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

::: mobile/android/base/BrowserApp.java
@@ +1571,5 @@
>          }
>      }
>  
>      private boolean dismissEditingMode() {
> +        return mBrowserToolbar.cancelEdit();

Do we still even need this method then?

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8357140 [details] [diff] [review]
Change cancelEdit() to return a boolean (r=wesj)

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

TBH, I'm not sure if this boolean means that the cancel failed or that there was nothing to cancel. Maybe we could rename this if we want to go this way. cancelIfEditing() or something. Otherwise, callers should just query isEditing before calling this to be clear what's going on.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Handle toolbar's back press all in onBackPressed() (r=wesj)

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8363043 [details] [diff] [review]
Handle toolbar's back press all in onBackPressed() (r=wesj)

Here's a much simpler patch that does all the previous patches do and hopefully covers your concerns in terms of clarity.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8363043 [details] [diff] [review]
Handle toolbar's back press all in onBackPressed() (r=wesj)

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

Seems like an improvement.

Comment 12

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/85b768925776

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85b768925776