Issue 1852041: Remove unneeded @imports handling

Issue 1852041: Remove unneeded @imports handling (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years, 8 months ago by Jasvir

Modified:
15 years, 8 months ago

Reviewers:
johnfargo, gagan.goku

CC:
shindig.remailer_gmail.com

Base URL:
https://svn.apache.org/repos/asf/shindig/trunk/

Visibility:
Public.

Description

* uses full caja parser rather than a lexer workaround for @imports in CSS * Adds missing broken test for @import "url"

Patch Set 2 : Remove unneeded @imports handling #

Patch Set 3 : Remove unneeded @imports handling #

Patch Set 4 : Missing test case added #

Patch Set 5 : Syncing to trunk #

Patch Set 6 : Corrected test in StyleTagProxyEmbeddedUrlsVisitorTest #

Created: 15 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+91 lines, -62 lines)			Patch
M	java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java	View	1 2 3 4 5	5 chunks	+14 lines, -8 lines	0 comments	Download
M	java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java	View	2 3 4 5	2 chunks	+3 lines, -2 lines	0 comments	Download
M	java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java	View	1 2 3 4 5	6 chunks	+36 lines, -22 lines	0 comments	Download
M	java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java	View	3 4 5	4 chunks	+5 lines, -5 lines	0 comments	Download
M	java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingResponseRewriterTest.java	View	3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java	View	1 2 3 4 5	5 chunks	+24 lines, -6 lines	0 comments	Download
M	java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java	View	2 3 4 5	4 chunks	+6 lines, -3 lines	0 comments	Download
M	java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/old/rewritebasic-expected.css	View	2 3 4 5	2 chunks	+0 lines, -5 lines	0 comments	Download
M	java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritebasic.css	View	2 3 4 5	2 chunks	+0 lines, -5 lines	0 comments	Download
M	java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritebasic-expected.css	View	2 3 4 5	1 chunk	+1 line, -5 lines	0 comments	Download

Messages

Total messages: 11

Expand All Messages | Collapse All Messages

Jasvir

Please hold off on reviewing this change - it was a first pass and not ...

15 years, 8 months ago (2010-07-15 07:55:54 UTC) #2

gagan.goku

I don't have in-depth knowledge about caja css parser and how its used in gadgets ...

15 years, 8 months ago (2010-07-15 19:17:32 UTC) #3

johnfargo

http://codereview.appspot.com/1852041/diff/1/4 File java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java (right): http://codereview.appspot.com/1852041/diff/1/4#newcode56 java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java:56: * TODO(jasvir): This ought to be set to the ...

15 years, 8 months ago (2010-07-16 01:12:34 UTC) #4

Jasvir

15 years, 8 months ago (2010-07-20 06:30:08 UTC) #5

http://codereview.appspot.com/1852041/diff/1/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java
(right):

http://codereview.appspot.com/1852041/diff/1/4#newcode56
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java:56:
* TODO(jasvir): This ought to be set to the real URI for accurate reporting
On 2010/07/16 01:12:34, johnfargo wrote:
> want to just do it in this CL? all calling contexts have the source Uri
> available.

I also changed the relative urls like "www.example.org/foo" into actual absolute
ones.

Done.

http://codereview.appspot.com/1852041/diff/1/5
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
(right):

http://codereview.appspot.com/1852041/diff/1/5#newcode167
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:167:
// Extract the import statment, add its URI to the returned list
On 2010/07/16 01:12:34, johnfargo wrote:
> nit: statement

Done.

http://codereview.appspot.com/1852041/diff/1/5#newcode171
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:171:
((AbstractParseTreeNode) chain.getParentNode()).removeChild(chain.node);
On 2010/07/16 01:12:34, johnfargo wrote:
> Specifically, rewriting the Uri of the @import, when !extractImports AFAICT.
> 
> On 2010/07/15 19:17:32, gagan.goku wrote:
> > The block corresponding to
> > if (extractImports) {
> > ...} else {
> > dont remove @import url node.
> > }
> > 
> > is missing.
> 
> 

Done.

http://codereview.appspot.com/1852041/diff/1/5#newcode171
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:171:
((AbstractParseTreeNode) chain.getParentNode()).removeChild(chain.node);
On 2010/07/15 19:17:32, gagan.goku wrote:
> The block corresponding to
> if (extractImports) {
> ...} else {
> dont remove @import url node.
> }
> 
> is missing.

