On Wed, Aug 19, 2009 at 8:29 PM, <j...@google.com> wrote: > Thanks for the review. > > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011 > File dev/oophm/src/com/google/gwt/dev/ModuleTabPanel.java (right): > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011#newcode89 > Line 89: public synchronized void addModule(String moduleName, > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> Who's calling this that it needs to be synchronized? I only see it >> > being reached > >> from ModulePanel's constructor--how's that getting on multiple >> > threads? > > Likewise on the various other synchronized methods on this class. They >> > seem like > >> a bad sign. >> > > Two modules could be created at the same time, and the socket listener > thread forks them into their own threads.
So you're calling into the UI objects from other threads? Spooky. I suppose if it's working it's working... > > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011#newcode113 > Line 113: // TODO(jat): don't show dropdown when only one module > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> boolean showIt = modules.size() > 1; >> moduleDropdown.setVisible(showIt); >> moduleDropdown.setEnabled(showIt); >> > > And don't bother building it if !showIt, for that matter >> > > Actually, I suppose you'd put the dropdown and its label in a panel, >> > and set > >> visibility on that. Same idea. >> > > Ok. I was thinking we needed to show a label with the module name in > that case, since otherwise we don't have it anywhere. Maybe that isn't > important if they only have one module, but I could imagine an app that > might load one module or another one and you would have no easy way of > knowing which one you were seeing. Could do the same label trick as for the session popup. Hey presto, you have a new widget. > > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011#newcode119 > Line 119: if (firstModule != null) { > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> never true >> > > Good catch, should be == null. > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011#newcode211 > Line 211: private static final class SessionModule { > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> ditto on breaking this out and putting a unit test around it >> > > Ok, though the only thing interesting about it is keeping an instance > cache to ensure identity -- otherwise it is just an immutable data > object so that the map can have multiple keys. > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011#newcode313 > Line 313: private JComboBox sessionDropdown; > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> private JLabel singleSessionLabel; >> private JPanel sessionPanel; >> > > For showing a label for a single session? > > Is it really that much better than just having a dropdown with only one > thing in it? Single entry dropdowns are sloppy and distracting. > Without duplicating a lot of work, then how will it show > when it is disconnected or active, since we planned on doing that with > the JComboBox renderer? The whole page will be pink if it's disconnected. I think they'll figure it out. > > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2011#newcode453 > Line 453: for (Session session : sessions.values()) { > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> You're listing dead sessions too, which is just noise. But the point >> > is moot if > >> you change the scope of the close button to sessions instead of tabs. >> > > When you do that, remember to put the close button in the same row as >> > the Expand > >> All / Collapse All buttons, to make it clear it's subordinate to the >> session+module >> > > That gets more complicated, since those are in a SwingLoggerPanel -- I > would have to expose the internals of how that is laid out, or move the > close functionality into that class (which seems wrong since we don't > want to be able to close non-module tabs). You can make the SLP accept an onClose callback, and show the close button iff one has been provided. > > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2010 > File dev/oophm/src/com/google/gwt/dev/OophmHostedModeBase.java (right): > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2010#newcode165 > Line 165: assert remoteHost != null; > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> I know we use asserts in client code, but it's not a great idea to use >> > them in > >> JRE code. Do you know when we enable them and when we don't? Will that >> accidentally change tomorrow? >> > > Someday we'll get our act together and be able to use >> > > > http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/base/Preconditions.html > >> for this kind of thing. Until then, please just throw >> > IllegalArgumentExceptions: > > > if (userAgent == null) { >> throw new IllegalArgumentException("userAgent cannot be null"); >> } >> if (remoteHost == null) { >> throw new IllegalArgumentException("remoteHost cannot be null"); >> } >> > > Do we want to pay that runtime cost all the time? In general our > philosophy has been not to do so, so I didn't. Here, there are only > going to be a handful of these objects so it is pretty much irrelevant. That runtime cost is completely negligible, especially for something like this that is happening in response to user actions. You aren't on web browser, you're in a native app. Live a little, and don't introduce points of mystery into your code. > > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2010#newcode169 > Line 169: // TODO(jat): is it correct to strip off the query part? > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> Is there any reason you're building this by hand in a stringbuffer >> > rather than > >> using a URI or URL? >> > > I need only part of the URL and I didn't see an easy way to do it. I > could create a copy and set just the pieces I want, but that seemed > overkill. > > Also, I would still have to convert to a string since the case where the > supplied URL is malformed means I need to just use the supplied URL, > which necessarily can't be represented in URL/URI. > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2010#newcode316 > Line 316: String remoteHost = remoteSocket.substring(0, hostEnd); > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> The socket value here is just being used for display purposes, right? >> > I hope > >> you're not actually doing network operations in the UI thread? >> > Anything that > >> hangs in that thread hangs the UI, which is one reason IntelliJ is so >> > crazy > >> making. >> > > http://java.sun.com/docs/books/tutorial/uiswing/concurrency/index.html >> > > http://java.sun.com/products/jfc/tsc/articles/threads/threads2.html >> > > > It is used as a key to help differentiate tabs when using an old browser > plugin and logging. > > http://gwt-code-reviews.appspot.com/59801/diff/2001/2010#newcode347 > Line 347: protected static String randomString() { > On 2009/08/20 02:37:12, Ray Ryan wrote: > >> Why not just >> > > return "" + new Date().getTime()? >> > > If you have several requests coming in at once it seems likely they may > get the same timestamp. This is used when using an old plugin and is > part of the key given to CardLayout to select the write logger. > > > http://gwt-code-reviews.appspot.com/59801 > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
