So that we can edit URL without having to enter awesome screen.

This patch changes a two fundamental things in our UI: - Adds an editing mode to browser toolbar - Awesomescreen is now handled in BrowserApp instead of the separate AwesomeBar activity Entering URLs for current and new tabs seems to be working fine. The actual history/bookmarks search is not implemented yet as it needs Brian's patch to land first. Known issues: - The animation to expand the entry is a bit janky right now because we're inflating AboutHome at the same time. We can fix this once the HomePager stuff from Brian lands. - No handling of search keys. This can only be done once we move the search result filtering into AboutHome. - Code to handle clipboard actionbar has been commented out for now. Needs work. And, of course, all the regressions that a major change like this will probably bring :-) I'm requesting review from mfinkle and feedback from sriram to get more eyes on this new code. Drive-bys are welcome!

Comment on attachment 747499 [details] [diff] [review] Implement toolbar editing mode and move awesome seach into abouthome Review of attachment 747499 [details] [diff] [review]: ----------------------------------------------------------------- I didn't have a chance to look through this all very closely, but two drive-bys came to mind while looking through it. ::: mobile/android/base/BrowserApp.java @@ +1084,5 @@ > void updateAboutHomeTopSites() { > mAboutHome.update(EnumSet.of(AboutHome.UpdateFlags.TOP_SITES)); > } > > + public boolean showAwesomebar(BrowserToolbar.Target target) { To bikeshed on naming, I don't know if refering to things as "Awesomebar" makes sense anymore. What's really happening is we're entering an editing mode and showing about:home at the same time. Maybe we could refer to this state as "AwesomeMode" call this method "enterAwesomeMode" or something like that. ::: mobile/android/base/GeckoApp.java @@ -602,5 @@ > - url = ReaderModeUtils.getUrlFromAboutReader(url); > - > - GeckoAppShell.openUriExternal(url, "text/plain", "", "", > - Intent.ACTION_SEND, tab.getDisplayTitle()); > - } Is there a reason for moving this logic as part of this patch. I'm definitely in favor of moving things out of GeckoApp if they don't need to be here, but I don't want us to move around more stuff than we need to in one patch. -my inner lucasr

More issues worth noting: - Tapping the AwesomeBar and then clicking a "tab from last time" (or clicking an external shortcut outside of Fennec) opens a new tab but keeps the AwesomeBar in edit mode. - While on about:home, tapping the AwesomeBar, entering text, then clicking back doesn't restore the title. - Tapping the AwesomeBar and touching the "home" view doesn't take the focus away from the AwesomeBar. - Tapping the AwesomeBar and then hitting back hides the keyboard, but it doesn't leave the home view and a second back press is required. The current Nightly hides the AwesomeScreen with a single back press.

