Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
683203006:
Adding option for adding new tab in tab manager for chrome shell. (Closed)
|
Created:
6 years, 1 month ago by divya.bansal Modified:
6 years, 1 month ago Reviewers:
Bernhard Bauer CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding option for adding new tab in tab manager for chrome shell.
Currently in order to open a new tab, option from menu is used.
These changes add new tab button in tabswitcher mode in place
of reload/stop button. In this mode reload/stop button is not required.
Signed-off-by: divyabansal <divya.bansal@samsung.com>;
BUG=428660
Committed: https://crrev.com/8383c8fbdd51ce81a030f783625229cecf08ce89
Cr-Commit-Position: refs/heads/master@{#303820}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes according to review comments #Patch Set 3 : Review changes #
Total comments: 7
Patch Set 4 : Review Changes #Patch Set 5 : Changes According to Review Comments #
Total comments: 4
Patch Set 6 : Review Changes #
Total comments: 6
Patch Set 7 : Adding Review Comments #
Total comments: 10
Patch Set 8 : Review changes #
Total comments: 4
Patch Set 9 : Review comment changes #
Total comments: 4
Patch Set 10 : Changes For Review Comments #
Messages
Total messages: 29 (2 generated)
divya.bansal@samsung.com changed reviewers: + bauerb@chromium.org
@Bernhard PTAL
https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/java/sr... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/java/sr... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:104: mAddButton.setVisibility(GONE); So, are we not going to show the stop/reload button again here? In general, this seems a bit... random. What if there is some other code path that hides or shows the tab switcher? If we want to have the invariant that the add tab button is visible if and only if the tab switcher is visible, and the stop/reload button is visible otherwise, we should see if there is a way to observe state changes, instead of trying to hook into every single place where we might hide or show the tab switcher. https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/res/lay... File chrome/android/shell/res/layout/chrome_shell_activity.xml (right): https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/res/lay... chrome/android/shell/res/layout/chrome_shell_activity.xml:19: <org.chromium.chrome.browser.widget.TintedImageButton This line should be indented one more space.
Review changes are done. Please check and give your suggestions https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/java/sr... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/java/sr... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:104: mAddButton.setVisibility(GONE); On 2014/11/04 12:22:10, Bernhard Bauer wrote: > So, are we not going to show the stop/reload button again here? > > In general, this seems a bit... random. What if there is some other code path > that hides or shows the tab switcher? If we want to have the invariant that the > add tab button is visible if and only if the tab switcher is visible, and the > stop/reload button is visible otherwise, we should see if there is a way to > observe state changes, instead of trying to hook into every single place where > we might hide or show the tab switcher. Done. Showing the add button when tabswitcher is shown. https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/res/lay... File chrome/android/shell/res/layout/chrome_shell_activity.xml (right): https://codereview.chromium.org/683203006/diff/1/chrome/android/shell/res/lay... chrome/android/shell/res/layout/chrome_shell_activity.xml:19: <org.chromium.chrome.browser.widget.TintedImageButton On 2014/11/04 12:22:10, Bernhard Bauer wrote: > This line should be indented one more space. Done.
https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: mTabManager.mToolbar.initializeAddButton(false); Hmm... the encapsulation violation here aside, I'm not very happy for the model to know about the toolbar and the state of the buttons on it. Really, the toolbar should observe changes to the model and update itself in reaction. We could add a method onVisibilityChanged() to the TabModelSelector.ChangeListener interface and have ChromeShellToolbar implement that to update its buttons. That would also allow us to move the code that updates the view here to some place else, like the TabManager. Then the model would only know about its state and not deal with updating any views. If we end up doing this, most of my comments below probably don't apply anymore; I've left them in for completeness. https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:248: public void initializeAddButton(boolean tabswitcher) { This should have a Javadoc comment. Also, |tabswitcher| as a boolean parameter isn't very intuitive: what does a true or false value actually mean? And finally, there is no need to reinitialize |mAddButton| or set a new OnClickListener every time we want to hide or show the button. https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:36: public ChromeShellToolbar mToolbar; Don't make member variables public, add a getter method instead.
Please suggest https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: mTabManager.mToolbar.initializeAddButton(false); On 2014/11/05 10:01:53, Bernhard Bauer wrote: > Hmm... the encapsulation violation here aside, I'm not very happy for the model > to know about the toolbar and the state of the buttons on it. Really, the > toolbar should observe changes to the model and update itself in reaction. > > We could add a method onVisibilityChanged() to the > TabModelSelector.ChangeListener interface and have ChromeShellToolbar implement > that to update its buttons. That would also allow us to move the code that > updates the view here to some place else, like the TabManager. Then the model > would only know about its state and not deal with updating any views. > > If we end up doing this, most of my comments below probably don't apply anymore; > I've left them in for completeness. AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and its object is initialized in ChromeShellTabModelSelector according to visibility of tabswitcher mode. So any method in TabModelSelector.ChangeListener cannot be implemented in ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that how to implement it.
https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: mTabManager.mToolbar.initializeAddButton(false); On 2014/11/05 12:22:17, divya.bansal wrote: > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > Hmm... the encapsulation violation here aside, I'm not very happy for the > model > > to know about the toolbar and the state of the buttons on it. Really, the > > toolbar should observe changes to the model and update itself in reaction. > > > > We could add a method onVisibilityChanged() to the > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > implement > > that to update its buttons. That would also allow us to move the code that > > updates the view here to some place else, like the TabManager. Then the model > > would only know about its state and not deal with updating any views. > > > > If we end up doing this, most of my comments below probably don't apply > anymore; > > I've left them in for completeness. > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and its > object is initialized in ChromeShellTabModelSelector according to visibility of > tabswitcher mode. > So any method in TabModelSelector.ChangeListener cannot be implemented in > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that how > to implement it. Why is it a problem that AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener? Just implement the new method with an empty body there.
On 2014/11/05 12:36:04, Bernhard Bauer wrote: > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > File > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java > (right): > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: > mTabManager.mToolbar.initializeAddButton(false); > On 2014/11/05 12:22:17, divya.bansal wrote: > > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > > Hmm... the encapsulation violation here aside, I'm not very happy for the > > model > > > to know about the toolbar and the state of the buttons on it. Really, the > > > toolbar should observe changes to the model and update itself in reaction. > > > > > > We could add a method onVisibilityChanged() to the > > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > > implement > > > that to update its buttons. That would also allow us to move the code that > > > updates the view here to some place else, like the TabManager. Then the > model > > > would only know about its state and not deal with updating any views. > > > > > > If we end up doing this, most of my comments below probably don't apply > > anymore; > > > I've left them in for completeness. > > > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and > its > > object is initialized in ChromeShellTabModelSelector according to visibility > of > > tabswitcher mode. > > So any method in TabModelSelector.ChangeListener cannot be implemented in > > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that how > > to implement it. > > Why is it a problem that AccessibilityTabModelWrapper implements > TabModelSelector.ChangeListener? Just implement the new method with an empty > body there. That is true that new method with an empty body can be added there but if this listener is also implemented in ChromeShellToolbar with implementation of new method, then that will never be called because through show and hide tabswitcher, listener in AccessibilityTabModelWrapper is getting registered through object of ChromeShellTabModelSelector. tHAT IS TRUE THA
On 2014/11/05 12:45:07, divya.bansal wrote: > On 2014/11/05 12:36:04, Bernhard Bauer wrote: > > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > > File > > > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java > > (right): > > > > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > > > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: > > mTabManager.mToolbar.initializeAddButton(false); > > On 2014/11/05 12:22:17, divya.bansal wrote: > > > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > > > Hmm... the encapsulation violation here aside, I'm not very happy for the > > > model > > > > to know about the toolbar and the state of the buttons on it. Really, the > > > > toolbar should observe changes to the model and update itself in reaction. > > > > > > > > We could add a method onVisibilityChanged() to the > > > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > > > implement > > > > that to update its buttons. That would also allow us to move the code that > > > > updates the view here to some place else, like the TabManager. Then the > > model > > > > would only know about its state and not deal with updating any views. > > > > > > > > If we end up doing this, most of my comments below probably don't apply > > > anymore; > > > > I've left them in for completeness. > > > > > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and > > its > > > object is initialized in ChromeShellTabModelSelector according to visibility > > of > > > tabswitcher mode. > > > So any method in TabModelSelector.ChangeListener cannot be implemented in > > > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that > how > > > to implement it. > > > > Why is it a problem that AccessibilityTabModelWrapper implements > > TabModelSelector.ChangeListener? Just implement the new method with an empty > > body there. > That is true that new method with an empty body can be added there but if this > listener is also implemented in ChromeShellToolbar with implementation of new > method, then that will never be called because through show and hide > tabswitcher, listener in AccessibilityTabModelWrapper is getting registered > through object of ChromeShellTabModelSelector. The AccessibilityTabModelWrapper is only created and registered as a listener the first time the tab switcher is shown. Hiding and showing it again just removes it from the view hierarchy and reattaches it, it doesn't unregister or re-register it as a listener, so it will get notified subsequently.
On 2014/11/05 13:15:04, Bernhard Bauer wrote: > On 2014/11/05 12:45:07, divya.bansal wrote: > > On 2014/11/05 12:36:04, Bernhard Bauer wrote: > > > > > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > > > File > > > > > > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java > > > (right): > > > > > > > > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > > > > > > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: > > > mTabManager.mToolbar.initializeAddButton(false); > > > On 2014/11/05 12:22:17, divya.bansal wrote: > > > > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > > > > Hmm... the encapsulation violation here aside, I'm not very happy for > the > > > > model > > > > > to know about the toolbar and the state of the buttons on it. Really, > the > > > > > toolbar should observe changes to the model and update itself in > reaction. > > > > > > > > > > We could add a method onVisibilityChanged() to the > > > > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > > > > implement > > > > > that to update its buttons. That would also allow us to move the code > that > > > > > updates the view here to some place else, like the TabManager. Then the > > > model > > > > > would only know about its state and not deal with updating any views. > > > > > > > > > > If we end up doing this, most of my comments below probably don't apply > > > > anymore; > > > > > I've left them in for completeness. > > > > > > > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener > and > > > its > > > > object is initialized in ChromeShellTabModelSelector according to > visibility > > > of > > > > tabswitcher mode. > > > > So any method in TabModelSelector.ChangeListener cannot be implemented in > > > > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that > > how > > > > to implement it. > > > > > > Why is it a problem that AccessibilityTabModelWrapper implements > > > TabModelSelector.ChangeListener? Just implement the new method with an empty > > > body there. > > That is true that new method with an empty body can be added there but if this > > listener is also implemented in ChromeShellToolbar with implementation of new > > method, then that will never be called because through show and hide > > tabswitcher, listener in AccessibilityTabModelWrapper is getting registered > > through object of ChromeShellTabModelSelector. > > The AccessibilityTabModelWrapper is only created and registered as a listener > the first time the tab switcher is shown. Hiding and showing it again just > removes it from the view hierarchy and reattaches it, it doesn't unregister or > re-register it as a listener, so it will get notified subsequently. Thanks alot for the explanation.
PTAL new changes https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: mTabManager.mToolbar.initializeAddButton(false); On 2014/11/05 12:36:03, Bernhard Bauer wrote: > On 2014/11/05 12:22:17, divya.bansal wrote: > > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > > Hmm... the encapsulation violation here aside, I'm not very happy for the > > model > > > to know about the toolbar and the state of the buttons on it. Really, the > > > toolbar should observe changes to the model and update itself in reaction. > > > > > > We could add a method onVisibilityChanged() to the > > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > > implement > > > that to update its buttons. That would also allow us to move the code that > > > updates the view here to some place else, like the TabManager. Then the > model > > > would only know about its state and not deal with updating any views. > > > > > > If we end up doing this, most of my comments below probably don't apply > > anymore; > > > I've left them in for completeness. > > > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and > its > > object is initialized in ChromeShellTabModelSelector according to visibility > of > > tabswitcher mode. > > So any method in TabModelSelector.ChangeListener cannot be implemented in > > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that how > > to implement it. > > Why is it a problem that AccessibilityTabModelWrapper implements > TabModelSelector.ChangeListener? Just implement the new method with an empty > body there. @Bernhard I have tried to implement this behaviour in other way using local objects and methods. PTAL at new changes.
On 2014/11/07 05:26:26, divya.bansal wrote: > PTAL new changes > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > File > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java > (right): > > https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... > chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: > mTabManager.mToolbar.initializeAddButton(false); > On 2014/11/05 12:36:03, Bernhard Bauer wrote: > > On 2014/11/05 12:22:17, divya.bansal wrote: > > > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > > > Hmm... the encapsulation violation here aside, I'm not very happy for the > > > model > > > > to know about the toolbar and the state of the buttons on it. Really, the > > > > toolbar should observe changes to the model and update itself in reaction. > > > > > > > > We could add a method onVisibilityChanged() to the > > > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > > > implement > > > > that to update its buttons. That would also allow us to move the code that > > > > updates the view here to some place else, like the TabManager. Then the > > model > > > > would only know about its state and not deal with updating any views. > > > > > > > > If we end up doing this, most of my comments below probably don't apply > > > anymore; > > > > I've left them in for completeness. > > > > > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and > > its > > > object is initialized in ChromeShellTabModelSelector according to visibility > > of > > > tabswitcher mode. > > > So any method in TabModelSelector.ChangeListener cannot be implemented in > > > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that > how > > > to implement it. > > > > Why is it a problem that AccessibilityTabModelWrapper implements > > TabModelSelector.ChangeListener? Just implement the new method with an empty > > body there. > > @Bernhard I have tried to implement this behaviour in other way using local > objects and methods. > PTAL at new changes. Did you see my previous comment about the toolbar observing the tab model selector? I really would like to reduce the amount of code in ChromeShellTabModelSelector that depends on UI, rather than adding more code. We could move the code that shows and hides the tab switcher to the TabManager, and have that update the toolbar as well.
PTAL new changes. I have implemented code in TabManager. https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:123: mTabManager.mToolbar.initializeAddButton(false); On 2014/11/07 05:26:26, divya.bansal wrote: > On 2014/11/05 12:36:03, Bernhard Bauer wrote: > > On 2014/11/05 12:22:17, divya.bansal wrote: > > > On 2014/11/05 10:01:53, Bernhard Bauer wrote: > > > > Hmm... the encapsulation violation here aside, I'm not very happy for the > > > model > > > > to know about the toolbar and the state of the buttons on it. Really, the > > > > toolbar should observe changes to the model and update itself in reaction. > > > > > > > > We could add a method onVisibilityChanged() to the > > > > TabModelSelector.ChangeListener interface and have ChromeShellToolbar > > > implement > > > > that to update its buttons. That would also allow us to move the code that > > > > updates the view here to some place else, like the TabManager. Then the > > model > > > > would only know about its state and not deal with updating any views. > > > > > > > > If we end up doing this, most of my comments below probably don't apply > > > anymore; > > > > I've left them in for completeness. > > > > > > AccessibilityTabModelWrapper implements TabModelSelector.ChangeListener and > > its > > > object is initialized in ChromeShellTabModelSelector according to visibility > > of > > > tabswitcher mode. > > > So any method in TabModelSelector.ChangeListener cannot be implemented in > > > ChromeShellToolbar or ChromeShellToolbarModelSelector. Please suggest that > how > > > to implement it. > > > > Why is it a problem that AccessibilityTabModelWrapper implements > > TabModelSelector.ChangeListener? Just implement the new method with an empty > > body there. > > @Bernhard I have tried to implement this behaviour in other way using local > objects and methods. > PTAL at new changes. Done. Made changes according to your suggestion. PTAL
Nice! https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:34: private AccessibilityTabModelWrapper mTabModelWrapper; This doesn't seem to be used anymore now. https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:196: mToolbar.showAddButton(false); Is there some method to where these calls appear? I could see doing both at the beginning of the method, both at the end, or even one at the beginning and one at the end (making the methods inverse to each other), but this seems a bit... random.
PTAL https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:34: private AccessibilityTabModelWrapper mTabModelWrapper; On 2014/11/10 13:58:33, Bernhard Bauer wrote: > This doesn't seem to be used anymore now. Done. https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/80001/chrome/android/shell/jav... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:196: mToolbar.showAddButton(false); On 2014/11/10 13:58:33, Bernhard Bauer wrote: > Is there some method to where these calls appear? I could see doing both at the > beginning of the method, both at the end, or even one at the beginning and one > at the end (making the methods inverse to each other), but this seems a bit... > random. Done.
https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:262: public void showAddButton(boolean tabSwitcherStatus) { Please add a Javadoc comment. https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:190: /* Add a second asterisk here to make this a Javadoc comment. https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:218: public boolean isTabSwitcherVisible() { Please add a Javadoc comment.
PTAL https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:262: public void showAddButton(boolean tabSwitcherStatus) { On 2014/11/11 09:08:22, Bernhard Bauer wrote: > Please add a Javadoc comment. Done. https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:190: /* On 2014/11/11 09:08:22, Bernhard Bauer wrote: > Add a second asterisk here to make this a Javadoc comment. Done. https://codereview.chromium.org/683203006/diff/100001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:218: public boolean isTabSwitcherVisible() { On 2014/11/11 09:08:22, Bernhard Bauer wrote: > Please add a Javadoc comment. Done.
https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:95: mParent.getContext(), loadUrlParams.getUrl(), mWindow, client, mTabManager); This is the only place where we use |mParent|, so we could instead directly pass in the context. Sorrry for missing that earlier. https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:263: * Shows the Add Button if tab Switcher is shown. Don't capitalize "add button", and neither "switcher". I'm also not certain if that if-clause is necessary; for purposes of this class, it doesn't have to know under which circumstances it is called. https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:264: * @param tabSwitcherStatus The visibility status of Tab Switcher. Don't capitalize the tab switcher here, and use an article (otherwise it sounds like a common name). https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:204: * Shows the tab switcher. FYI: for private methods Javadoc isn't strictly necessary, but in this case it's still good to have it, to be consistent with hideTabSwitcher(). https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/re... File chrome/android/shell/res/layout/chrome_shell_activity.xml (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/re... chrome/android/shell/res/layout/chrome_shell_activity.xml:24: android:visibility="gone" One less space.
PTAL https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:95: mParent.getContext(), loadUrlParams.getUrl(), mWindow, client, mTabManager); On 2014/11/11 11:34:02, Bernhard Bauer wrote: > This is the only place where we use |mParent|, so we could instead directly pass > in the context. Sorrry for missing that earlier. Done. https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:263: * Shows the Add Button if tab Switcher is shown. On 2014/11/11 11:34:02, Bernhard Bauer wrote: > Don't capitalize "add button", and neither "switcher". I'm also not certain if > that if-clause is necessary; for purposes of this class, it doesn't have to know > under which circumstances it is called. Done. https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:264: * @param tabSwitcherStatus The visibility status of Tab Switcher. On 2014/11/11 11:34:02, Bernhard Bauer wrote: > Don't capitalize the tab switcher here, and use an article (otherwise it sounds > like a common name). Done. https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:204: * Shows the tab switcher. On 2014/11/11 11:34:02, Bernhard Bauer wrote: > FYI: for private methods Javadoc isn't strictly necessary, but in this case it's > still good to have it, to be consistent with hideTabSwitcher(). Acknowledged. https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/re... File chrome/android/shell/res/layout/chrome_shell_activity.xml (right): https://codereview.chromium.org/683203006/diff/120001/chrome/android/shell/re... chrome/android/shell/res/layout/chrome_shell_activity.xml:24: android:visibility="gone" On 2014/11/11 11:34:02, Bernhard Bauer wrote: > One less space. Done.
https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:37: WindowAndroid window, ContentVideoViewClient videoViewClient, ViewGroup parent, You don't even need to pass in the parent, just directly pass the context (and |mContext| for it is fine). https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:266: public void showAddButton(boolean tabSwitcherStatus) { This parameter could be |visibility|. This class doesn't have to know under which circumstances it shows the add button, that's what TabManager does.
@Bernhard PTAL https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java (right): https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellTabModelSelector.java:37: WindowAndroid window, ContentVideoViewClient videoViewClient, ViewGroup parent, On 2014/11/12 09:23:17, Bernhard Bauer wrote: > You don't even need to pass in the parent, just directly pass the context (and > |mContext| for it is fine). Done. https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/140001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:266: public void showAddButton(boolean tabSwitcherStatus) { On 2014/11/12 09:23:17, Bernhard Bauer wrote: > This parameter could be |visibility|. This class doesn't have to know under > which circumstances it shows the add button, that's what TabManager does. Done.
https://codereview.chromium.org/683203006/diff/160001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/160001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:263: * Shows the add button and hides the stop/reload button. Technically, this can also do the opposite... Add ", or vice-versa" at the end? https://codereview.chromium.org/683203006/diff/160001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:264: * @param visibility The visibility status of the tab switcher. Replace "tab switcher" with "add button".
PTAL https://codereview.chromium.org/683203006/diff/160001/chrome/android/shell/ja... File chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java (right): https://codereview.chromium.org/683203006/diff/160001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:263: * Shows the add button and hides the stop/reload button. On 2014/11/12 10:00:13, Bernhard Bauer wrote: > Technically, this can also do the opposite... Add ", or vice-versa" at the end? Done. https://codereview.chromium.org/683203006/diff/160001/chrome/android/shell/ja... chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:264: * @param visibility The visibility status of the tab switcher. On 2014/11/12 10:00:13, Bernhard Bauer wrote: > Replace "tab switcher" with "add button". Done.
LGTM
On 2014/11/12 10:40:37, Bernhard Bauer wrote: > LGTM @Bernhard Thanks for the review and suggestions.
The CQ bit was checked by divya.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/683203006/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8383c8fbdd51ce81a030f783625229cecf08ce89 Cr-Commit-Position: refs/heads/master@{#303820} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
