Closed Bug 454880 Opened 11 years ago Closed 7 years ago

Allow access to recent history through back/forward buttons

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox21 --- verified
relnote-firefox --- 21+
fennec + ---

People

(Reporter: christian.bugzilla, Assigned: cvielma)

References

(Blocks 1 open bug)

Details

(Keywords: uiwanted, Whiteboard: [mentor=bnicholson@mozilla.com][lang=java])

Attachments

(1 file, 12 obsolete files)

11.92 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
In order to be able to slide the control strip away after completing an action, one option is to bring up the back/forward history on press-and-hold so the user can get to the exact location with one "command-sequence".
Whiteboard: UI
Flags: wanted-fennec1.0+
What is the use case here?  You want to be able to view your browser history so you can jump back multiple pages?
Target Milestone: Fennec A1 → ---
This is related to bug 454978, where the strips will slide away after you have completed one action, so in the case where you want to go back say 3 entries in your history, you will either have to slide out the strip 3 times or do the "press-and-hold" to bring up a history dropdown (or similar).
madhava: any thoughts on UI for this kind of feature?
Whiteboard: UI → UI polish
Keywords: uiwanted
This is a good idea.  The use case is wanting to go multiple pages back -- one example is reading a multipage article in a newspaper;  by the time I'm finished on page four, I want to go back to the front page, and I currently have to go back through pages 1-3.  This is problem, partly because of the UI going away after hitting the button, but even more because it means loading each of those intervening pages, which consumes data and time.

We may want to surface it some way other than tap and hold... it could be a separate button, or we could have it be both tap and hold _and_ have it come up if the user taps "back" more than once before the first back has fully loaded.  Still thinking about this.

Could we show thumbnails or a page title list?  Grouped by TLD would be useful, if a user really wants to go to the previous site rather than just the previous page.
From an implementation point of view it would be a good idea to have a new modifier that we can specify on a <key> element. Just as <key keycode="VK_LEFT" modifiers="control"/> means the left arrow key in combination with the control modifier key, <key keycode="VK_LEFT" modifiers="long"/> would mean the left arrow key held down for longer than some threshold.

Of course, we can fake it in JS, but that would complicate a lot of things (such as the shortcut editor) unnecessarily.
Attached patch wip 1 - basic feature support (obsolete) — Splinter Review
This patch adds the code needed to support a context menu on the back and forward buttons. The menu holds the last (or next) X URL entries from the session history.

