Removing Quirks Mode default

Patch that changes default doctype for OS2.0 gadgets to use HTML5 instead of quirks mode. Also supports a new doctype feature outlined in the Remove Quirks Mode specification proposal.

https://issues.apache.org/jira/browse/SHINDIG-1504

http://docs.opensocial.org/display/OSD/Remove+Quirks+Mode

Patch includes Java changes and associated JUnits.

[mgmarum, 2011-03-22 20:24:35]

Refreshing patch with latest

[Paul Lindner, 2011-03-23 16:18:42]

Good start, only issue I can see is that existing sites might want to roll this
out on a container-by-container basis.  Also putting the metadata in the
ContainerConfig is a cleaner idiom than optional guice injection.

I'd also like to change the OpenSocialVersion class, but those are mostly
stylistic things that can wait till this is committed.

Thanks!

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:21:
import java.util.Collection;
looks like your IDE is putting java.* here.. can you revert this chunk here and
in other places in this patch?

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:110:
//Default doctype for OpenSocial 2.0 containers should be HTML5, however we
allow this to be overwritten in shindig.properties.
line > 100

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:132:
@Inject(optional = true)
These might be better living in the container config?

optional injection is not my favorite way of doing things...

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:187:
// Override & insert DocType if Gadget is written for OpenSocial 2.0 or greater,
if quirksmode is not set
> 100 here and below

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java
(right):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java:150:
public String getSpecificationVersion(){
Would it make sense to return the OpenSocialversion object here instead of a
string?

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(left):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19: 
more import flux...

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
(left):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java:72:

import flux.

[mgmarum, 2011-03-23 19:15:00]

I'll look at migrating to using ContainerConfig, need to get familiar with it.

Do you think it would be best to leave quirks mode as default for now and make
it very simple to change to HTML5?

On 2011/03/23 16:18:42, Paul Lindner wrote:
> Good start, only issue I can see is that existing sites might want to roll
this
> out on a container-by-container basis.  Also putting the metadata in the
> ContainerConfig is a cleaner idiom than optional guice injection.
> 
> I'd also like to change the OpenSocialVersion class, but those are mostly
> stylistic things that can wait till this is committed.
> 
> Thanks!
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
> (right):
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:21:
> import java.util.Collection;
> looks like your IDE is putting java.* here.. can you revert this chunk here
and
> in other places in this patch?
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:110:
> //Default doctype for OpenSocial 2.0 containers should be HTML5, however we
> allow this to be overwritten in shindig.properties.
> line > 100
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:132:
> @Inject(optional = true)
> These might be better living in the container config?
> 
> optional injection is not my favorite way of doing things...
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:187:
> // Override & insert DocType if Gadget is written for OpenSocial 2.0 or
greater,
> if quirksmode is not set
> > 100 here and below
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
> File
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java
> (right):
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java:150:
> public String getSpecificationVersion(){
> Would it make sense to return the OpenSocialversion object here instead of a
> string?
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
> File
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
> (left):
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19: 
> more import flux...
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
> (left):
> 
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java:72:
> 
> import flux.

[Paul Lindner, 2011-03-24 05:08:59]

ContainerConfig is really easy.

See how it's used here:

  protected void injectBaseTag(Gadget gadget, Node headTag) {
    GadgetContext context = gadget.getContext();
    if (containerConfig.getBool(context.getContainer(),
INSERT_BASE_ELEMENT_KEY)) {
      Uri base = gadget.getSpec().getUrl();
      View view = gadget.getCurrentView();
      if (view != null && view.getHref() != null) {
        base = view.getHref();
      }
      Element baseTag = headTag.getOwnerDocument().createElement("base");
      baseTag.setAttribute("href", base.toString());
      headTag.insertBefore(baseTag, headTag.getFirstChild());
    }
  }

you can then add the default value to the config/container.js

This allows you to configure this feature on a per-container basis.

On Wed, Mar 23, 2011 at 12:15 PM, <mgmarum@gmail.com> wrote:

> I'll look at migrating to using ContainerConfig, need to get familiar
> with it.
>
> Do you think it would be best to leave quirks mode as default for now
> and make it very simple to change to HTML5?
>
> On 2011/03/23 16:18:42, Paul Lindner wrote:
>
>> Good start, only issue I can see is that existing sites might want to
>>
> roll this
>
>> out on a container-by-container basis.  Also putting the metadata in
>>
> the
>
>> ContainerConfig is a cleaner idiom than optional guice injection.
>>
>
>  I'd also like to change the OpenSocialVersion class, but those are
>>
> mostly
>
>> stylistic things that can wait till this is committed.
>>
>
>  Thanks!
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>> File
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
>
>> (right):
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:21:
>
>> import java.util.Collection;
>> looks like your IDE is putting java.* here.. can you revert this chunk
>>
> here and
>
>> in other places in this patch?
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:110:
>
>> //Default doctype for OpenSocial 2.0 containers should be HTML5,
>>
> however we
>
>> allow this to be overwritten in shindig.properties.
>> line > 100
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:132:
>
>> @Inject(optional = true)
>> These might be better living in the container config?
>>
>
>  optional injection is not my favorite way of doing things...
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:187:
>
>> // Override & insert DocType if Gadget is written for OpenSocial 2.0
>>
> or greater,
>
>> if quirksmode is not set
>> > 100 here and below
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>> File
>>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java
>
>> (right):
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java:150:
>
>> public String getSpecificationVersion(){
>> Would it make sense to return the OpenSocialversion object here
>>
> instead of a
>
>> string?
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>> File
>>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
>
>> (left):
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19:
>
>  more import flux...
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
>
>> File
>>
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
>
>> (left):
>>
>
>
>
>
http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java:72:
>
>  import flux.
>>
>
>
>
> http://codereview.appspot.com/4160042/
>



-- 
Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner

[mgmarum, 2011-03-25 15:09:41]

Fixed import flux, use ContainerConfig for setting DOCTYPE

[mgmarum, 2011-03-25 15:09:59]

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:21:
import java.util.Collection;
On 2011/03/23 16:18:43, Paul Lindner wrote:
> looks like your IDE is putting java.* here.. can you revert this chunk here
and
> in other places in this patch?

Done.

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:110:
//Default doctype for OpenSocial 2.0 containers should be HTML5, however we
allow this to be overwritten in shindig.properties.
On 2011/03/23 16:18:43, Paul Lindner wrote:
> line > 100

Done.

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:132:
@Inject(optional = true)
On 2011/03/23 16:18:43, Paul Lindner wrote:
> These might be better living in the container config?
> 
> optional injection is not my favorite way of doing things...

Done.

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:187:
// Override & insert DocType if Gadget is written for OpenSocial 2.0 or greater,
if quirksmode is not set
On 2011/03/23 16:18:43, Paul Lindner wrote:
> > 100 here and below

Done.

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java
(right):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/GadgetSpec.java:150:
public String getSpecificationVersion(){
On 2011/03/23 16:18:43, Paul Lindner wrote:
> Would it make sense to return the OpenSocialversion object here instead of a
> string?

Done.

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(left):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19: 
On 2011/03/23 16:18:43, Paul Lindner wrote:
> more import flux...

Done.

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
(left):

http://codereview.appspot.com/4160042/diff/9001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java:72:

On 2011/03/23 16:18:43, Paul Lindner wrote:
> import flux.

Done.