Closed Bug 949216 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attachment #8357140 - Flags: review?(wjohnston)
Attachment #8357141 - Flags: review?(wjohnston)
Attachment #8357142 - Flags: review?(wjohnston)
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?
(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 :-)
Ping?
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?
Attachment #8357141 - Flags: review?(wjohnston) → review-
Attachment #8357140 - Flags: review?(wjohnston) → review+
Attachment #8357142 - Flags: review?(wjohnston) → review+
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 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.
Attachment #8363043 - Flags: review?(wjohnston)
Attachment #8357140 - Attachment is obsolete: true
Attachment #8357141 - Attachment is obsolete: true
Attachment #8357142 - Attachment is obsolete: true
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.
Attachment #8363043 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/85b768925776
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.