895816 - Tapping "New tab" should immediately create a new tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Lucas Rocha (:lucasr)]

...instead of simply entering editing mode. This means that even if you dismiss editing mode, you'd still be in the newly created tab (as opposed to just canceling editing mode).

Comment 2

[:Margaret Leibovic]

Attachment: (Part 1) Tapping "New tab" should immediately create a new tab

Well, that was quite simple. If you look below, we basically already do the same thing for adding a new private tab.

Comment 3

[:Margaret Leibovic]

Attachment: (Part 2) Remove EditingTarget because it's no longer needed

Since we're only ever entering editing mode for the currently open tab, we can get rid of this EditingTarget logic.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 779875 [details] [diff] [review]
(Part 1) Tapping "New tab" should immediately create a new tab

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

Nice and simple. I assume adding tabs from tabs tray and menu both call this very same method?

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 779875 [details] [diff] [review]
> (Part 1) Tapping "New tab" should immediately create a new tab
> 
> Review of attachment 779875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice and simple. I assume adding tabs from tabs tray and menu both call this
> very same method?

Yes, correct.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 779878 [details] [diff] [review]
(Part 2) Remove EditingTarget because it's no longer needed

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

Nice,

::: mobile/android/base/BrowserApp.java
@@ +1294,5 @@
>          tab.setFaviconLoadId(Favicons.NOT_LOADING);
>      }
>  
> +    public boolean enterEditingMode() {
> +        return enterEditingMode(null, HomePager.Page.BOOKMARKS);

Add an enterEditingMode(HomePager.Page page) and use it here? Just so that these calls look cleaner.

@@ +1303,4 @@
>      }
>  
> +    public boolean enterEditingMode(HomePager.Page page) {
> +        return enterEditingMode(null, page);

Ditto.

Comment 7

[:Margaret Leibovic]

Attachment: (Part 3) Update BaseTest.addTab to use loadUrl

Now that opening a new tab just loads about:home, loading a URL in a new tab can just follow the regular loadUrl() logic.

testNewTab passes locally for me, but we can wait on the try results before landing this.

Comment 8

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 779878 [details] [diff] [review]
> (Part 2) Remove EditingTarget because it's no longer needed
> 
> Review of attachment 779878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice,
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1294,5 @@
> >          tab.setFaviconLoadId(Favicons.NOT_LOADING);
> >      }
> >  
> > +    public boolean enterEditingMode() {
> > +        return enterEditingMode(null, HomePager.Page.BOOKMARKS);
> 
> Add an enterEditingMode(HomePager.Page page) and use it here? Just so that
> these calls look cleaner.

We already have that right down below. Is it worth the extra method call, rather than just calling directly into the method with the main implementation?

> @@ +1303,4 @@
> >      }
> >  
> > +    public boolean enterEditingMode(HomePager.Page page) {
> > +        return enterEditingMode(null, page);
> 
> Ditto.

Wait... this is the implementation of enterEditingMode(HomePager.Page page), so we can't do that, right?

Comment 9

[:Margaret Leibovic]

Attachment: (Part 2) Remove EditingTarget because it's no longer needed (v2)

This version does a bit more cleaning up...

onSearchRequested is never used anymore, so we can get rid of that. With that gone, enterEditingMode is only called in onActivate and when the user selects the "Paste" context menu. Talking about this on IRC, lucasr and I decided it made sense for us to just always open the VISITED page in these two cases, so we don't need to pass the page as a parameter anymore.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 780064 [details] [diff] [review]
(Part 2) Remove EditingTarget because it's no longer needed (v2)

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

I think onSearchRequested is called when you press the hw search button which is only available on older Android devices. I'd probably keep it around. Looks good otherwise.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 2) Remove EditingTarget because it's no longer needed (v3)

This version keeps onSearchRequested, although I found that's not actually working properly, even on fig nightly, so I filed bug 897274 about that.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/projects/fig/rev/0ffd6875249b
https://hg.mozilla.org/projects/fig/rev/77efbb48885d
https://hg.mozilla.org/projects/fig/rev/a9f98a5f888f

There are some intermittent oranges on the try run, but I feel like we're okay since testNewTab isn't failing at all (and there's been at least one fully green rc1 and rc2).

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0ffd6875249b
https://hg.mozilla.org/mozilla-central/rev/77efbb48885d
https://hg.mozilla.org/mozilla-central/rev/a9f98a5f888f