Done.

http://codereview.appspot.com/1852041/diff/1/2
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java
(right):

http://codereview.appspot.com/1852041/diff/1/2#newcode28
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java:28:
import org.apache.shindig.gadgets.parse.caja.CajaCssParser;
On 2010/07/15 19:17:32, gagan.goku wrote:
> Please add @import "http://www.example.org" test case.

Done.

johnfargo

LGTM On 2010/07/20 06:30:08, jasvir wrote: > http://codereview.appspot.com/1852041/diff/1/4 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java > (right): > ...

15 years, 8 months ago (2010-07-21 09:48:49 UTC) #6

LGTM

On 2010/07/20 06:30:08, jasvir wrote:
> http://codereview.appspot.com/1852041/diff/1/4
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java
> (right):
> 
> http://codereview.appspot.com/1852041/diff/1/4#newcode56
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java:56:
> * TODO(jasvir): This ought to be set to the real URI for accurate reporting
> On 2010/07/16 01:12:34, johnfargo wrote:
> > want to just do it in this CL? all calling contexts have the source Uri
> > available.
> 
> I also changed the relative urls like "www.example.org/foo" into actual
absolute
> ones.
> 
> Done.
> 
> http://codereview.appspot.com/1852041/diff/1/5
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
> (right):
> 
> http://codereview.appspot.com/1852041/diff/1/5#newcode167
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:167:
> // Extract the import statment, add its URI to the returned list
> On 2010/07/16 01:12:34, johnfargo wrote:
> > nit: statement
> 
> Done.
> 
> http://codereview.appspot.com/1852041/diff/1/5#newcode171
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:171:
> ((AbstractParseTreeNode) chain.getParentNode()).removeChild(chain.node);
> On 2010/07/16 01:12:34, johnfargo wrote:
> > Specifically, rewriting the Uri of the @import, when !extractImports AFAICT.
> > 
> > On 2010/07/15 19:17:32, gagan.goku wrote:
> > > The block corresponding to
> > > if (extractImports) {
> > > ...} else {
> > > dont remove @import url node.
> > > }
> > > 
> > > is missing.
> > 
> > 
> 
> Done.
> 
> http://codereview.appspot.com/1852041/diff/1/5#newcode171
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:171:
> ((AbstractParseTreeNode) chain.getParentNode()).removeChild(chain.node);
> On 2010/07/15 19:17:32, gagan.goku wrote:
> > The block corresponding to
> > if (extractImports) {
> > ...} else {
> > dont remove @import url node.
> > }
> > 
> > is missing.
> 
> Done.
> 
> http://codereview.appspot.com/1852041/diff/1/2
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java
> (right):
> 
> http://codereview.appspot.com/1852041/diff/1/2#newcode28
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java:28:
> import org.apache.shindig.gadgets.parse.caja.CajaCssParser;
> On 2010/07/15 19:17:32, gagan.goku wrote:
> > Please add @import "http://www.example.org" test case.
> 
> Done.

gagan.goku

Thanks for doing this Jasvir. On Wed, Jul 21, 2010 at 3:18 PM, <johnfargo@gmail.com> wrote: ...

15 years, 8 months ago (2010-07-21 10:53:40 UTC) #7

Thanks for doing this Jasvir.

On Wed, Jul 21, 2010 at 3:18 PM, <johnfargo@gmail.com> wrote:

