You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Agustin Casiva <ca...@gmail.com> on 2011/06/16 16:17:59 UTC

Review Request: Improvements on Caja for PHP

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/924/
-----------------------------------------------------------

Review request for shindig and Bastian Hofmann.


Summary
-------

This Diff contains only the changes on core shindig files, there is two extra files from caja not included because are too long for review (es53-taming-frame.js, es53-guest-frame.js).

More info on https://issues.apache.org/jira/browse/SHINDIG-1550?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel#issue-tabs


Diffs
-----

  http://svn.apache.org/repos/asf/shindig/trunk/php/config/container.php 1135335 
  http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php PRE-CREATION 
  http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/GadgetHtmlRenderer.php 1135335 

Diff: https://reviews.apache.org/r/924/diff


Testing
-------


Thanks,

Agustin

Re: Review Request: Improvements on Caja for PHP

Posted by Bastian Hofmann <ba...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/924/#review850
-----------------------------------------------------------



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1846>

    phpdoc comment



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1843>

    - creating the path should probably be recursive
    - error handling is missing here, we should throw an exception if creating the path failed



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1844>

    Instead of throwing a generic Exception having a dedicated CajolingException would be nice



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1845>

    error handling, see comment above



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1847>

    phpdoc comment



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1848>

    phpdoc comment



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1849>

    phpdoc comment



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1850>

    phpdoc comment



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1857>

    You should use single quotes (consistency)



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1856>

    concatenating the string would be better ' . $jsCode . '



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1851>

    phpdoc comment



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1852>

    error handling, see comment above



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1853>

    error handling, see comment above



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1855>

    phpdoc



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1860>

    using in_array is probably faster



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php
<https://reviews.apache.org/r/924/#comment1854>

    visibility (public, private, protected) missing



http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/GadgetHtmlRenderer.php
<https://reviews.apache.org/r/924/#comment1859>

    You should be able to enable or disable cajoling with a config switch in container config


- Bastian


On 2011-06-16 14:17:59, Agustin Casiva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/924/
> -----------------------------------------------------------
> 
> (Updated 2011-06-16 14:17:59)
> 
> 
> Review request for shindig and Bastian Hofmann.
> 
> 
> Summary
> -------
> 
> This Diff contains only the changes on core shindig files, there is two extra files from caja not included because are too long for review (es53-taming-frame.js, es53-guest-frame.js).
> 
> More info on https://issues.apache.org/jira/browse/SHINDIG-1550?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel#issue-tabs
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/php/config/container.php 1135335 
>   http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php PRE-CREATION 
>   http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/GadgetHtmlRenderer.php 1135335 
> 
> Diff: https://reviews.apache.org/r/924/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Agustin
> 
>

Re: Review Request: Improvements on Caja for PHP

Posted by Agustin Casiva <ca...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/924/
-----------------------------------------------------------

(Updated 2011-06-22 19:10:57.984877)


Review request for shindig and Bastian Hofmann.


Changes
-------

Here it is the Full diff with the caja improvements, I wrote some test too (php/test/gadgets/CajaGadgetHtmlRendererTest.php).


Summary
-------

This Diff contains only the changes on core shindig files, there is two extra files from caja not included because are too long for review (es53-taming-frame.js, es53-guest-frame.js).

More info on https://issues.apache.org/jira/browse/SHINDIG-1550?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel#issue-tabs


Diffs (updated)
-----

  http://svn.apache.org/repos/asf/shindig/trunk/php/config/container.php 1135335 
  http://svn.apache.org/repos/asf/shindig/trunk/php/external/resources/com/google/caja/plugin/es53-guest-frame.js PRE-CREATION 
  http://svn.apache.org/repos/asf/shindig/trunk/php/external/resources/com/google/caja/plugin/es53-taming-frame.js PRE-CREATION 
  http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php PRE-CREATION 
  http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/GadgetHtmlRenderer.php 1135335 
  http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/CajaGadgetHtmlRendererTest.php PRE-CREATION 

Diff: https://reviews.apache.org/r/924/diff


Testing
-------


Thanks,

Agustin

Re: Review Request: Improvements on Caja for PHP

Posted by Agustin Casiva <ca...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/924/
-----------------------------------------------------------

(Updated 2011-06-16 18:21:54.268207)


Review request for shindig and Bastian Hofmann.


Changes
-------

Bastian Comments Fixed


Summary
-------

This Diff contains only the changes on core shindig files, there is two extra files from caja not included because are too long for review (es53-taming-frame.js, es53-guest-frame.js).

More info on https://issues.apache.org/jira/browse/SHINDIG-1550?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel#issue-tabs


Diffs (updated)
-----

  http://svn.apache.org/repos/asf/shindig/trunk/php/config/container.php 1135335 
  http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/Cajoling.php PRE-CREATION 
  http://svn.apache.org/repos/asf/shindig/trunk/php/src/gadgets/render/GadgetHtmlRenderer.php 1135335 

Diff: https://reviews.apache.org/r/924/diff


Testing
-------


Thanks,

Agustin