incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yuri Zelikov" <vega...@gmail.com>
Subject Re: Review Request: Attachments.
Date Tue, 09 Oct 2012 19:38:19 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message