> LGTM
>
>
> On 2010/07/20 06:30:08, jasvir wrote:
>
>> http://codereview.appspot.com/1852041/diff/1/4
>> File
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1852041/diff/1/4#newcode56
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssParser.java:56:
>
>> * TODO(jasvir): This ought to be set to the real URI for accurate
>>
> reporting
>
>> On 2010/07/16 01:12:34, johnfargo wrote:
>> > want to just do it in this CL? all calling contexts have the source
>>
> Uri
>
>> > available.
>>
>
>  I also changed the relative urls like "www.example.org/foo" into
>>
> actual absolute
>
>> ones.
>>
>
>  Done.
>>
>
>  http://codereview.appspot.com/1852041/diff/1/5
>> File
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1852041/diff/1/5#newcode167
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:167:
>
>> // Extract the import statment, add its URI to the returned list
>> On 2010/07/16 01:12:34, johnfargo wrote:
>> > nit: statement
>>
>
>  Done.
>>
>
>  http://codereview.appspot.com/1852041/diff/1/5#newcode171
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:171:
>
>> ((AbstractParseTreeNode)
>>
> chain.getParentNode()).removeChild(chain.node);
>
>> On 2010/07/16 01:12:34, johnfargo wrote:
>> > Specifically, rewriting the Uri of the @import, when !extractImports
>>
> AFAICT.
>
>> >
>> > On 2010/07/15 19:17:32, gagan.goku wrote:
>> > > The block corresponding to
>> > > if (extractImports) {
>> > > ...} else {
>> > > dont remove @import url node.
>> > > }
>> > >
>> > > is missing.
>> >
>> >
>>
>
>  Done.
>>
>
>  http://codereview.appspot.com/1852041/diff/1/5#newcode171
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriter.java:171:
>
>> ((AbstractParseTreeNode)
>>
> chain.getParentNode()).removeChild(chain.node);
>
>> On 2010/07/15 19:17:32, gagan.goku wrote:
>> > The block corresponding to
>> > if (extractImports) {
>> > ...} else {
>> > dont remove @import url node.
>> > }
>> >
>> > is missing.
>>
>
>  Done.
>>
>
>  http://codereview.appspot.com/1852041/diff/1/2
>> File
>>
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1852041/diff/1/2#newcode28
>>
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java:28:
>
>> import org.apache.shindig.gadgets.parse.caja.CajaCssParser;
>> On 2010/07/15 19:17:32, gagan.goku wrote:
>> > Please add @import "http://www.example.org" test case.
>>
>
>  Done.
>>
>
>
>
> http://codereview.appspot.com/1852041/show
>

Jasvir

Thanks for the review. Sorry to hold off on submitting this. I synced to trunk ...

15 years, 8 months ago (2010-07-23 07:11:59 UTC) #8

Jasvir

StyleTagProxyEmbeddedUrlsVisitorTest.testImportsAndBackgroundUrlsInStyleTag() is incorrect. According the W3C CSS2 spec http://www.w3.org/TR/CSS2/syndata.html#at-rules: CSS 2.1 user agents must ignore ...

15 years, 8 months ago (2010-07-26 03:51:27 UTC) #9

gagan.goku

On Mon, Jul 26, 2010 at 9:21 AM, <jasvir@gmail.com> wrote: > > StyleTagProxyEmbeddedUrlsVisitorTest.testImportsAndBackgroundUrlsInStyleTag() > is ...

15 years, 8 months ago (2010-07-26 04:00:59 UTC) #10

johnfargo

15 years, 8 months ago (2010-07-26 16:59:27 UTC) #11

On Sun, Jul 25, 2010 at 9:00 PM, Gagandeep Singh <gagan.goku@gmail.com>wrote:

>
> On Mon, Jul 26, 2010 at 9:21 AM, <jasvir@gmail.com> wrote:
>
>>
>> StyleTagProxyEmbeddedUrlsVisitorTest.testImportsAndBackgroundUrlsInStyleTag()
>> is incorrect.  According the W3C CSS2 spec
>> http://www.w3.org/TR/CSS2/syndata.html#at-rules:  CSS 2.1 user agents
>> must ignore any '@import' rule that occurs inside a block or after any
>> non-ignored statement other than an @charset or an @import rule.
>
>
>> I've changed the test case to test what I assume was intended to be
>> tested (an unignored @import), synced and submitted.
>>
> Makes sense. Out of curiosity, caja must be ignoring @import statement
> after a non ignorable css statement [ which is probably why that test failed
> :) ]
> If so, is there a flag asking it to not do so ? It could come in handy if
>  we find a user agent that does not ignore such statements and want to
> emulate its behavior.
>
>
>>
>> (Incidentally the submission was made while at 10969m and traveling at
>> the speed of at 822km/h.  Is this a contender for the Shindig patch made
>> at the greatest height and speed? :) )
>
>
> Nice ! Travelling in first class eh :)
> Definitely a solid contestant id say.
>

Contestant, yes. Winner: I regret to say no. I made a commit a few weeks ago
@37k ft and (tailwind assist) 590 mph :P


>
>
>>
>> http://codereview.appspot.com/1852041/show
>>
>
>

Expand All Messages | Collapse All Messages