----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7471/#review12284 -----------------------------------------------------------
Great patch, thanks Andrew! I didn't test the patch locally yet, but here are some initial comments. ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26037> Can we make all these static members final? ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26038> Please add space before "?" and after, same with ":". ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26039> Line too long. ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26043> Probably would be better to extract the code that computes width and height and creates thumbnail into a method. ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26040> Please add spaces. ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26041> Spaces ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26042> I think would be nice to add check that imageHeight and imageWidth are not zero, i.e. Preconditions.checkState(imageHeight != 0) Preconditions.checkState(imageWidth != 0) ./src/org/waveprotocol/box/server/attachment/AttachmentService.java <https://reviews.apache.org/r/7471/#comment26044> I think the code below that renders the graphics should be extracted into a separate method. ./src/org/waveprotocol/box/server/persistence/file/FileAttachmentStore.java <https://reviews.apache.org/r/7471/#comment26045> The copyright should belongs to Apache. There's a patch in progress that will remove all copyright line from the files. So please remove it. ./src/org/waveprotocol/box/server/persistence/mongodb/MongoDbStore.java <https://reviews.apache.org/r/7471/#comment26046> I think the original idea of including the waveletName in the complete attachment id was in order to prevent security issue, when someone will request at attachment by crafting request with a wavelet that he has access to by changing attachment to the one from a wave that he can't access. Does the new approach handle this issue? ./src/org/waveprotocol/box/server/rpc/AttachmentInfoServlet.java <https://reviews.apache.org/r/7471/#comment26047> Please combine the two javadocs into one. ./src/org/waveprotocol/box/server/rpc/AttachmentInfoServlet.java <https://reviews.apache.org/r/7471/#comment26048> Make it final ./src/org/waveprotocol/box/server/rpc/AttachmentInfoServlet.java <https://reviews.apache.org/r/7471/#comment26049> There's already such method in AttachmentsServlet. Can we maybe extract it into AttachmentsUtil class? ./src/org/waveprotocol/wave/client/doodad/attachment/AttachmentImpl.java <https://reviews.apache.org/r/7471/#comment26050> Please remove white space. ./src/org/waveprotocol/wave/client/doodad/attachment/AttachmentManagerImpl.java <https://reviews.apache.org/r/7471/#comment26052> Make the filed static ./src/org/waveprotocol/wave/client/doodad/attachment/AttachmentManagerImpl.java <https://reviews.apache.org/r/7471/#comment26051> The interface definition should be at the top. ./src/org/waveprotocol/wave/client/doodad/attachment/AttachmentManagerImpl.java <https://reviews.apache.org/r/7471/#comment26053> The declaration of static fields should be above of non static fields. - Yuri Zelikov On Oct. 8, 2012, 10:58 a.m., Andrew Kaplanov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7471/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2012, 10:58 a.m.) > > > Review request for wave and Yuri Zelikov. > > > Description > ------- > > -- Storage > > As in original version, attachments stored in the directory, defined in the > parameter attachment_store_directory of server.config. > But now all attachments with thumbnails and metadata stored in single > directory. > If you have attachments in your instance of Wiab, move files from > subdirectories in attachment_store_directory up and remove subdirectories. > > -- Thumbnails > > Image attachment shown in the wave as the reduced picture. > Not image attachment shown as icon, representing type of this attachment. > In this case icon is taken from the directory, defined in parameter > thumbnail_patterns_directory of server.config. > Icon must be in PNG format, and named as MIME type with replacing '/' to '_'. > For example thumbnail file for ZIP format (MIME type application/zip) must be > named application_zip. > > > Diffs > ----- > > ./build-proto.xml 1393974 > ./build.xml 1393974 > ./proto_src/org/waveprotocol/box/attachment/AttachmentProto.java > PRE-CREATION > ./server-config.xml 1393974 > ./src/org/waveprotocol/box/attachment/Attachment.gwt.xml PRE-CREATION > ./src/org/waveprotocol/box/attachment/attachment.proto PRE-CREATION > ./src/org/waveprotocol/box/server/CoreSettings.java 1393974 > ./src/org/waveprotocol/box/server/ServerMain.java 1393974 > ./src/org/waveprotocol/box/server/attachment/AttachmentService.java > PRE-CREATION > ./src/org/waveprotocol/box/server/persistence/AttachmentStore.java 1393974 > ./src/org/waveprotocol/box/server/persistence/AttachmentUtil.java 1393974 > ./src/org/waveprotocol/box/server/persistence/file/FileAttachmentStore.java > 1393974 > ./src/org/waveprotocol/box/server/persistence/mongodb/MongoDbStore.java > 1393974 > ./src/org/waveprotocol/box/server/rpc/AttachmentInfoServlet.java > PRE-CREATION > ./src/org/waveprotocol/box/server/rpc/AttachmentServlet.java 1393974 > ./src/org/waveprotocol/box/server/rpc/ProtoSerializer.java 1393974 > ./src/org/waveprotocol/box/webclient/WebClient.gwt.xml 1393974 > ./src/org/waveprotocol/wave/client/StageTwo.java 1393974 > ./src/org/waveprotocol/wave/client/doodad/attachment/AttachmentImpl.java > PRE-CREATION > > ./src/org/waveprotocol/wave/client/doodad/attachment/AttachmentManagerImpl.java > PRE-CREATION > > ./src/org/waveprotocol/wave/client/doodad/attachment/ImageThumbnailAttachmentHandler.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/ImageThumbnailNodeEventHandler.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/SimpleAttachmentManager.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/render/ImageThumbnailRenderer.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/render/ImageThumbnailWidget.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/render/ImageThumbnailWrapper.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/testing/FakeAttachment.java > 1393974 > > ./src/org/waveprotocol/wave/client/doodad/attachment/testing/FakeAttachmentsManager.java > 1393974 > ./src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/EditToolbar.java > 1393974 > ./src/org/waveprotocol/wave/media/model/AttachmentDocumentWrapper.java > 1393974 > ./src/org/waveprotocol/wave/media/model/AttachmentV3.java 1393974 > ./src/org/waveprotocol/wave/media/model/ClientAttachment.java 1393974 > ./src/org/waveprotocol/wave/media/model/MutableClientAttachment.java > 1393974 > ./test/org/waveprotocol/box/server/persistence/AttachmentStoreTestBase.java > 1393974 > ./test/org/waveprotocol/wave/media/model/AttachmentDocumentWrapperTest.java > 1393974 > > Diff: https://reviews.apache.org/r/7471/diff/ > > > Testing > ------- > > > Thanks, > > Andrew Kaplanov > >
