887268 - Open items from "tabs from last time" page on a new tabs

Status: RESOLVED FIXED

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

Description

[Lucas Rocha (:lucasr)]

Instead of simply opening the URL in the current tab.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Allow passing an explicit EditingTarget to openUrl()

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Use HomePager.OnUrlOpenListener in BrowserSearch

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Open items from "tabs from last time" on a new tabs

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 773282 [details] [diff] [review]
(3/3) Open items from "tabs from last time" on a new tabs

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

Since we have a single method called openUrl() in BrowserApp that's overloaded to handle both behaviors, wouldn't it be more consistent to add another onUrlOpen() method that accepts an EditingTarget? This could be added to the existing OnUrlOpenListener interface instead of creating a new one.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 773282 [details] [diff] [review]
> (3/3) Open items from "tabs from last time" on a new tabs
> 
> Review of attachment 773282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since we have a single method called openUrl() in BrowserApp that's
> overloaded to handle both behaviors, wouldn't it be more consistent to add
> another onUrlOpen() method that accepts an EditingTarget? This could be
> added to the existing OnUrlOpenListener interface instead of creating a new
> one.

I considered doing that but EditingTarget is an implementation detail of toolbar/editing mode in the main UI. I don't want to leak this into the about:home interface. I feel that the new listener creates a more clear/concise contract between the two components.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Open items from "tabs from last time" on a new tabs

Comment 7

[Lucas Rocha (:lucasr)]

Changes the OnNewTabsListener interface to better support the fix for bug 887269.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 774036 [details] [diff] [review]
Open items from "tabs from last time" on a new tabs

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

I don't understand the reasoning behind an array here. In bug 887269, you're iterating through the cursors, building an array, passing it to onNewTabs(), then iterating through that array. Wouldn't it be more straightforward to simply call mNewTabsListener.onNewTab() directly for each item in the cursor? From an API perspective, it's also cleaner IMO to pass a single URL over an array of them.

I guess one advantage of using an array would be that you could execute some kind of pre/post batch operation without needing a separate callback, but I don't see you using that here. Any reason you can't just use the single onNewTab() callback you used before?

::: mobile/android/base/BrowserApp.java
@@ +1966,5 @@
>  
> +    // HomePager.OnNewTabsListener
> +    @Override
> +    public void onNewTabs(String[] urls) {
> +        for (int i = 0; i < urls.length; i++) {

Nit: use a for each loop

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> Comment on attachment 774036 [details] [diff] [review]
> Open items from "tabs from last time" on a new tabs
> 
> Review of attachment 774036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand the reasoning behind an array here. In bug 887269, you're
> iterating through the cursors, building an array, passing it to onNewTabs(),
> then iterating through that array. Wouldn't it be more straightforward to
> simply call mNewTabsListener.onNewTab() directly for each item in the
> cursor? From an API perspective, it's also cleaner IMO to pass a single URL
> over an array of them.

The reason is simple: about:home is guaranteed to be closed once the onNewTab listener is used (see the last few lines in openUrl() for reference). We can't assume the state of the HomePager fragments will be alive once onNewTab is called.

In other words, the operation to open new tabs has to done in one step. Hence the list-of-tabs approach. I'm open to suggestions for a better API though. Just let me know.
 
> ::: mobile/android/base/BrowserApp.java
> @@ +1966,5 @@
> >  
> > +    // HomePager.OnNewTabsListener
> > +    @Override
> > +    public void onNewTabs(String[] urls) {
> > +        for (int i = 0; i < urls.length; i++) {
> 
> Nit: use a for each loop

Done.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 774036 [details] [diff] [review]
Open items from "tabs from last time" on a new tabs

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

OK, makes sense.

Comment 11

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/projects/fig/rev/45ae7d54666f
http://hg.mozilla.org/projects/fig/rev/dca194f33a36
http://hg.mozilla.org/projects/fig/rev/c5d17b84a826

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/45ae7d54666f
https://hg.mozilla.org/mozilla-central/rev/dca194f33a36
https://hg.mozilla.org/mozilla-central/rev/c5d17b84a826