Review Request 124140: Ported over to Netty and added SSL support

[Albert Vaca Cintora]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124140/#review81789
-----------------------------------------------------------


I still have some comments, and we need to fix the CPU and memory consumption on older devices.


src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java (line 127)
<https://git.reviewboard.kde.org/r/124140/#comment56110>

    Is 200ms not enough?



src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java (line 376)
<https://git.reviewboard.kde.org/r/124140/#comment56111>

    No keepalive? Why?



src/org/kde/kdeconnect/Device.java (lines 244 - 251)
<https://git.reviewboard.kde.org/r/124140/#comment56106>

    We don't need to call sendPackage in Device, the link can do it because they have a similar (but lower-level) function. Also, this allows us to have Links that don't even need to send a packet to pair, because they use some other mechanism. If we make this change, Device should only allow sending packets if the device is paired.



src/org/kde/kdeconnect/Device.java (lines 298 - 302)
<https://git.reviewboard.kde.org/r/124140/#comment56107>

    same thing here



src/org/kde/kdeconnect/Device.java (lines 338 - 346)
<https://git.reviewboard.kde.org/r/124140/#comment56108>

    Same thing



src/org/kde/kdeconnect/Device.java (lines 485 - 490)
<https://git.reviewboard.kde.org/r/124140/#comment56109>

    ?



src/org/kde/kdeconnect/Backends/BaseLinkProvider.java (line 33)
<https://git.reviewboard.kde.org/r/124140/#comment56112>

    Make protected and add a getter.



src/org/kde/kdeconnect/Backends/BasePairingHandler.java (line 26)
<https://git.reviewboard.kde.org/r/124140/#comment56105>

    Could be an interface instead of an abstract class.



src/org/kde/kdeconnect/Backends/BasePairingHandler.java (line 30)
<https://git.reviewboard.kde.org/r/124140/#comment56104>

    accept_pairing -> acceptPairing like the others



src/org/kde/kdeconnect/Backends/LanBackend/LanPairingHandler.java (line 54)
<https://git.reviewboard.kde.org/r/124140/#comment56113>

    This function is called requestPairing but does not request anything, it just puts info in a NP. See my comments about this below.



src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackPairingHandler.java (line 26)
<https://git.reviewboard.kde.org/r/124140/#comment56103>

    Actually now the loopback link can be much simpler, it's not a must have, but we can definitely simplify it in a future.


- Albert Vaca Cintora


On June 25, 2015, 7:35 a.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124140/
> -----------------------------------------------------------
> 
> (Updated June 25, 2015, 7:35 a.m.)
> 
> 
> Review request for kdeconnect and Albert Vaca Cintora.
> 
> 
> Repository: kdeconnect-android
> 
> 
> Description
> -------
> 
> * Since MINA was tying hands in adding SSL, ported over Netty which has a good, clean and easy interface and a larger user and developer base.
> * Added support to setup links on SSL
> * Links automatically removed if wrong certificate is sent
> * Shows keys based on hash of certificate to check right certificates are received
> * Added a preference to use SSL, as it is experienced to cause high CPU usage on devices.
> * Corrected unit tests, LanLinkProvider is removed as it is nearly impossible to write it with current design. Will find a way
> 
> 
> Diffs
> -----
> 
>   build.gradle 6520c1c 
>   res/values/strings.xml c128342 
>   src/org/kde/kdeconnect/Backends/BaseLink.java 579a7af 
>   src/org/kde/kdeconnect/Backends/BaseLinkProvider.java cfaf621 
>   src/org/kde/kdeconnect/Backends/BasePairingHandler.java PRE-CREATION 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLink.java 5994142 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java ae26958 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanPairingHandler.java PRE-CREATION 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLink.java add92f8 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLinkProvider.java bd9c41b 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackPairingHandler.java PRE-CREATION 
>   src/org/kde/kdeconnect/BackgroundService.java 5e3d8c2 
>   src/org/kde/kdeconnect/Device.java 3262aef 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/RsaHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/SslHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/NetworkPackage.java f62b5d0 
>   src/org/kde/kdeconnect/UserInterface/PairActivity.java 7a45751 
>   tests/org/kde/kdeconnect/DeviceTest.java 5d3383d 
>   tests/org/kde/kdeconnect/LanLinkProviderTest.java fe7863a 
>   tests/org/kde/kdeconnect/LanLinkTest.java d3d94c9 
>   tests/org/kde/kdeconnect/NetworkPackageTest.java a21114e 
> 
> Diff: https://git.reviewboard.kde.org/r/124140/diff/
> 
> 
> Testing
> -------
> 
> Tesed on some device with where both supports SSL, also with PC where ssl is not supported, working fine. Need a little bit more testing with more devices.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20150626/3c11b0bb/attachment-0001.html>