Depends on bug 427987 for tap-and-hold. However, it works in the desktop builds using right click.
Assignee: nobody → mark.finkle
Duplicate of this bug: 465287
Does it still work in desktop builds on right click?  I can't seem to get it to do anything.
(In reply to comment #8)
> Does it still work in desktop builds on right click?  I can't seem to get it to
> do anything.

Won't do anything until the patch lands
What about the idea of doing a left-stroke from the back button -- dragging back from that button into the content area -- as a way to trigger going deeper into the history?
Blocks: 477628
tracking-fennec: --- → 1.0+
Summary: Use press-and-hold for back/forward to bring up history → Allow access to recent history through back/forward buttons
Madhava - Are we still doing this or not?
tracking-fennec: 1.0+ → ---
Flags: wanted-fennec1.0+
We should probably push this to 1.1
Whiteboard: UI polish
Duplicate of this bug: 610635
Whiteboard: [fennec-4.1?]
Madhava - any new thoughts on wanting this feature?
tracking-fennec: --- → 7+
Whiteboard: [fennec-4.1?]
IMHO, the best solution here is pressing and holding on the back button. It's not the most elegant solution, but it's becoming a standard way to discover “more” functionality on any given button/target.

I think we already have a discoverability problem with bringing up the sidebar, which is done with a left swipe. I think mapping that gesture to another action once the sidebar is shown may be really hard to discover. In addition, wouldn't we run into a problem with panning on the actual content if you swiped past the borders of the sidebar?

Given that the next version of android will almost force us to put an action bar at the top, the current thinking is that we will probably add back and forward buttons there. Should we put off features like this until we know what we're doing for ice cream sandwich?
(In reply to comment #15)

> Given that the next version of android will almost force us to put an action
> bar at the top, the current thinking is that we will probably add back and
> forward buttons there. Should we put off features like this until we know
> what we're doing for ice cream sandwich?

We should make designs for phone and tablet. Long press on our back button is a fine way to start. What do we show after that?
Brian, we need a UI for this to move forward, and I think we want to get it in soonish.
Assignee: mark.finkle → bdils.mozilla
tracking-fennec: 7+ → 8+
tracking-fennec: 8+ → +
OS: Maemo → All
Adding Ian as well
Duplicate of this bug: 683888
presumably we still care about this for the tablet UI where we have back and forward buttons
Assignee: bdils.mozilla → nobody
Product: Fennec → Fennec Native
(In reply to Brad Lassey [:blassey] from comment #20)
> presumably we still care about this for the tablet UI where we have back and
> forward buttons

The stock android browser allows you to hold the system back button to bring up your history on both tablet and phone (though it's global history as opposed to per tab history). If we do something similar, this feature could be relevant on a phone too.
This might be a good bug for a new contributor.  (Michael, feel free to take a shot at this if you are interested.)
Whiteboard: [mentor=mbrubeck@mozilla.com][lang=java]
Hi! 

I'm a new contributor, and I would like to fix this bug as my second fix contribution to Fennec :) 

Can someone, please, provide me with an introduction to which files are affected, where should I look to make the change and if there is an UI already developed that I should use fixing this bug?
Great!  I'll try to give some pointers.

To handle system key input, I think you'll want to override the OnKeyLongPress method of BrowserApp:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java

For the on-screen back button, you can add a SimpleOnGestureListener to the back button to detect long presses:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java

When one of these events happens, you'll need to send a message to the Gecko/JavaScript code, since that's where the session history lives.  In Tab.java and browser.js you can see the code that sends and receives various existing messages like Session:Back and Session:Forward:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js

You can send a new message from your Java event-handling code (something like "Browser:ShowSessionHistory") and add a listener for it in browser.js.  The listener can access BrowserApp.selectedBrowser.sessionHistory to get a list of history entries.  The sessionHistory object implements the nsISHistory interface:
http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/public/nsISHistory.idl

Then I think the JavaScript code can use the Prompt:Show message to display a dialog with the list of history entries.  This code has a similar use Prompt:Show to display a list:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectHelper.js

As you can see, there are many different pieces required here. Don't hesitate to ask if you have any questions or problems.
Assignee: nobody → cvielma
Status: NEW → ASSIGNED
Thanks Matt! I will give a try and let you know when doubts arise ;)
Hi Matt, 

I have reviewed the code and have some questions. 

- I can't find the OnKeyLongPress in BrowserApp or any of the superclasses or interfaces, should I implement a new method or should I override another method?

- Is there any documentation of a global view of how do the code integrates? I've seen this:

 https://wiki.mozilla.org/Fennec/NativeUI/Architecture_Overview 

for Fennec for Android and cpterson recommended me this one too:

https://developer.mozilla.org/en-US/docs/Mozilla_Source_Code_Directory_Structure 

but I can't see more explanation about how is organized or what are the functions of the code in Fennec.

- I noticed the way the information is passed from Tab.java to browser.js. But I don't understand how to integrate browser.js with nsISHistory.idl. And when you say that then I can use Prompt:Show of SelectHelper.js, I don't see where should I call this function, is it in browser.js?

Finally, I have seen some "tricky" (at least for me) code in the .js files that I've never seen before, do you use a specific js library? And where should I read a little bit more to understand .idl files? are they XUL files?

Thanks in advance!

Best regards,
(In reply to Christian Vielma (IRC :cvielma) from comment #27)
> - I can't find the OnKeyLongPress in BrowserApp or any of the superclasses
> or interfaces, should I implement a new method or should I override another
> method?

Sorry, I spelled it wrong.  The first letter should be lowercase.  It's a method of the Activity class:
http://developer.android.com/reference/android/app/Activity.html#onKeyLongPress%28int,%20android.view.KeyEvent%29

> - Is there any documentation of a global view of how do the code integrates?

Sorry, there's not much overview documentation besides the links you posted.  We need better developer docs.

> - I noticed the way the information is passed from Tab.java to browser.js.
> But I don't understand how to integrate browser.js with nsISHistory.idl.

An IDL file defines an interface that can be called from both JavaScript and C++ code.  In this case it lists the methods and properties of the "sessionHistory" object.  It tells you that you can do things like:

  var hist = BrowserApp.selectedBrowser.sessionHistory;
  var n = hist.count;
  var entry = hist.getEntryAtIndex(n-1, false);

and that "entry" is an object that implements the "nsIHistoryEntry" interface.

Reading an IDL file isn't always very informative; you might also want to click the name of a method you are interested in to search for example code that uses that method, e.g.:
http://mxr.mozilla.org/mozilla-central/ident?i=getEntryAtIndex

> And when you say that then I can use Prompt:Show of SelectHelper.js, I
> don't see where should I call this function, is it in browser.js?

Prompt:Show is a message that can be sent from JavaScript to Java using the sendMessageToJava function.  Here's some example code that builds a Prompt:Show message and then sends it to Java:

http://hg.mozilla.org/mozilla-central/file/13fd49ef7786/mobile/android/chrome/content/SelectHelper.js#l59
http://hg.mozilla.org/mozilla-central/file/13fd49ef7786/mobile/android/chrome/content/SelectHelper.js#l26

> Finally, I have seen some "tricky" (at least for me) code in the .js files
> that I've never seen before, do you use a specific js library?

For the most part our JS code does not use libraries, but it does use new JS language features that are not yet available in all browsers.  You can generally find information about these here; you might especially be interested in the pages about specific JavaScript versions:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference

Our JS code also uses "XPCOM" to access objects that are implemented in C++.  (The IDL file discussed above is an example of an XPIDL interface.)  You don't need a deep understanding XPCOM to use it; to a JS programmer, XPCOM interfaces are just objects that have a set of methods and properties that you can use like any other object.  But if you get curious, you can read more about it here:
https://developer.mozilla.org/en-US/docs/XPCOM
Thanks for the answers Matt. 

I have a few more :S 

What does this line (1024) in browser.js do? Who does it call?
browser.goBack();

I was thinking about doing something like this in browser.js 

//old
if (aTopic == "Session:Back") {
      browser.goBack();
    } else if (aTopic == "Session:Forward") {
      browser.goForward();
//new added
    } else if (aTopic == "Session:ShowHistory") {
        browser.showHistory();
   }

And then implement a showHistory function with this:

var hist = browser.sessionHistory;
var n = hist.count;
var entry = hist.getEntryAtIndex(n-1, false);

let msg = {
       
          type: "Prompt:Show",
          listitems: entry
       
      };
      let data = JSON.parse(sendMessageToJava(msg));

Am I right? or how should I send the message?
(In reply to Christian Vielma (IRC :cvielma) from comment #29)
> What does this line (1024) in browser.js do? Who does it call?
> browser.goBack();

Here, "browser" points to a XUL <browser> element.  You can read about this element and its methods here:
https://developer.mozilla.org/en-US/docs/XUL/browser

> I was thinking about doing something like this in browser.js 
> 
> //old
> if (aTopic == "Session:Back") {
>       browser.goBack();
>     } else if (aTopic == "Session:Forward") {
>       browser.goForward();
> //new added
>     } else if (aTopic == "Session:ShowHistory") {
>         browser.showHistory();
>    }

Yes, this looks good -- but instead of adding a showHistory method to the <browser> element you would add it to the Browser object in browser.js, and call it like: this.showHistory();

> And then implement a showHistory function with this:
> 
> var hist = browser.sessionHistory;
> var n = hist.count;
> var entry = hist.getEntryAtIndex(n-1, false);

This just looks up one history entry (the last one).  You'll want to write "for" loop that fetches every history entry, and adds an item to the Prompt:Show message for each one.

The Prompt:Show message must include a "listitems" array that contains an object for each entry; each object has properties {label, id, inGroup, isGroup, disabled}.  For your use case, you can just set the last three of those to false.  The id is an arbitrary number that will be returned from the Java code that processes the message and displays the result.  You can use the entry index as the id.  The label is the string to display in the prompt dialog.

The message also includes a "selected" array that contains a boolean for each entry -- true if the entry should be selected by default, false otherwise.  And the message should include {"multiple": false} which means the user can select only one entry.

See this function for example code that builds a valid Prompt:Show message:
http://hg.mozilla.org/mozilla-central/file/dd61540f237c/mobile/android/chrome/content/SelectHelper.js#l59
Hi Matt, thank you for the answers. About the "browser" element, where are this methods and properties implemented?
(In reply to Christian Vielma (IRC :cvielma) from comment #31)
> Hi Matt, thank you for the answers. About the "browser" element, where are
> this methods and properties implemented?

The <browser> element is implemented mostly in this XBL file:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml
Thanks Matt. A few more questions:

- Why should I OnKeyLongPress method of BrowserApp? Who does this method integrates with the BrowserToolbar or the ShowHistory method?

- How can I debug the javascript of browser.js? Using alerts? or is there any other more efficient way to test it?

- Finally, I'm implementing the showHistory method in browser like this:

let result = {
              type: "Prompt:Show",
              multiple: false,
              selected: [],
              listitems: []
      };

      let index = 0;
      
      let hist = browser.sessionHistory;
      let n = hist.count;
      
      for(let i = 1; i<=n, i++){
          let entry = hist.getEntryAtIndex(n-i, false);
          let item = {
                  label: entry,
                  isGroup: false,
                  inGroup: false,
                  disabled: false,
                  id: index
          }
          result.listitems[index] = item;
          result.selected[index] = false;
          index++;
      }
      
   let data = JSON.parse(sendMessageToJava({ gecko: result }));

Is that right? or should I change the "item" label to other thing? How does this Prompt:Show message will know to redirect to an specific url when clicking on an item?

Thanks in advance.
(In reply to Christian Vielma (IRC :cvielma) from comment #33)
> - Why should I OnKeyLongPress method of BrowserApp? Who does this method
> integrates with the BrowserToolbar or the ShowHistory method?

On phones with a hardware "Back" key, OnKeyLongPress is called when you press-and-hold the "Back" key.  (On these devices, there are two different Back buttons -- one in hardware and one in software -- so we need separate code to handle each of them.)

> - How can I debug the javascript of browser.js? Using alerts? or is there
> any other more efficient way to test it?

I don't think we have a real debugger that works with chrome JavaScript.  For printf/alert-style debugging, I usually use the "dump" function.  Its output is visible in the Android system log, which you can access with apps like aLogCat or by using various tools from the Android SDK.

> - Finally, I'm implementing the showHistory method in browser like this:

>           let item = {
>                   label: entry,
>                   isGroup: false,
>                   inGroup: false,
>                   disabled: false,
>                   id: index
>           }
>
> Is that right? or should I change the "item" label to other thing?

For the label, I would use the title if it's available, or the URL otherwise:

   label: entry.title || entry.URI.spec,

Also, it looks like "index" is not defined here; you probably want "n-i" instead.

> How does this Prompt:Show message will know to redirect to an
> specific url when clicking on an item?

After you parse the JSON response to your Java message and store it in the "data" variable, data.button should contain the ID of the selected entry.  Then you can access that entry again,  and call BrowserApp.loadURI to open the entry's URI.
By the way, thanks for your patience working through all this mostly-undocumented code.  I'll work on taking my comments from this bug and turning them into real documentation to help the next person using these APIs...
Hi Matt! Thanks for your answers. 

I have a few more questions :S

- I don't understand well how to implement the onLongKeyPress. Should I call the new showHistory() method I created in Tab.java? Which key should I check in onLongKeyPress?

- Is there a way to find other errors in the js? for example if there is a problem parsing the file (sintax error), where can I see the errors? 

- When you say:
>After you parse the JSON response to your Java message and store it in the "data" variable, >data.button should contain the ID of the selected entry.  Then you can access that entry >again,  and call BrowserApp.loadURI to open the entry's URI.

That method (BrowserApp.loadURI) is already implemented and I don't have to do anything else right? Or do I have to do something with the data variable after sending the message to java?


Thanks in advance.
(In reply to Christian Vielma (IRC :cvielma) from comment #36)
> - I don't understand well how to implement the onLongKeyPress. Should I call
> the new showHistory() method I created in Tab.java? Which key should I check
> in onLongKeyPress?

Yes, you should call the function that shows the history dialog, when onLongKeyPress is called with KeyEvent.KEYCODE_BACK.

> - Is there a way to find other errors in the js? for example if there is a
> problem parsing the file (sintax error), where can I see the errors? 

JS errors (including syntax errors) are also printed to the Android system log, where you can view them with "adb logcat" or similar tools.

> That method (BrowserApp.loadURI) is already implemented and I don't have to
> do anything else right? Or do I have to do something with the data variable
> after sending the message to java?

Right, loadURI is already implemented.  You just need to use "data.button" to see which history entry the user selected, and then tell the browser to load that entry's URI.  Something like this:

    let selectedEntry = hist.getEntryByIndex(data.button);
    this.loadURI(selectedEntry.URI);

BrowserApp.loadURI and much of the other core Fennec code is implemented in browser.js.  Aside from the source code, it looks like we actually have some basic documentation too!
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/BrowserApp
Thanks Matt!

Really sorry for bothering you so much. I think I almost have the patch, but I do the following at the end:

   let data = JSON.parse(sendMessageToJava({ gecko: result }));
   let selected = data.button;
   if (selected == -1)
       return;
   let selectedURI = hist.getEntryAtIndex(selected, false);
   this.loadURI(selectedURI.URI);

And doesn't do anything. I also tried using dump(selected); but nothing is shown. I followed also this: 
https://wiki.mozilla.org/Mobile/Fennec/Android#JavaScript_dump.28.29
and this:
http://joone4u.blogspot.com/2009/10/debugging-fennec-front-end.html

But still can't see the output of dump in the logcat.

Can you see what I'm doing wrong? with this I think I finish the patch.
(In reply to Christian Vielma (IRC :cvielma) from comment #38)
> And doesn't do anything. I also tried using dump(selected); but nothing is
> shown.

Do you see outputs from dump() calls elsewhere in the code? What if you dump() at the start of the function?  You can also try logging on the Java side to make sure the Session:ShowHistory message is sent.

If the message is sent but it doesn't seem to be received, make sure you add a call to addObserver here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#166

If the message is received but you aren't making it to the end of the function, maybe there's an exception somewhere earlier.  Try wrapping all your code in "try { ... } catch(ex) { dump(ex); }"
Hi. No I don't see any outputs from dump. I also wrapped the code with try and catch and nothing.

The Java is calling browser.js correctly. It shows the list with the title of the webpages but when selecting one the list disappears without loading the selected page.
It seems that is not refreshing the browser.js when I deploy on the device. The rest of the files update successfully but browser.js don't.
Attached patch Proposed patch (obsolete) — Splinter Review
Proposed patch. It was tested on Asus Transforme tablet with dock. Tests done: long press over back and forward button, and long back key pressed. Both shows the history depending if it has gone back or not. Clicking an option will navigate to that element of the history instead of only loading a new page.
Comment on attachment 670196 [details] [diff] [review]
Proposed patch

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

This looks great!  Just a few minor changes and I think this will be ready for review.

::: mobile/android/base/BrowserApp.java
@@ +905,2 @@
>          }
> +        return super.onKeyLongPress(keyCode, event);

Just some minor style nits:
- The brace at the start of the function should have a space before it.
- Opening brace should go on the same line as "if".
- Please remove the blank lines before the final closing brace.
- Try to avoid any whitespace at the end of a line.  (If you click on the "Splinter review" link in Bugzilla, it will highlight trailing whitespace.)

These also apply to some of the other code in this patch.

Our coding style isn't as consistent or well-documented as some other projects; some of it you just have to pick up as you go.  There's some documentation at:
https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

::: mobile/android/base/Tab.java
@@ +494,5 @@
> +    
> +    /**
> +     * This method will show the history starting on fromIndex until the end of the history. 
> +     */
> +    public boolean showHistory(int fromIndex){

I think we also want a toIndex argument (or maybe a "direction" argument), so showBackHistory can exclude pages that are "forward" from the current index.

::: mobile/android/chrome/content/browser.js
@@ +1131,5 @@
> +              type: "Prompt:Show",
> +              multiple: false,
> +              selected: [],
> +              listitems: []
> +      };

Nit: The mobile team uses 2-space indentation for JavaScript.

@@ +1138,5 @@
> +      
> +      let hist = browser.sessionHistory;
> +      let n = hist.count;
> +      
> +      for(let i = n-1; i>fromIndex; i--){

When showing the "forward" history, maybe we should show the items in forward order instead of reverse order.

I also wonder if we should limit the number of entries shown... are the UI and the performance reasonable when the session history is large (like, 50 or more items)?
Attachment #670196 - Flags: feedback+
Hi Matt, thank you for the feedback and sorry for my late answer. I made some changes but now even changing many things I can't make the button "forward" to activate. I mean, even if I use the normal "Back" once it never activates the forward button. I don't know what could it be.

Can you check it out?
Attached patch Proposed patch 2 (buggy) (obsolete) — Splinter Review
This includes some style corrections as well as limitations in the entries shown in the history. It uses the history by ranges. Although currently no matter what you do, the forward button never gets activated.
Comment on attachment 680449 [details] [diff] [review]
Proposed patch 2 (buggy)

Hi, sorry I haven't had a chance to look at this yet.  I'll try to get to it very soon.
Attachment #680449 - Flags: feedback?(mbrubeck)
Comment on attachment 680449 [details] [diff] [review]
Proposed patch 2 (buggy)

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

This code is looking good, but I have no idea what might cause the problem with the forward button -- at first glance it doesn't look like you've changed anything that could cause that.  I haven't had time to test this myself, but I'd try updating to the latest mozilla-central code (in case the bug was caused by something other than your patch).  If that doesn't work you try debugging the code to figure out the problem -- see whether Tab.canDoForward is called at the correct time and has the right values.
Attachment #680449 - Flags: feedback?(mbrubeck) → feedback+
Attachment #680449 - Flags: feedback+
Comment on attachment 680449 [details] [diff] [review]
Proposed patch 2 (buggy)

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

Thanks for working on this! I've commented on a few style fixes. I agree with the suggestions in comment 43; there's still a number of places where opening braces are missing spaces before them, and there's still several places where you have trailing spaces at the ends of lines. I've pointed out some of these.

::: mobile/android/base/BrowserApp.java
@@ +892,5 @@
>                  return super.onOptionsItemSelected(item);
> +        }  
> +    }
> +    
> +    /** 

Nit: Remove extra trailing spaces at the ends of these lines.

::: mobile/android/base/BrowserToolbar.java
@@ +182,5 @@
>              public void onClick(View view) {
>                  Tabs.getInstance().getSelectedTab().doBack();
>              }
>          });
> +        

Nit: Remove extra spaces on this blank line

@@ +183,5 @@
>                  Tabs.getInstance().getSelectedTab().doBack();
>              }
>          });
> +        
> +        mBack.setOnLongClickListener(new Button.OnLongClickListener(){

Nit: Add space before "{"

::: mobile/android/base/Tab.java
@@ +89,5 @@
>          mEnteringReaderMode = false;
>          mThumbnail = null;
>          mHistoryIndex = -1;
>          mHistorySize = 0;
> +        mMaxHistoryListSize=50;

Nit: Add a space before and after the "="

@@ +492,5 @@
> +        if (!canDoForward())
> +            return false;
> +        return this.showHistory(mHistoryIndex, Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize));
> +    }
> +    /**

Nit: Add extra line between "}" and the comment

@@ +496,5 @@
> +    /**
> +     * This method will show the history starting on fromIndex until toIndex of the history.
> +     */
> +    public boolean showHistory(int fromIndex, int toIndex){
> +        GeckoEvent e = GeckoEvent.createBroadcastEvent("Session:ShowHistory", ""+fromIndex+"+"+toIndex);

Nit: Add spaces before and after "+"

::: mobile/android/chrome/content/browser.js
@@ +1028,5 @@
>        browser.reload();
>      } else if (aTopic == "Session:Stop") {
>        browser.stop();
> +    } else if (aTopic == "Session:ShowHistory"){
> +        let indexes = aData.split("+");

Nit: Please use 2-space indentation for JavaScript

@@ +1137,5 @@
> +      };
> +      let index = 0;
> +      let hist = browser.sessionHistory;
> +      
> +      for(let i = toIndex; i>fromIndex; i--){

Nit: space before "(", and space before and after ">"
Brian is going to take over for Matt on mentoring this bug. It's really nice to see the progress here.
Whiteboard: [mentor=mbrubeck@mozilla.com][lang=java] → [mentor=bnicholson@mozilla.com][lang=java]
Attached patch Possible final patch (obsolete) — Splinter Review
This fix lists the history up to a specified size. Shows the back and forward history including current page, listed by descending order (most recent first).
Attachment #670196 - Attachment is obsolete: true
Attachment #680449 - Attachment is obsolete: true
This is looking good, though I noticed that we don't we don't get forward history on phones if we long press the forward button. That might be difficult to implement since we inflate our own custom menu. Rather than have separate back/forward menus for each button, what if we showed the full list for the back button, and the current page was shown as the selected element in the list?

For example, say I go to google.com, yahoo.com, then mozilla.org. I then tap the back button to go back to yahoo.com. If I hold the back button, the list could look like this:

mozilla.org [ ]
yahoo.com   [X]
google.com  [ ]

Aside from being a workaround to the forward button problem, I think this approach has the added convenience of being able to access all of your history from a single place on phones. This approach may not work as well for tablets, though, so maybe we can use a different implementation for tablets vs phones.

Could you add a third method, showAllHistory(), that we could use for phones? GeckoApp exposes an isTablet() method that you could use: http://hg.mozilla.org/integration/mozilla-inbound/file/88dbe3740665/mobile/android/base/GeckoApp.java#l1377
I don't think we need/want to have any difference between phones and tablets here. Desktop also shows the full back and forward history for both buttons. I think we should probably just follow their lead.
Christian, I think the patch you uploaded is the same as the previous one--did you maybe forget to refresh with your changes?

A tip for uploading patches: when you want somebody to take a look, please set the "review" or "feedback" flag for the attachment, and enter the name of the person you'd like to do the review. If your patch isn't ready yet but you want some comments, you can use the "feedback" flag. You can use the "review" flag if you've addressed all prior review comments and think your patch may be ready to land.
(In reply to Wesley Johnston (:wesj) from comment #52)
> I don't think we need/want to have any difference between phones and tablets
> here. Desktop also shows the full back and forward history for both buttons.
> I think we should probably just follow their lead.

The difference on phones is that we only have a dedicated back button, and getting the forward history requires going through the menu and long pressing the forward button (which is completely disconnected from the back button). I also figured there would be some issues trying to set the long press listener for the forward button given our custom menu, so a single menu seemed like a simpler workaround.

With that said: Christian, if you're able to get the forward menu button working on phones without any problems, just ignore my suggestions in comment 51.
Hi! I didn't understand very well the recommendations or what the problem is. Currently, if you keep pressed the forward or back arrows it will have the same behaviour as in the Desktop Firefox version. Also, if you keep pressed the "Back" button (the physical one) on the phone, you will also see the back history. 

Is that correct? or should it have another behaviour? 

Best regards,
(In reply to Christian Vielma (IRC :cvielma) from comment #55)
> Hi! I didn't understand very well the recommendations or what the problem
> is. Currently, if you keep pressed the forward or back arrows it will have
> the same behaviour as in the Desktop Firefox version. Also, if you keep
> pressed the "Back" button (the physical one) on the phone, you will also see
> the back history. 
> 
> Is that correct? or should it have another behaviour? 
> 
> Best regards,

Hi Christian - the issue is that long pressing the forward button on my phone doesn't bring up the history. If this is working for you, maybe I'm still using an outdated patch? (see comment 53)
Looking at your previous comments, you might be testing this only on a tablet, so let me clarify: on tablets, we show the back/forward buttons at the top, next to the URL bar. This works fine since they're next to each other; we can easily long press the back button to see back history, and we can long press the forward button to see forward history.

On phones, we do not always show the back and forward buttons on the screen. The only way to go back is to use the phone's hardware back button. The forward button is inside of the Android menu, so the only way to go forward is to first hit menu, and then hit forward. Your patch attaches onLongPress listeners in BrowserToolbar.java, which will only work for tablets. To make the current patch work on phones, you'd have to set up a long press listener in the menu (since that's where the forward button is for phones).

But instead of worrying about separate back/forward button lists for phones, I think we do want to go with the approach in comment 51 (I confirmed this with ibarlow). So here's the desired behavior:
- When you long press the back/forward buttons next to the URL bar (which we only have on tablets), it should show corresponding back/forward history lists. That's what you do now, so you should be done with this part.
- When you long press the hardware back button, though, it should show a full history list (you don't have to worry about checking phones vs tablets).

So basically, I recommend implementing an additional showAllHistory() method. When the hardware back button is long pressed, it calls this method shows a combined back/forward history view (see comment 51).

Sorry for not being more clear from the beginning! Please let me know if you need more information, and thanks again for working on this.
Where can I download a version of Firefox so I can help test this?
Thank you Brian! Now I understood the problem. Ok, I will soon implement it.

> Where can I download a version of Firefox so I can help test this?
You can check this: https://wiki.mozilla.org/Mobile/Fennec/Android

Best regards,
Attached patch Proposed patch (obsolete) — Splinter Review
I think this may be it. When pressing the back button in a phone it shows the complete history (back and forward), and when pressing the back or forward history buttons it only shows the selected.
Attachment #690251 - Attachment is obsolete: true
Attachment #695817 - Flags: review?(bnicholson)
Attachment #695817 - Flags: feedback?(bnicholson)
Comment on attachment 695817 [details] [diff] [review]
Proposed patch

Sorry for taking so long to look at this - I've been away from Bugzilla over the holidays.

This looks like the same patch you uploaded previously; you'll have to refresh your patch so it contains your newest changes. If you are using Mercurial queues, you can update your patch with the changes in your working tree by using the command "hg qrefresh".
Attachment #695817 - Flags: review?(bnicholson)
Attachment #695817 - Flags: feedback?(bnicholson)
Attached patch Proposed patch 2 (obsolete) — Splinter Review
Happy new year! It seems that is now updated, tell me if there is any problem.
Attachment #695817 - Attachment is obsolete: true
Attachment #697262 - Flags: review?(bnicholson)
Attachment #697262 - Flags: feedback?(bnicholson)
Comment on attachment 697262 [details] [diff] [review]
Proposed patch 2

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

This is looking great. The remaining work is mostly minor fixes.

::: mobile/android/base/BrowserApp.java
@@ +892,5 @@
>                  return super.onOptionsItemSelected(item);
> +        }  
> +    }
> +    
> +    /** 

Nit: Please remove trailing spaces after these lines (and elsewhere in the patch). You can see them by clicking "splinter review" next to your patch and looking for the red highlights.

@@ +899,5 @@
> +    @Override
> +    public boolean onKeyLongPress(int keyCode, KeyEvent event) {
> +        if (keyCode == KeyEvent.KEYCODE_BACK) {
> +            Tab tab = Tabs.getInstance().getSelectedTab();
> +            return tab.showAllHistory();

You'll want to check if tab is null before doing "tab.showAllHistory();" since there are cases where tabs won't yet exist (such as during some session restores).

::: mobile/android/base/Tab.java
@@ +89,5 @@
>          mEnteringReaderMode = false;
>          mThumbnail = null;
>          mHistoryIndex = -1;
>          mHistorySize = 0;
> +        mMaxHistoryListSize=50;

Nit: Please add space before and after "=".

@@ +481,5 @@
>          GeckoAppShell.sendEventToGecko(e);
>          return true;
>      }
> +    
> +    public boolean showBackHistory(){

Nit: Please add space before "{" (here and everywhere else in this patch).

@@ +498,5 @@
> +        if (!canDoForward())
> +            return false;
> +        return this.showHistory(Math.max(1, mHistoryIndex - mMaxHistoryListSize/2), Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize/2)-1, mHistoryIndex);
> +    }
> +    /**

Please add an extra line above to separate the two methods.

@@ +502,5 @@
> +    /**
> +     * This method will show the history starting on fromIndex until toIndex of the history.
> +     */
> +    public boolean showHistory(int fromIndex, int toIndex, int selIndex){
> +        GeckoEvent e = GeckoEvent.createBroadcastEvent("Session:ShowHistory", ""+fromIndex+"+"+toIndex+"+"+selIndex);

Generally, we like to use JSON to send/parse multiple arguments. You can do this by creating a new JSONObject, assigning values to its properties, and sending the stringified object.

Here's one example: http://hg.mozilla.org/integration/mozilla-inbound/file/e72009158e15/mobile/android/base/ui/SubdocumentScrollHelper.java#l90

::: mobile/android/chrome/content/browser.js
@@ +1028,5 @@
>        browser.reload();
>      } else if (aTopic == "Session:Stop") {
>        browser.stop();
> +    } else if (aTopic == "Session:ShowHistory"){
> +        let indexes = aData.split("+");

Nit: Please use 2-space indentation for JavaScript code (here and all other lines in this file).

@@ +1029,5 @@
>      } else if (aTopic == "Session:Stop") {
>        browser.stop();
> +    } else if (aTopic == "Session:ShowHistory"){
> +        let indexes = aData.split("+");
> +        dump("History indexes: "+indexes);

Debugging calls should be removed from your final patch.

@@ +1033,5 @@
> +        dump("History indexes: "+indexes);
> +        let fromIndex = parseInt(indexes[0]);
> +        let toIndex = parseInt(indexes[1]);
> +        let selIndex =parseInt(indexes[2]);
> +    	this.showHistory(fromIndex, toIndex, selIndex);

After you switch the arguments to use JSON, you can simplify this by doing:

let data = JSON.parse(aData);
this.showHistory(data.fromIndex, data.toIndex, data.selIndex);

Here's an example: http://hg.mozilla.org/integration/mozilla-inbound/file/358c8fc9b59b/mobile/android/chrome/content/browser.js#l3968

@@ +1140,5 @@
> +      };
> +      let index = 0;
> +      let hist = browser.sessionHistory;
> +      
> +      for(let i = toIndex; i>=fromIndex; i--){

Nit: Please add a space before "(" wherever a function isn't being called. Also, please add a space before and after ">=".
Attachment #697262 - Flags: review?(bnicholson)
Attachment #697262 - Flags: feedback?(bnicholson)
Attachment #697262 - Flags: feedback+
Attached patch Proposed patch 3 (obsolete) — Splinter Review
Ok, I made the changes. Please let me know if I missed something else.
Attachment #697262 - Attachment is obsolete: true
Attachment #697753 - Flags: review?(bnicholson)
Comment on attachment 697753 [details] [diff] [review]
Proposed patch 3

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

::: mobile/android/base/BrowserApp.java
@@ +899,5 @@
> +    @Override
> +    public boolean onKeyLongPress(int keyCode, KeyEvent event) {
> +        if (keyCode == KeyEvent.KEYCODE_BACK) {
> +            Tab tab = Tabs.getInstance().getSelectedTab();
> +            if(tab!=null) {

Nit: Please add a space before "(", and before and after the "!=".

@@ +910,4 @@
>      /*
>       * If the app has been launched a certain number of times, and we haven't asked for feedback before,
>       * open a new tab with about:feedback when launching the app from the icon shortcut.
>       */ 

Nit: You still have some trailing spaces throughout the patch; please click Splinter Review next to your uploaded patch and look for the red highlights to find them all.

::: mobile/android/base/BrowserToolbar.java
@@ +182,5 @@
>              public void onClick(View view) {
>                  Tabs.getInstance().getSelectedTab().doBack();
>              }
>          });
> +        

Nit: You can remove this newline to keep the mBack code together as a group.

@@ +195,5 @@
>              public void onClick(View view) {
>                  Tabs.getInstance().getSelectedTab().doForward();
>              }
>          });
> +        

Nit: You can remove this newline to keep the mForward code together as a group.

::: mobile/android/base/Tab.java
@@ +480,5 @@
>          GeckoEvent e = GeckoEvent.createBroadcastEvent("Session:Back", "");
>          GeckoAppShell.sendEventToGecko(e);
>          return true;
>      }
> +    

Nit: More trailing spaces here (and additional ones in this file - I haven't pointed them all out).

@@ +484,5 @@
> +    
> +    public boolean showBackHistory() {
> +        if (!canDoBack())
> +            return false;
> +        return this.showHistory(Math.max(mHistoryIndex - mMaxHistoryListSize, 1), mHistoryIndex, mHistoryIndex);

I don't understand the math you're using here. If we're showing back history, I thought the range would be from the beginning (0) to the selected index (mHistoryIndex). I might be overlooking something - what was your reason for the "Math.max(mHistoryIndex - mMaxHistoryListSize, 1)"?

@@ +490,5 @@
> +    
> +    public boolean showForwardHistory() {
> +        if (!canDoForward())
> +            return false;
> +        return this.showHistory(mHistoryIndex, Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize)-1, mHistoryIndex);

Similar question here. If we're showing forward history, I thought the range would be from the beginning (mHistoryIndex) to the last index (mHistorySize - 1). What is the "Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize)-1, mHistoryIndex)" doing?

@@ +495,5 @@
> +    }
> +    
> +    public boolean showAllHistory() {
> +        if (!canDoForward())
> +            return false;

We want to still show the list if they can go forward *or* back. Can you change this to "!canDoForward() && !canDoBack()"?

@@ +496,5 @@
> +    
> +    public boolean showAllHistory() {
> +        if (!canDoForward())
> +            return false;
> +        return this.showHistory(Math.max(1, mHistoryIndex - mMaxHistoryListSize/2), Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize/2)-1, mHistoryIndex);

Same question again. If we're showing all history, I thought the range would be from the beginning index (0) to the end index (mHistorySize - 1)?

@@ +508,5 @@
> +        try {
> +            json.put("fromIndex", fromIndex);
> +            json.put("toIndex", toIndex);
> +            json.put("selIndex", selIndex);
> +        }

This won't compile without a catch (or finally) block with your try statement. This can be something generic like:

} catch (JSONException e) {
    Log.e(LOGTAG, "JSON error", e);
}

::: mobile/android/chrome/content/browser.js
@@ +1028,5 @@
>        browser.reload();
>      } else if (aTopic == "Session:Stop") {
>        browser.stop();
> +    } else if (aTopic == "Session:ShowHistory") {
> +        let data = JSON.parse(aData);

Nit: Please use 2-space indentation for JS (note how the code inside this block doesn't align with the "browser.stop()" above).

@@ +1124,5 @@
>      return this.getTabForId(tabId);
> +  },
> +
> +  //This method will print a list from fromIndex to toIndex, optionally 
> +  //selecting selIndex(if fromIndex<=selIndex<=toIndex)

Nit: Please add a space after each "//".

@@ +1126,5 @@
> +
> +  //This method will print a list from fromIndex to toIndex, optionally 
> +  //selecting selIndex(if fromIndex<=selIndex<=toIndex)
> +  showHistory: function(fromIndex, toIndex, selIndex) {
> +      let browser = this.selectedBrowser;

Nit: Please use 2-space indentation for JS here and below.

@@ +1139,5 @@
> +
> +      for (let i = toIndex; i >= fromIndex; i--) {
> +          let entry = hist.getEntryAtIndex(i, false);
> +          let item = {
> +                  label:  entry.title || entry.URI.spec,

Nit: Please use 2-space indentation for this block too, e.g.:

let item = {
  label: entry.title || entry.URI.spec,
  ...
};

@@ +1146,5 @@
> +                  disabled: false,
> +                  id: i
> +          };
> +          result.listitems[index] = item;
> +          if (i==selIndex) {

Nit: Space before and after "==".
Attachment #697753 - Flags: review?(bnicholson) → feedback+
Comment on attachment 697753 [details] [diff] [review]
Proposed patch 3

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

Perfect. I think I removed all the trailing spaces and corrected all the style problems. I'm still getting used to the style conventions and fighting with Eclipse to auto-format the code with the style, I couldn't program it with JS, but I'll see what can I do about it.

Also below is explained the reason for the math.  

Please let me know if there is anything else. I didn't test this one or the last patch. I have to do it on weekend, but I think this will work (the changes since the last test where mostly style)

::: mobile/android/base/Tab.java
@@ +484,5 @@
> +    
> +    public boolean showBackHistory() {
> +        if (!canDoBack())
> +            return false;
> +        return this.showHistory(Math.max(mHistoryIndex - mMaxHistoryListSize, 1), mHistoryIndex, mHistoryIndex);

The reason for the math was suggested by Matt (https://bugzilla.mozilla.org/show_bug.cgi?id=454880#c43) Sometimes you could have a lot (a lot) of history and showing the list could take a long time in the phone, so he suggested to reduce the list to show only the last records in the list.

@@ +490,5 @@
> +    
> +    public boolean showForwardHistory() {
> +        if (!canDoForward())
> +            return false;
> +        return this.showHistory(mHistoryIndex, Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize)-1, mHistoryIndex);

Same. It shows the forward history but not complete (unless there are less than 50 records)

@@ +496,5 @@
> +    
> +    public boolean showAllHistory() {
> +        if (!canDoForward())
> +            return false;
> +        return this.showHistory(Math.max(1, mHistoryIndex - mMaxHistoryListSize/2), Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize/2)-1, mHistoryIndex);

Same as before
Attachment #697753 - Flags: feedback+
Attached patch Propose patch 4 (obsolete) — Splinter Review
Attachment #697753 - Attachment is obsolete: true
Attachment #700170 - Flags: review?(bnicholson)
Comment on attachment 700170 [details] [diff] [review]
Propose patch 4

This patch touches lots of code that isn't related to this change. You said you're using eclipse - did you perhaps tell it to reformat the code? I'd recommend going back to your previous patch and making the latest changes from there.

Thanks for explaining the logic behind the math you're using - that makes sense.

(In reply to Christian Vielma (IRC :cvielma) from comment #66)
> Comment on attachment 697753 [details] [diff] [review]
> Proposed patch 3

> ::: mobile/android/base/Tab.java
> @@ +484,5 @@
> > +    
> > +    public boolean showBackHistory() {
> > +        if (!canDoBack())
> > +            return false;
> > +        return this.showHistory(Math.max(mHistoryIndex - mMaxHistoryListSize, 1), mHistoryIndex, mHistoryIndex);

The history index is 0-based, so I think you'll want to use a max of 0 here instead of 1.

> @@ +490,5 @@
> > +    
> > +    public boolean showForwardHistory() {
> > +        if (!canDoForward())
> > +            return false;
> > +        return this.showHistory(mHistoryIndex, Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize)-1, mHistoryIndex);
> 

It looks like you're subtracting 1 here because the list is 0-based, but I think you only need to do that for mHistorySize since mHistoryIndex is already a valid index. In other words, I think this should be:

return this.showHistory(mHistoryIndex, Math.min(mHistorySize - 1, mHistoryIndex + mMaxHistoryListSize), mHistoryIndex);

> @@ +496,5 @@
> > +    
> > +    public boolean showAllHistory() {
> > +        if (!canDoForward())
> > +            return false;
> > +        return this.showHistory(Math.max(1, mHistoryIndex - mMaxHistoryListSize/2), Math.min(mHistorySize, mHistoryIndex+mMaxHistoryListSize/2)-1, mHistoryIndex);

Now I understand where the mMaxHistoryListSize/2 was coming from. One problem with this is that the list will be limited to just 25 entries if the current index is the beginning or the end of the list. Can we have a combined limit of 50, rather than having a limit of 25 for each end? For example, say I have 100 history entries, and the selected index is 90. The range would end at 100 (the number of total entries), but would start at 50.
Attachment #700170 - Flags: review?(bnicholson)
Hi Brian! Sure I'll make the change about the limits.

Just a question about mercurial, how can I go back to the previous code/patch?
(In reply to Christian Vielma (IRC :cvielma) from comment #69)
> Hi Brian! Sure I'll make the change about the limits.
> 
> Just a question about mercurial, how can I go back to the previous
> code/patch?

Patches aren't automatically version controlled, so once you do a qrefresh, there's no way to back to the previous patch version. But since you uploaded your patches to bugzilla, you can import the last good patch you submitted. Here's what I'd do:

hg qpop (to pop off the current patch)
hg qremove <patch name> (to delete the current patch)
hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=697753 (to import your proposed patch 3)
hg qpush (to push the imported patch onto the stack)

Also, if it's been awhile since you've pulled changes, your patch may no longer apply to the latest tip. Before you qpush your patch, you may want to do this:

hg pull -u (pulls the latest changes from mozilla-central and updates)

If you then get a message saying something like "update crosses branches", see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Things_that_have_changed.

If you qpush now, you'll probably see that your patch failed to apply. If you look at the .rej files that are generated, you can see the conflicts that need to be resolved. You can do this manually, or you can use a tool like wiggle (http://linux.die.net/man/1/wiggle). Once you've updated your patch to work on the latest codebase, you can do "hg qrefresh" to update your patch.
Attached patch Proposed patch 5 (obsolete) — Splinter Review
This patch fixes the things suggested in patch 3 and 4. I still have to test it because I changed the formula of calculating the history list. The rest of the changes were formatting.
Attachment #700170 - Attachment is obsolete: true
Attachment #703156 - Flags: feedback?(bnicholson)
Comment on attachment 703156 [details] [diff] [review]
Proposed patch 5

This patch has the same problem as described in comment 68 - it touches lots of code that isn't related to this bug. You said you're using Eclipse - did you perhaps tell it to reformat the code? I'd recommend going back to patch 3 (https://bugzilla.mozilla.org/attachment.cgi?id=697753) and working from there. Notice how patch 3 was only 11kB, but patches 4 and 5 are a whopping 151kB!
Attachment #703156 - Flags: feedback?(bnicholson)
Actually, patches 4 and 5 are the exact same size, so it doesn't look like patch 5 has your latest changes. Maybe you already fixed this, and you forgot to qrefresh before uploading patch 5?
One other suggestion: in the future, after uploading your patch, you could open it up in the browser to make sure it's what you expected. If you accidentally uploaded the wrong one, checking it yourself can save you some time so you don't have to wait for me to tell you.
Thank you Brian and really sorry for that. I'll try to me more cautious about that.

I'm facing a problem right now. I'm using mozilla-aurora repository to work my patch on following recommendations from another mentor (of a previous patch) to have a less changing code or less impact when landing the patch. But when I do "hg pull -u"  and use only the base code from aurora it doesn't compile. I get a lot of errors in the classes. Do you know what could it be?
(In reply to Christian Vielma (IRC :cvielma) from comment #75)
> Thank you Brian and really sorry for that. I'll try to me more cautious
> about that.
> 
> I'm facing a problem right now. I'm using mozilla-aurora repository to work
> my patch on following recommendations from another mentor (of a previous
> patch) to have a less changing code or less impact when landing the patch.
> But when I do "hg pull -u"  and use only the base code from aurora it
> doesn't compile. I get a lot of errors in the classes. Do you know what
> could it be?

There's a small chance that the build was broken when you pulled it; you can check https://tbpl.mozilla.org/?tree=Mozilla-Aurora to see the current status of the tree (if nearly all of the tests are green for a given revision, that revision should be working).

Beyond that, it's difficult to try to figure out these kind of issues over Bugzilla - I'd recommend joining the #mobile channel on IRC. Details for connecting to our IRC channel are here: https://wiki.mozilla.org/IRC#Connect_to_the_Mozilla_IRC_server. My nick on IRC is bnicholson if you'd like to chat with me directly once you're in.

It's worth getting IRC working. It's how most of us communicate with each other throughout the day, and you can ask lots of questions and get responses right away!
Blocks: 833607
OS: All → Android
Hardware: Other → ARM
Thanks! I'll do it!
Attached patch Possible final patch (obsolete) — Splinter Review
This might be the final patch. I reviewed all the syntax corrections and made additional corrections to the patch. Additionally, it was tested both in a tablet (Asus Transformer) and phone (Nexus 4). 

I changed the calculation of the history index from starting in 0 to 1 because with 0, the history starts in "about:home".

So for example, when showing the complete history, instead of calculating:
 Math.max(0, mHistoryIndex - mMaxHistoryListSize);
Now it does the following:
 Math.max(1, mHistoryIndex - mMaxHistoryListSize);
Attachment #703156 - Attachment is obsolete: true
Attachment #710518 - Flags: review?(bnicholson)
Comment on attachment 710518 [details] [diff] [review]
Possible final patch

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

(In reply to Christian Vielma (IRC :cvielma) from comment #78)
> Created attachment 710518 [details] [diff] [review]
> Possible final patch
>
> I changed the calculation of the history index from starting in 0 to 1
> because with 0, the history starts in "about:home".
> 

I think showing about:home in the list is the desired behavior. After all, about:home is just another page in the history! Also, going to about:home, then to another page, then clicking back will bring you back to about:home - I think we want the list of pages to correspond to what would happen if you were to click back/forward.

Overall, this is looking good. r- for fixing the index as described above, and also for fixing the canDoForward() check and the index math (below):

::: mobile/android/base/Tab.java
@@ +42,5 @@
>      private BitmapDrawable mThumbnail;
>      private int mHistoryIndex;
>      private int mHistorySize;
> +    private final int mParentId;
> +    private final boolean mExternal;

Nit: Leave these non-final

@@ +53,5 @@
>      private ZoomConstraints mZoomConstraints;
> +    private final ArrayList<View> mPluginViews;
> +    private final HashMap<Object, Layer> mPluginLayers;
> +    private final ContentResolver mContentResolver;
> +    private final ContentObserver mContentObserver;

Nit: Leave these non-final

@@ +59,5 @@
>      private int mState;
>      private Bitmap mThumbnailBitmap;
>      private boolean mDesktopMode;
>      private boolean mEnteringReaderMode;
> +    private final int mMaxHistoryListSize;

Nit: Please make this a final static int, name it MAX_HISTORY_LIST_SIZE, and initialize it here instead of in the constructor. This is the general convention for "magic numbers"; see this for other examples: http://mxr.mozilla.org/mozilla-central/search?string=static+final+int&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

@@ +431,5 @@
> +        if (!canDoBack())
> +            return false;
> +        return this.showHistory(
> +                Math.max(mHistoryIndex - mMaxHistoryListSize, 1),
> +                mHistoryIndex, mHistoryIndex);

Nit: Please combine these to be on a single line

@@ +442,5 @@
> +    }
> +
> +    public boolean showAllHistory() {
> +        if (!canDoForward())
> +            return false;

We want to still show the list if they can go forward *or* back. Please change this to "!canDoForward() && !canDoBack()"

@@ +444,5 @@
> +    public boolean showAllHistory() {
> +        if (!canDoForward())
> +            return false;
> +        int min = Math.max(1, mHistoryIndex - mMaxHistoryListSize);
> +        int max = Math.min(mHistorySize -1, mHistoryIndex +mMaxHistoryListSize - (mHistoryIndex-min));

I'm not sure if this math is correct. Let's say you have a list size of 100 and a current index of 90. This means:
min = max(1, 90 - 50) = 40
max = min(100 - 1, 90 + 50 - (50 - 40)) = 99

So you end up with a min of 40 and a max of 99, which is greater than the max list size of 50. I think this algorithm could work:

min = index - MAX_HISTORY_LIST_SIZE / 2
max = index + MAX_HISTORY_LIST_SIZE / 2
if min < 0:
    max -= min

if max > mHistorySize - 1:
    min -= max - (mHistorySize - 1)
    max = mHistorySize - 1

min = Math.max(min, 0)


Also nit: Please use consistent spacing around arithmetic symbols; generally, we use a single space both before and after each symbol.

::: mobile/android/chrome/content/browser.js
@@ +1270,5 @@
> +        isGroup: false,
> +        inGroup: false,
> +        disabled: false,
> +        id: i
> +        };

Nit: Move "};" two spaces to the left

@@ +1271,5 @@
> +        inGroup: false,
> +        disabled: false,
> +        id: i
> +        };
> +      result.listitems[index] = item;

Rather than keeping track of a separate index, I would just do this:

result.listitems.push(item);

@@ +1276,5 @@
> +      if (i == selIndex) {
> +          result.selected[index] = true;
> +      } else {
> +          result.selected[index] = false;
> +      }

And you can change these 5 lines to just be:

result.selected.push(i == selIndex);

@@ +1279,5 @@
> +          result.selected[index] = false;
> +      }
> +      index++;
> +    }
> +    let data = JSON.parse(sendMessageToJava({ gecko: result }));

We've recently removed the gecko property from sendMessageToJava messages. You should change this to:

let data = JSON.parse(sendMessageToJava(result));

Note that you'll have to update your source tree again for this change to work.
Attachment #710518 - Flags: review?(bnicholson) → review-
Comment on attachment 710518 [details] [diff] [review]
Possible final patch

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

::: mobile/android/base/Tab.java
@@ +444,5 @@
> +    public boolean showAllHistory() {
> +        if (!canDoForward())
> +            return false;
> +        int min = Math.max(1, mHistoryIndex - mMaxHistoryListSize);
> +        int max = Math.min(mHistorySize -1, mHistoryIndex +mMaxHistoryListSize - (mHistoryIndex-min));

Yes, the math seems correct to me. There is an error in your example it should be:

min = max(1, 90 - 50) = 40
max = min(100 - 1, 90 + 50 - (90 - 40)) = 90.

Let me know

::: mobile/android/chrome/content/browser.js
@@ +1276,5 @@
> +      if (i == selIndex) {
> +          result.selected[index] = true;
> +      } else {
> +          result.selected[index] = false;
> +      }

Thanks!
Attached patch Possible final patch (obsolete) — Splinter Review
This might be it. I noticed that eclipse again played against me and changed some things, so I corrected and also corrected the things indicated. 

Ah I forgot, in the Desktop Firefox you can't go back to "about:home" so that's why I thought that it shouldn't be displayed in the list. I changed it so it shows it now.
Attachment #710518 - Attachment is obsolete: true
Attachment #711128 - Flags: review?(bnicholson)
(In reply to Christian Vielma (IRC :cvielma) from comment #80)
> Comment on attachment 710518 [details] [diff] [review]
> Possible final patch
> 
> Review of attachment 710518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Tab.java
> @@ +444,5 @@
> > +    public boolean showAllHistory() {
> > +        if (!canDoForward())
> > +            return false;
> > +        int min = Math.max(1, mHistoryIndex - mMaxHistoryListSize);
> > +        int max = Math.min(mHistorySize -1, mHistoryIndex +mMaxHistoryListSize - (mHistoryIndex-min));
> 
> Yes, the math seems correct to me. There is an error in your example it
> should be:
> 
> min = max(1, 90 - 50) = 40
> max = min(100 - 1, 90 + 50 - (90 - 40)) = 90.
> 
> Let me know
> 

Oops, I used mMaxHistoryListSize instead of mHistoryIndex. So the limit (50) is correct - but it doesn't show any forward entries!

I think we should try to show an even amount of each - 25 forward and 25 back entries - if we can. But if there are fewer entries than that (in this case, there's only 10 forward entries), we can add the surplus to the other side. So if we have 100 entries and the index is 90, the range would start at 50 and end at 100 (or 99 if we adjust for 0-based indexing). If the index is 50, the range would be 25 through 75. If the index is 5, the range would be 0 through 49.
Comment on attachment 711128 [details] [diff] [review]
Possible final patch

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

This looks great. r+ with nits fixed and adjusted math (see comment 82 and algorithm in comment 79).

::: mobile/android/base/Tab.java
@@ +429,5 @@
> +    public boolean showBackHistory() {
> +        if (!canDoBack())
> +            return false;
> +        return this.showHistory(
> +                Math.max(mHistoryIndex - MAX_HISTORY_LIST_SIZE, 0), mHistoryIndex, mHistoryIndex);

Nit: Please combine this return statement into one line

::: mobile/android/chrome/content/browser.js
@@ +1260,5 @@
> +      multiple: false,
> +      selected: [],
> +      listitems: []
> +    };
> +    let index = 0;

This index variable can be removed now

@@ +1273,5 @@
> +        id: i
> +      };
> +      result.listitems.push(item);
> +      result.selected.push(i == selIndex);
> +      index++;

remove this
Attachment #711128 - Flags: review?(bnicholson) → review-
Attached patch Possible final patch (obsolete) — Splinter Review
Corrected! ;)
Attachment #711128 - Attachment is obsolete: true
Attachment #711142 - Flags: review?(bnicholson)
Comment on attachment 711142 [details] [diff] [review]
Possible final patch

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

Looks good. Thanks for working through this! I'm looking forward to this feature.

::: mobile/android/base/Tab.java
@@ +439,5 @@
> +    }
> +
> +    public boolean showAllHistory() {
> +        if (!canDoForward() && !canDoBack())
> +            return false;

Nit: newline after this return
Attachment #711142 - Flags: review?(bnicholson) → review+
Done! ;) No problem... in fact I feel bad about not finishing it sooner and all the little errors I have had :(

Looking forward to see this patch landing soon :)
Attachment #711142 - Attachment is obsolete: true
Attachment #711143 - Flags: review?(bnicholson)
Awesome!

If you feel like rollin' onwards while this is fresh in your mind, you can take a look at Bug 833607 :D
Attachment #711143 - Flags: review?(bnicholson) → review+
Target Milestone: --- → Firefox 21
https://hg.mozilla.org/mozilla-central/rev/993712f9b175
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Am I reading this bug wrong? Because holding back or forward isn't giving me a list of the tab's recently visited page in the nightly build for 20130207
Flags: in-testsuite?
Flags: in-moztrap?(fennec)
(In reply to Paul [sabret00the] from comment #90)
> Am I reading this bug wrong? Because holding back or forward isn't giving me
> a list of the tab's recently visited page in the nightly build for 20130207

I dont see the change-set in today's branded Nightly build; check tomorrow.
Thanks again for your work (and persistence) here, cvielma!
No problem at all! I'm happy to help. Matt and Brian, if any of you live near Miami I would invite you a beer ;)
Blocks: 839463
Pressing and holding forward does not bring up history on HTC One X (Android 2.4.1/Sense 4+).
(In reply to Paul [sabret00the] from comment #94)
> Pressing and holding forward does not bring up history on HTC One X (Android
> 2.4.1/Sense 4+).

On the menu? I don't think this patch affects the menu. Only the URL bar buttons (back and forward in the tablet UI) and the BACK button.
(In reply to Paul [sabret00the] from comment #94)
> Pressing and holding forward does not bring up history on HTC One X (Android
> 2.4.1/Sense 4+).

You meant Android 4.1.2.
Depends on: 839492
(In reply to Peter Henkel [:Terepin] from comment #96)
> (In reply to Paul [sabret00the] from comment #94)
> > Pressing and holding forward does not bring up history on HTC One X (Android
> > 2.4.1/Sense 4+).
> 
> You meant Android 4.1.2.

Indeed I do, thanks for the catch.
(In reply to Paul [sabret00the] from comment #97)

I'm not clear: are you experiencing any problem with this patch?
(In reply to Christian Vielma (IRC :cvielma) from comment #98)
> (In reply to Paul [sabret00the] from comment #97)
> 
> I'm not clear: are you experiencing any problem with this patch?

Yup, I can't jump forward.
Using the URL bar buttons or the Back button?  If you press the back button are you able to see forward history?
From the back button I can access forward and back history. However from the forward button that's in the menu on the URL bar, I'm unable to bring up the history.
No, it doesn't work there as said by Mark (mfinkle) in comment #95. Only in the URL bar buttons and back button. That would be a new requirement.
(In reply to Christian Vielma (IRC :cvielma) from comment #102)
> No, it doesn't work there as said by Mark (mfinkle) in comment #95. Only in
> the URL bar buttons and back button. That would be a new requirement.

So does this only work on a tablet then? 

Can I test this currently in nightly?
(In reply to donrhummy from comment #103)
> (In reply to Christian Vielma (IRC :cvielma) from comment #102)
> > No, it doesn't work there as said by Mark (mfinkle) in comment #95. Only in
> > the URL bar buttons and back button. That would be a new requirement.
> 
> So does this only work on a tablet then? 

Long press on "Forward" only works on tablets. Long press on the BACK hardware button also shows the list.

> Can I test this currently in nightly?

Yes
I tested it on a tablet (Nexus 7) and it appeared to work perfectly.

On phone, there needs to be some way to make it clear to people that the hardware back button has that functionality. Can you change the image used there (this is allowed in Android)? Otherwise, it's and inconsistent UI.
(In reply to donrhummy from comment #105)
> I tested it on a tablet (Nexus 7) and it appeared to work perfectly.
> 
> On phone, there needs to be some way to make it clear to people that the
> hardware back button has that functionality. Can you change the image used
> there (this is allowed in Android)? Otherwise, it's and inconsistent UI.

It's an inconsistent UI regardless because we have completely different layouts for tablets and phones. We don't have the ability to change the on-screen back button at the bottom of phones like the Nexus, and even if we could, we'd have this issue for phones with hardware back buttons.

I agree that discoverability is an issue (as are many of our long press interactions). Hopefully, we can eventually address these with some kind of introductory helper feature.
It's also a nascent cultural stereotype. As more apps add long-press functionality, it'll start to be expected, like context menus.
(In reply to Christian Vielma (IRC :cvielma) from comment #102)
> No, it doesn't work there as said by Mark (mfinkle) in comment #95. Only in
> the URL bar buttons and back button. That would be a new requirement.

Ah, totally missed Mark's comment.


(In reply to Mark Finkle (:mfinkle) from comment #104)
> Long press on "Forward" only works on tablets. Long press on the BACK
> hardware button also shows the list.

Is there any particular reason to disable on phones?
(In reply to Brian Nicholson (:bnicholson) from comment #106)
> (In reply to donrhummy from comment #105)
> > I tested it on a tablet (Nexus 7) and it appeared to work perfectly.
> > 
> > On phone, there needs to be some way to make it clear to people that the
> > hardware back button has that functionality. Can you change the image used
> > there (this is allowed in Android)? Otherwise, it's and inconsistent UI.
> 
> It's an inconsistent UI regardless because we have completely different
> layouts for tablets and phones. We don't have the ability to change the
> on-screen back button at the bottom of phones like the Nexus, and even if we
> could, we'd have this issue for phones with hardware back buttons.

No, I didn't mean inconsistent across phones and tablets, I meant inconsistent within the phone. Because:

1. The Firefox forward button contains a long-press action
2. The Firefox back button does not contain a long press action (instead it's in the OS back button)

That's inconsistent and confusing. You can have the OS back button have the functionality, but the firefox back button should also have it.
No, in tablets Back and Forward buttons contains long press actions. In phones only the OS back button because the Back and Forward buttons are not shown.
Attachment #347184 - Attachment is obsolete: true
Hi, I haven't seen this functionality in Beta nor the official Firefox. Has it been deployed?
(In reply to Christian Vielma (IRC :cvielma) from comment #111)
> Hi, I haven't seen this functionality in Beta nor the official Firefox. Has
> it been deployed?

The target milestone field is Firefox 21, which is currently Aurora.
Blocks: 847435
Verified on Firefox for Android 21.0b6 using: Nexus 4 (4.2.2) and Samsung Galaxy Tab (4.0.4)
Status: RESOLVED → VERIFIED
Depends on: 1048969
Depends on: 1048970
No longer depends on: 1048969
You need to log in before you can comment on or make changes to this bug.