> On 2012-01-18 20:42:51, Michael MacFadden wrote: > > src/org/waveprotocol/wave/client/wavepanel/WavePanel.java, line 52 > > <https://reviews.apache.org/r/3530/diff/1/?file=69700#file69700line52> > > > > I noticed that this method was added to this listener interface. > > However, every class that previously implemented this interface added this > > method as a no-op. Only the new class that was added actually implemented > > this method with any actionable code. If we have 6 classes that don't use > > this method and only 1 that does, perhaps, this method should be added to > > another interface. For one thing the abstraction would be cleaner. Also, > > you would not have to distribute events in the fire method to lots of > > listeners that ultimately will do nothing.
Yes, I also was thinking about creating a new interface that extends LifecycleListener and puting the onLoad() method there, but: 1. The onLoad() event sounds very basic and important to me. 2. The reset() method has no-op implementations too - but it's still part of the interface because of logical reasons. But I am OK with extending LifecycleListener too, let me know if you still prefer this option. - Yuri ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3530/#review4442 ----------------------------------------------------------- On 2012-01-18 19:45:01, Yuri Zelikov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3530/ > ----------------------------------------------------------- > > (Updated 2012-01-18 19:45:01) > > > Review request for wave, Michael MacFadden and Ali Lown. > > > Summary > ------- > > Sets the current wave title also as the browser window title. > > > Diffs > ----- > > src/org/waveprotocol/box/webclient/client/StagesProvider.java 8a5355a > src/org/waveprotocol/wave/client/StageThree.java f31ed3b > src/org/waveprotocol/wave/client/doodad/title/TitleAnnotationHandler.java > e8b13f7 > src/org/waveprotocol/wave/client/scroll/ProxyScrollPanel.java 27e5da7 > src/org/waveprotocol/wave/client/wavepanel/WavePanel.java d1944b7 > src/org/waveprotocol/wave/client/wavepanel/impl/WavePanelImpl.java 282387e > src/org/waveprotocol/wave/client/wavepanel/impl/edit/EditSession.java > b4f141b > > src/org/waveprotocol/wave/client/wavepanel/impl/focus/FocusFramePresenter.java > 1351987 > > src/org/waveprotocol/wave/client/wavepanel/impl/title/WindowTitleHandler.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3530/diff > > > Testing > ------- > > Tested locally, verified that the wave title also set as browser window title. > > > Thanks, > > Yuri > >
