Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(215)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 1632042: Removing conversion of & to & and vice versa when generating url (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 10 months ago by gagan.goku
Modified:
15 years, 5 months ago
Reviewers:
johnfargo, zhoresh, shindig.remailer, dev-remailer
CC:
anupama.dutta, pradnya, vikaas.arora, cool-shindig-committers_googlegroups.com
Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/
Visibility:
Public.

Description

Currently we have code that converts & to & and then & in url back to &. There is a bug right now in the way that dynamic nodes are added (say from StyleConcatVisitor) for which the reconversion from & to & does not happen.

Patch Set 1 #

Patch Set 2 : removing_unnecessary_imports #

Total comments: 2

Patch Set 3 : uploading_all_changes_before_svn_up #

Patch Set 4 : adding_option_for_not_escaping_ampersand #

Patch Set 5 : removing_unnecessary_files #

Unified diffs Side-by-side diffs Delta from patch set Stats (+188 lines, -8 lines) Patch
M .gitignore View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java View 1 2 3 4 2 chunks +3 lines, -4 lines 0 comments Download
M java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java View 4 3 chunks +7 lines, -3 lines 0 comments Download
M java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
A java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializerTest.java View 1 chunk +170 lines, -0 lines 0 comments Download

Messages

Total messages: 6
zhoresh
Good catch in the MutableContent! http://codereview.appspot.com/1632042/diff/2001/3003 File java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java (left): http://codereview.appspot.com/1632042/diff/2001/3003#oldcode169 java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java:169: elem.getNamespaceURI() == null && ...
15 years, 10 months ago (2010-06-09 23:04:12 UTC) #1
gagan.goku
On 2010/06/09 23:04:12, zhoresh wrote: > Good catch in the MutableContent! > Thanks > http://codereview.appspot.com/1632042/diff/2001/3003 ...
15 years, 9 months ago (2010-06-13 05:20:23 UTC) #2
zhoresh
If I understand correctly, the html definition call for escaping & with &, if only ...
15 years, 9 months ago (2010-06-14 17:12:51 UTC) #3
johnfargo
+1 to that. The reason you can't copy-and-paste the URL into your browser bar is ...
15 years, 9 months ago (2010-06-14 17:22:23 UTC) #4
gagan.goku
On 2010/06/14 17:22:23, johnfargo wrote: > +1 to that. The reason you can't copy-and-paste the ...
15 years, 9 months ago (2010-06-14 22:06:09 UTC) #5
johnfargo
15 years, 9 months ago (2010-06-14 23:52:52 UTC) #6 
On Mon, Jun 14, 2010 at 3:06 PM, <gagan.goku@gmail.com> wrote:

> On 2010/06/14 17:22:23, johnfargo wrote:
>
>> +1 to that. The reason you can't copy-and-paste the URL into your
>>
> browser
>
>> bar is that the URL is unescaped by the browser when it's the value of
>>
> a
>
>> target attribute ie src="value".
>>
>
> I chatted with zhoresh and i think now i understand why the browser
> would want all &'s to be replaced by &amp;
> So ideally, the best thing would be to take care of all the & escaping
> in html serializer itself.
> However, I would not want to change something that breaks current
> functionality :(
>
> I'l try enumerating all possible cases and try to change only the
> DefaultHtmlSerializer file.  This way, all the logic for escaping "&"
> and other escape characters like "quotes" will reside in only place and
> it would be cleaner.
> If you feel that approach is better, we can slowly start removing
> stringreplace("&", "&amp;") from places like  ConcatVisitor and
> RenderingGadgetRewriter after we are sure that we nailed down all
> possible scenarios.
>
> Sound reasonable ?
>

That sounds reasonable to me. I agree that proper encapsulation has
HtmlSerializer taking care of this exclusively.


>
> Btw, the Copy Location url not working when pasted into browser
> navigation bar is really annoying. The first thing that comes to mind is
> that somehow the url was not generated correctly.
> Maybe we should find out why google.com does not choose to escape &.
> Does google not bother about html standard, or does it assume that most
> famous browsers already take care of it ?


Good question, and worth asking to various parties. Anyone from Shindig know
why this is generally-accepted practice?


>
>
>  Re: the XSRF issue on codereview, that happens occasionally with the
>>
> tool
>
>> and is orthogonal. I've seen it occur when you start an operation but
>>
> don't
>
>> complete it within a period of time (in the tool). A refresh usually
>>
> fixes
>
>> the problem.
>>
>
> Yeah this went away when i refreshed, but my original comment was lost
> :(


Sadness :(


>
>  --j
>>
>
>  On Mon, Jun 14, 2010 at 10:12 AM, Ziv Horesh
>>
> <mailto:zhoresh@gmail.com> wrote:
>
>  >
>>
>
>
> http://codereview.appspot.com/1632042/show
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b