(In reply to Brian Nicholson (:bnicholson) from comment #3) Thanks, I'll try to cover some of these in the first pass (at least the ones that don't depend on the new homepager bits) and file follow-up for the remaining. > More issues worth noting: > - Tapping the AwesomeBar and then clicking a "tab from last time" (or > clicking an external shortcut outside of Fennec) opens a new tab but keeps > the AwesomeBar in edit mode. > - While on about:home, tapping the AwesomeBar, entering text, then clicking > back doesn't restore the title. I think I can fix those two right away. > - Tapping the AwesomeBar and touching the "home" view doesn't take the focus > away from the AwesomeBar. This will probably have to be done in homepager. I'll file a follow-up. > - Tapping the AwesomeBar and then hitting back hides the keyboard, but it > doesn't leave the home view and a second back press is required. The current > Nightly hides the AwesomeScreen with a single back press. I asked ibarlow about this and he recommended the behavior in the patch (dismiss keyboard then dismiss awesomebar). I actually don't really agree with the current behaviour (in Nightly) because it's inconsistent with Android in general.

(In reply to :Margaret Leibovic from comment #2) Thanks for the comments. > ::: mobile/android/base/BrowserApp.java > @@ +1084,5 @@ > > void updateAboutHomeTopSites() { > > mAboutHome.update(EnumSet.of(AboutHome.UpdateFlags.TOP_SITES)); > > } > > > > + public boolean showAwesomebar(BrowserToolbar.Target target) { > > To bikeshed on naming, I don't know if refering to things as "Awesomebar" > makes sense anymore. What's really happening is we're entering an editing > mode and showing about:home at the same time. Maybe we could refer to this > state as "AwesomeMode" call this method "enterAwesomeMode" or something like > that. Good points. enterAwesomeMode might be a better option. > ::: mobile/android/base/GeckoApp.java > @@ -602,5 @@ > > - url = ReaderModeUtils.getUrlFromAboutReader(url); > > - > > - GeckoAppShell.openUriExternal(url, "text/plain", "", "", > > - Intent.ACTION_SEND, tab.getDisplayTitle()); > > - } > > Is there a reason for moving this logic as part of this patch. I'm > definitely in favor of moving things out of GeckoApp if they don't need to > be here, but I don't want us to move around more stuff than we need to in > one patch. -my inner lucasr You're absolutely right :-) I'll factor out the changes that are just about moving stuff from GeckoApp to BrowserApp into a separate patch.

Comment on attachment 747499 [details] [diff] [review] Implement toolbar editing mode and move awesome seach into abouthome Review of attachment 747499 [details] [diff] [review]: ----------------------------------------------------------------- Overall, the changes look good to me. I could see most of the code copied from AwesomeBar, and things should work fine. Couple of suggestions: 1. I feel a lot of code related to AwesomeBar's EditText is pushed into BrowserToolbar. I would probably move it to a custom widget and handle it there. That would make the BrowserToolbar light. public class UrlBarEditEdit extends CustomEditText { .. add all callbacks here .. https://bugzilla.mozilla.org/attachment.cgi?id=747499&action=diff#a/mobile/android/base/BrowserToolbar.java_sec5 } 2. Moving the display related to AwesomeBar into a separate widget would be good too. 3. Removing the (redundant) "AwesomeBar" name everywhere (as mfinkle wants it ;) ). 4. Removing the dependency on "mActivity" in BrowserToolbar and exposing interfaces from BrowserToolbar that BrowserApp can listen to. ::: mobile/android/base/AwesomeBar.java @@ -48,5 @@ > > -interface AutocompleteHandler { > - void onAutocomplete(String res); > -} > - Given that this file is not changed, and is going to be removed altogether, I wouldn't do this change. This is inside AwesomeBar and wouldn't interfere with the pulled out one. ::: mobile/android/base/BrowserApp.java @@ +294,5 @@ > > + if (mBrowserToolbar.onKey(keyCode, event)) { > + return true; > + } > + I have doubts here. If I'm right, onKey() gives it to layer view first -- which we don't want to do when we have the AwesomeBar in editing mode. Probably there should be a check on "is in editing mode?" and change the order of using the key stroke. Oh! And, please check it with keyboard shortcuts too :) @@ +499,5 @@ > } > > @Override > + public boolean onSearchRequested() { > + return showAwesomebar(BrowserToolbar.Target.CURRENT_TAB); Agreeing with Margaret on "AwesomeBar" naming. @@ +528,5 @@ > + shareCurrentUrl(); > + return true; > + } > + case R.id.subscribe: { > + Tab tab = Tabs.getInstance().getSelectedTab(); I see you had copied this block from AwesomeBar as-is. But, could you clean this up a bit? "getSelectedTab()" is done for 3 cases, that can be factored out. @@ +1088,5 @@ > + public boolean showAwesomebar(BrowserToolbar.Target target) { > + return showAwesomebar(target, null); > + } > + > + public boolean showAwesomebar(BrowserToolbar.Target target, String url) { Shall we give "showAwesomebar" package access instead of public? It's used with BrowserApp and BrowserToolbar only. @@ +1143,5 @@ > + } > + > + private void animateShowAboutHome() { > + showAboutHome(true); > + } Do we need this extra method? I guess we can have "showAboutHome(boolean animate)" alone. But then, the name should change a bit. "showAboutHome(boolean)" makes me feel the toggling is on the show/hide and not on the animation. @@ +1172,5 @@ > // happens, using commit() would throw an IllegalStateException since > // it can't be used between the Activity's onSaveInstanceState() and > // onResume(). > getSupportFragmentManager().beginTransaction() > + .setCustomAnimations((animate ? R.anim.awesomebar_fade_in : 0), 0) I know the "0" is for specify no animations (or null resource). But the code gets cryptic with these zer0es. Could you add a final int "NO_ANIMATION" to the activity and use it here. It's just for making the code readability better. @@ +1179,5 @@ > > mBrowserToolbar.setNextFocusDownId(R.id.abouthome_content); > } > > + private void animateHideAboutHomeIfPossible() { The "IfPossible" part isn't needed actually. We could have this piece of code inside hideAboutHome(boolean). @@ +1181,5 @@ > } > > + private void animateHideAboutHomeIfPossible() { > + final Tab tab = Tabs.getInstance().getSelectedTab(); > + if (tab != null && !"about:home".equals(tab.getURL())) { I would recommend having "about:home" string as a final String with the activity. And, if getURL() could return a null, then it's better to use TextUtils. @@ +1194,5 @@ > private void hideAboutHome() { > + hideAboutHome(false); > + } > + > + private void hideAboutHome(boolean animate) { Could you rename it to hideAboutHomeWithAnimation(boolean animate)? That would make the boolean distinguish if the animation is needed on not. hideAboutHome(boolean) makes it feel the boolean is for hide/show and not the animation. Same could be applied for showAboutHome() too. ::: mobile/android/base/BrowserToolbar.java @@ +75,5 @@ > private static final String LOGTAG = "GeckoToolbar"; > public static final String PREFS_NAME = "BrowserToolbar"; > public static final String PREFS_SHOW_URL = "ShowUrl"; > + > + public static enum Target { NEW_TAB, CURRENT_TAB, PICK_SITE }; AwesomeBar.Target made sense as we know what the activity is for. BrowserToolbar.Target feels blunt. We might want to have a different name here. "UrlBarTarget" may be. @@ +85,5 @@ > private ImageView mAwesomeBarRightEdge; > private BrowserToolbarBackground mAddressBarBg; > private GeckoTextView mTitle; > + private View mAwesomeBarEdit; > + private CustomEditText mAwesomeBarText; I got confused of these two names and had to look into the XML to find what's what. Could we name the container and the editable view in a way that can be recognized? Something like, "mEditableContainer", "mEditText". Or, "mUrlEditContainer", "mUrlEditText". @@ +105,5 @@ > private GeckoImageView mMenuIcon; > private LinearLayout mActionItemBar; > private MenuPopup mMenuPopup; > private List<View> mFocusOrder; > + private Target mEditingTarget; A smaller name, may be. mTarget? @@ +193,5 @@ > > mLayout.setOnClickListener(new Button.OnClickListener() { > @Override > public void onClick(View v) { > + mActivity.showAwesomebar(Target.CURRENT_TAB); I like passing the event to BrowserApp so that I can do required checks before making the BrowserToolbar editable. But, I somehow doesn't seem to like "mActivity.showAwesomeBar()" these days (please don't do a blame on old code :( ). How about exposing an interface "OnBrowserToolbarClickListener" and, BrowserApp implementing it? That removes the dependency on mActivity. And BrowserApp knows about BrowserToolbar anyways. @@ +257,5 @@ > + // If the AwesomeBar has a composition string, don't submit the text yet. > + // ENTER is needed to commit the composition string. > + Editable content = mAwesomeBarText.getText(); > + if (!hasCompositionString(content)) { > + mActivity.commitAwesomebar(mEditingTarget); Same as "OnBrowserToolbarClickListener". May be we should expose a "OnBrowserToolbarCommitListener" and BrowserApp should implement it. @@ +325,5 @@ > + if (Build.VERSION.SDK_INT >= 11 && selStart == selEnd) { > + // mActivity.getActionBar().hide(); > + } > + } > + }); There are 2 patterns I'm seeing in Android codebase. 1. Specific private classes are made to implement interfaces, and a class instance is reused. https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/ShareActionProvider.java#L297 Hint: Kats uses a similar approach with GamepadUtils ;) 2. A bunch of listeners are implemented by a class, called Callback, and an instance of it is used by a view. https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/ActivityChooserView.java#L525 We should probably use the Callbacks approach here. This would make the methods small, and code clean. (And I vaguely remember that it helps with thread concurrency -- I might be wrong). @@ +445,5 @@ > + public void onClick(View v) { > + mActivity.commitAwesomebar(mEditingTarget); > + } > + }); > + This is same as the OnBrowserToolbarCommitListener that I mentioned above. @@ +969,2 @@ > visible &= !(url == null || (url.startsWith("about:") && > + !url.equals("about:blank"))) && !isEditing(); These strings could be made final and used here. @@ +1082,5 @@ > animator.start(); > } > > + private void showEditText() { > + mAwesomeBarContent.setVisibility(View.GONE); mAwesomeBarContent could be renamed to "mUrlBarDisplayContainer". This would make us distinguish between mUrlBarEditContainer and mUrlBarDisplayContainer. @@ +1114,5 @@ > + mAwesomeBarText.setText(url != null ? url : ""); > + > + // This animation doesn't make much sense in a sidebar UI > + if (HardwareUtils.isTablet()) { > + mLayout.setSelected(true); Do we need this? mAwesomeBarEdit already has focus now. I thought this piece was needed for showing the orange outline during animation. ::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml @@ +68,5 @@ > android:src="@drawable/ic_menu_back" > android:contentDescription="@string/back" > android:background="@drawable/address_bar_nav_button"/> > > + <LinearLayout android:id="@+id/awesome_bar_edit" It's better to call it "url_bar_edit_container". @@ +79,5 @@ > + android:layout_toRightOf="@id/back" > + android:layout_toLeftOf="@id/menu_items"> > + > + <view class="org.mozilla.gecko.CustomEditText" > + android:id="@+id/awesome_bar_text" This could better be "url_bar_edit_text". ::: mobile/android/base/widget/AboutHome.java @@ +6,5 @@ > package org.mozilla.gecko.widget; > > import java.util.EnumSet; > > +import org.mozilla.gecko.AutocompleteHandler; Is this needed?

I'm guessing you might be making a new patch based on the feedback so far. I'll wait for the next patch.

(In reply to Sriram Ramasubramanian [:sriram] from comment #6) Thanks for the detailed feedback, Sriram! > Comment on attachment 747499 [details] [diff] [review] > Implement toolbar editing mode and move awesome seach into abouthome > > Review of attachment 747499 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall, the changes look good to me. I could see most of the code copied > from AwesomeBar, and things should work fine. > > Couple of suggestions: > 1. I feel a lot of code related to AwesomeBar's EditText is pushed into > BrowserToolbar. I would probably move it to a custom widget and handle it > there. > That would make the BrowserToolbar light. > > public class UrlBarEditEdit extends CustomEditText { > .. add all callbacks here .. > https://bugzilla.mozilla.org/attachment.cgi?id=747499&action=diff#a/mobile/ > android/base/BrowserToolbar.java_sec5 > } Agree. Let's land this patch as is just to get things started though. I filed bug 871522 to track this. I won't be just about moving code around because the callbacks deal with stuff in BrowserToolbar e.g. the 'go' button. Maybe it will be better to turn the layout containing the EditText and the go Button to a custom view (as opposed to just the EditText part). > 2. Moving the display related to AwesomeBar into a separate widget would be > good too. What do you mean by "display related to AwesomeBar"? > 3. Removing the (redundant) "AwesomeBar" name everywhere (as mfinkle wants > it ;) ). I'll probably go with EditingMode for now. > 4. Removing the dependency on "mActivity" in BrowserToolbar and exposing > interfaces from BrowserToolbar that BrowserApp can listen to. Done. > ::: mobile/android/base/AwesomeBar.java > @@ -48,5 @@ > > > > -interface AutocompleteHandler { > > - void onAutocomplete(String res); > > -} > > - > > Given that this file is not changed, and is going to be removed altogether, > I wouldn't do this change. This is inside AwesomeBar and wouldn't interfere > with the pulled out one. AutoCompleteHandler is now used by AboutHome which is not in the org.mozilla.gecko package. > ::: mobile/android/base/BrowserApp.java > @@ +294,5 @@ > > > > + if (mBrowserToolbar.onKey(keyCode, event)) { > > + return true; > > + } > > + > > I have doubts here. If I'm right, onKey() gives it to layer view first -- > which we don't want to do when we have the AwesomeBar in editing mode. > Probably there should be a check on "is in editing mode?" and change the > order of using the key stroke. Nice catch. Added an isEditing() check. Not sure what you mean with "change the order of using the key stroke" though. > Oh! And, please check it with keyboard shortcuts too :) Ditto. What do you mean? > @@ +499,5 @@ > > } > > > > @Override > > + public boolean onSearchRequested() { > > + return showAwesomebar(BrowserToolbar.Target.CURRENT_TAB); > > Agreeing with Margaret on "AwesomeBar" naming. I'll go with enterEditingMode for now. > @@ +528,5 @@ > > + shareCurrentUrl(); > > + return true; > > + } > > + case R.id.subscribe: { > > + Tab tab = Tabs.getInstance().getSelectedTab(); > > I see you had copied this block from AwesomeBar as-is. But, could you clean > this up a bit? "getSelectedTab()" is done for 3 cases, that can be factored > out. > > @@ +1088,5 @@ > > + public boolean showAwesomebar(BrowserToolbar.Target target) { > > + return showAwesomebar(target, null); > > + } > > + > > + public boolean showAwesomebar(BrowserToolbar.Target target, String url) { > > Shall we give "showAwesomebar" package access instead of public? It's used > with BrowserApp and BrowserToolbar only. Agree. Done. > @@ +1143,5 @@ > > + } > > + > > + private void animateShowAboutHome() { > > + showAboutHome(true); > > + } > > Do we need this extra method? I guess we can have "showAboutHome(boolean > animate)" alone. > > But then, the name should change a bit. "showAboutHome(boolean)" makes me > feel the toggling is on the show/hide and not on the animation. Boolean arguments are cryptic. I could add a flags argument there but that would be a bit overkill. This extra method is just a readability aid. > @@ +1172,5 @@ > > // happens, using commit() would throw an IllegalStateException since > > // it can't be used between the Activity's onSaveInstanceState() and > > // onResume(). > > getSupportFragmentManager().beginTransaction() > > + .setCustomAnimations((animate ? R.anim.awesomebar_fade_in : 0), 0) > > I know the "0" is for specify no animations (or null resource). > But the code gets cryptic with these zer0es. Could you add a final int > "NO_ANIMATION" to the activity and use it here. It's just for making the > code readability better. Fine, done. > @@ +1179,5 @@ > > > > mBrowserToolbar.setNextFocusDownId(R.id.abouthome_content); > > } > > > > + private void animateHideAboutHomeIfPossible() { > > The "IfPossible" part isn't needed actually. We could have this piece of > code inside hideAboutHome(boolean). Good point. I wasn't sure if that was the expected behaviour in every case. I've just double-checked it and it seems safe do move this check inside hideAboutHome(). Done. > @@ +1181,5 @@ > > } > > > > + private void animateHideAboutHomeIfPossible() { > > + final Tab tab = Tabs.getInstance().getSelectedTab(); > > + if (tab != null && !"about:home".equals(tab.getURL())) { > > I would recommend having "about:home" string as a final String with the > activity. And, if getURL() could return a null, then it's better to use > TextUtils. Done. > @@ +1194,5 @@ > > private void hideAboutHome() { > > + hideAboutHome(false); > > + } > > + > > + private void hideAboutHome(boolean animate) { > > Could you rename it to hideAboutHomeWithAnimation(boolean animate)? That > would make the boolean distinguish if the animation is needed on not. > hideAboutHome(boolean) makes it feel the boolean is for hide/show and not > the animation. > > Same could be applied for showAboutHome() too. Done. > ::: mobile/android/base/BrowserToolbar.java > @@ +75,5 @@ > > private static final String LOGTAG = "GeckoToolbar"; > > public static final String PREFS_NAME = "BrowserToolbar"; > > public static final String PREFS_SHOW_URL = "ShowUrl"; > > + > > + public static enum Target { NEW_TAB, CURRENT_TAB, PICK_SITE }; > > AwesomeBar.Target made sense as we know what the activity is for. > BrowserToolbar.Target feels blunt. We might want to have a different name > here. "UrlBarTarget" may be. I'm go with EditingTarget for now. > > @@ +85,5 @@ > > private ImageView mAwesomeBarRightEdge; > > private BrowserToolbarBackground mAddressBarBg; > > private GeckoTextView mTitle; > > + private View mAwesomeBarEdit; > > + private CustomEditText mAwesomeBarText; > > I got confused of these two names and had to look into the XML to find > what's what. > Could we name the container and the editable view in a way that can be > recognized? > Something like, "mEditableContainer", "mEditText". Or, "mUrlEditContainer", > "mUrlEditText". Makes sense. Went with mUrlEditContainer and mUrlEditText. > > @@ +105,5 @@ > > private GeckoImageView mMenuIcon; > > private LinearLayout mActionItemBar; > > private MenuPopup mMenuPopup; > > private List<View> mFocusOrder; > > + private Target mEditingTarget; > > A smaller name, may be. mTarget? I prefer being explicit about the context of this target. > @@ +193,5 @@ > > > > mLayout.setOnClickListener(new Button.OnClickListener() { > > @Override > > public void onClick(View v) { > > + mActivity.showAwesomebar(Target.CURRENT_TAB); > > I like passing the event to BrowserApp so that I can do required checks > before making the BrowserToolbar editable. > > But, I somehow doesn't seem to like "mActivity.showAwesomeBar()" these days > (please don't do a blame on old code :( ). > > How about exposing an interface "OnBrowserToolbarClickListener" and, > BrowserApp implementing it? That removes the dependency on mActivity. And > BrowserApp knows about BrowserToolbar anyways. Done. Went with OnActivateListener. > @@ +257,5 @@ > > + // If the AwesomeBar has a composition string, don't submit the text yet. > > + // ENTER is needed to commit the composition string. > > + Editable content = mAwesomeBarText.getText(); > > + if (!hasCompositionString(content)) { > > + mActivity.commitAwesomebar(mEditingTarget); > > Same as "OnBrowserToolbarClickListener". May be we should expose a > "OnBrowserToolbarCommitListener" and BrowserApp should implement it. Done. Went with OnCommitListener. > @@ +325,5 @@ > > + if (Build.VERSION.SDK_INT >= 11 && selStart == selEnd) { > > + // mActivity.getActionBar().hide(); > > + } > > + } > > + }); > > There are 2 patterns I'm seeing in Android codebase. > 1. Specific private classes are made to implement interfaces, and a class > instance is reused. > https://github.com/android/platform_frameworks_base/blob/master/core/java/ > android/widget/ShareActionProvider.java#L297 > Hint: Kats uses a similar approach with GamepadUtils ;) > > 2. A bunch of listeners are implemented by a class, called Callback, and an > instance of it is used by a view. > https://github.com/android/platform_frameworks_base/blob/master/core/java/ > android/widget/ActivityChooserView.java#L525 > > We should probably use the Callbacks approach here. This would make the > methods small, and code clean. (And I vaguely remember that it helps with > thread concurrency -- I might be wrong). What are these comments about? > @@ +445,5 @@ > > + public void onClick(View v) { > > + mActivity.commitAwesomebar(mEditingTarget); > > + } > > + }); > > + > > This is same as the OnBrowserToolbarCommitListener that I mentioned above. Done. > @@ +969,2 @@ > > visible &= !(url == null || (url.startsWith("about:") && > > + !url.equals("about:blank"))) && !isEditing(); > > These strings could be made final and used here. > > @@ +1082,5 @@ > > animator.start(); > > } > > > > + private void showEditText() { > > + mAwesomeBarContent.setVisibility(View.GONE); > > mAwesomeBarContent could be renamed to "mUrlBarDisplayContainer". This would > make us distinguish between mUrlBarEditContainer and mUrlBarDisplayContainer. Makes sense. Done. > @@ +1114,5 @@ > > + mAwesomeBarText.setText(url != null ? url : ""); > > + > > + // This animation doesn't make much sense in a sidebar UI > > + if (HardwareUtils.isTablet()) { > > + mLayout.setSelected(true); > > Do we need this? mAwesomeBarEdit already has focus now. I thought this piece > was needed for showing the orange outline during animation. This is necessary to keep the bar in selected state while the toolbar is expanding/shrinking. > ::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml > @@ +68,5 @@ > > android:src="@drawable/ic_menu_back" > > android:contentDescription="@string/back" > > android:background="@drawable/address_bar_nav_button"/> > > > > + <LinearLayout android:id="@+id/awesome_bar_edit" > > It's better to call it "url_bar_edit_container". Done. > @@ +79,5 @@ > > + android:layout_toRightOf="@id/back" > > + android:layout_toLeftOf="@id/menu_items"> > > + > > + <view class="org.mozilla.gecko.CustomEditText" > > + android:id="@+id/awesome_bar_text" > > This could better be "url_bar_edit_text". Done. > ::: mobile/android/base/widget/AboutHome.java > @@ +6,5 @@ > > package org.mozilla.gecko.widget; > > > > import java.util.EnumSet; > > > > +import org.mozilla.gecko.AutocompleteHandler; > > Is this needed? Yes, as AboutHome is not part of the org.mozilla.gecko package.

> @@ +325,5 @@ > > + if (Build.VERSION.SDK_INT >= 11 && selStart == selEnd) { > > + // mActivity.getActionBar().hide(); > > + } > > + } > > + }); > > There are 2 patterns I'm seeing in Android codebase. > 1. Specific private classes are made to implement interfaces, and a class > instance is reused. > https://github.com/android/platform_frameworks_base/blob/master/core/java/ > android/widget/ShareActionProvider.java#L297 > Hint: Kats uses a similar approach with GamepadUtils ;) > > 2. A bunch of listeners are implemented by a class, called Callback, and an > instance of it is used by a view. > https://github.com/android/platform_frameworks_base/blob/master/core/java/ > android/widget/ActivityChooserView.java#L525 > > We should probably use the Callbacks approach here. This would make the > methods small, and code clean. (And I vaguely remember that it helps with > thread concurrency -- I might be wrong). What are these comments about? This is regarding moving all the event listeners for the EditText to a custom view, and make a Callbacks class that can handle all those events.

FYI: patches 1-3 can/should be pushed to m-c. 4 is for fig only.

Comment on attachment 748917 [details] [diff] [review] (3/4) Rename mAwesomeBarContent to mUrlDisplayContainer in BrowserToolbar Review of attachment 748917 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml @@ +68,5 @@ > android:src="@drawable/ic_menu_back" > android:contentDescription="@string/back" > android:background="@drawable/address_bar_nav_button"/> > > + <LinearLayout android:id="@+id/awesome_bar_display_container" I think this id should be url_display_container, to be consistent with the variable in BrowserToolbar.

Comment on attachment 748917 [details] [diff] [review] (3/4) Rename mAwesomeBarContent to mUrlDisplayContainer in BrowserToolbar Review of attachment 748917 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserToolbar.java @@ +71,2 @@ > private View mAwesomeBarEntry; > private ImageView mAwesomeBarRightEdge; A good follow-up would be to rename the variables for these other views, if our goal is to stop calling things "AwesomeBarWhatever". (Sorry, I was a bit quick to hit submit on that last comment.)

I just tried applying these first 3 patches to fig, and they don't apply. I think our game plan should be to land them on m-c, then we can merge m-c into fig, and base the 4th fig-only patch on top of that. Lucas, I can help out with this if you're busy with Google I/O stuff.

Comment on attachment 748918 [details] [diff] [review] (4/4) Move awesomescreen search into the about:home fragment >diff --git a/mobile/android/base/AutocompleteHandler.java b/mobile/android/base/AutocompleteHandler.java >\ No newline at end of file nit: fix >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java >+ InputMethodManager imm = (InputMethodManager) mActivity.getSystemService(Context.INPUT_METHOD_SERVICE); >+ try { >+ imm.hideSoftInputFromWindow(v.getWindowToken(), 0); >+ } catch (NullPointerException e) { >+ Log.e(LOGTAG, "InputMethodManagerService, why are you throwing" >+ + " a NullPointerException? See bug 782096", e); Should we check for null instead of catching? >+ mUrlEditText.setOnSelectionChangedListener(new CustomEditText.OnSelectionChangedListener() { >+ @Override >+ public void onSelectionChanged(int selStart, int selEnd) { >+ if (Build.VERSION.SDK_INT >= 11 && selStart == selEnd) { >+ // mActivity.getActionBar().hide(); >+ } Fixable? >+ } else if (keyCode == KeyEvent.KEYCODE_SEARCH) { >+ mUrlEditText.setText(""); >+ mUrlEditText.requestFocus(); >+ >+ InputMethodManager imm = >+ (InputMethodManager) mActivity.getSystemService(Context.INPUT_METHOD_SERVICE); >+ imm.showSoftInput(mUrlEditText, InputMethodManager.SHOW_IMPLICIT); No chance of null here? >+ if (Build.VERSION.SDK_INT >= 11) { >+ // mActivity.getActionBar().hide(); >+ } Fixable? >+ private void showUrlEditContainer() { >+ mUrlDisplayContainer.setVisibility(View.GONE); >+ mUrlEditContainer.setVisibility(View.VISIBLE); >+ mUrlEditText.requestFocus(); >+ >+ InputMethodManager imm = >+ (InputMethodManager) mActivity.getSystemService(Context.INPUT_METHOD_SERVICE); >+ imm.showSoftInput(mUrlEditText, InputMethodManager.SHOW_IMPLICIT); No chance of null? >+ InputMethodManager imm = InputMethods.getInputMethodManager(mUrlEditText.getContext()); >+ if (imm == null) { >+ return; We are checking here too >diff --git a/mobile/android/base/widget/AboutHome.java b/mobile/android/base/widget/AboutHome.java >+ public void filter(String searchTerm, AutocompleteHandler handler) { >+ // FIXME: implement actual awesomebar search >+ } File a followup?

I pushed the first 3 patches to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa58abdaf9c https://hg.mozilla.org/integration/mozilla-inbound/rev/3616ca00cbb1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fea1dd88de Maybe to keep better track of the state of this bug, we should split off the last patch into a separate bug.

(In reply to :Margaret Leibovic from comment #19) > I pushed the first 3 patches to inbound: > https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa58abdaf9c > https://hg.mozilla.org/integration/mozilla-inbound/rev/3616ca00cbb1 > https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fea1dd88de > > Maybe to keep better track of the state of this bug, we should split off the > last patch into a separate bug. I filed bug 872834 about this.

https://hg.mozilla.org/mozilla-central/rev/ffa58abdaf9c https://hg.mozilla.org/mozilla-central/rev/3616ca00cbb1 https://hg.mozilla.org/mozilla-central/rev/a4fea1dd88de

Comment on attachment 748918 [details] [diff] [review] (4/4) Move awesomescreen search into the about:home fragment Marking this patch as obsolete, since we're going to land it in bug 872834, not here.