----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3530/#review4442 -----------------------------------------------------------
Looks pretty good. My only large comment was in regards to extending an existing listener interface where the added functionality seems to be specific to one class and not generally applicable to all of the classes that implement the interface. src/org/waveprotocol/wave/client/doodad/title/TitleAnnotationHandler.java <https://reviews.apache.org/r/3530/#comment9973> This was made public. But I don't see that it us used anywhere outside of this class. src/org/waveprotocol/wave/client/wavepanel/WavePanel.java <https://reviews.apache.org/r/3530/#comment9974> 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. - Michael 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 > >
