Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4567408790470656: Issue 1393 - Block element dialog suggests decoded URLs (Closed)

Created:
Sept. 18, 2014, 3:10 p.m. by Thomas Greiner
Modified:
Sept. 25, 2014, 9:50 a.m.
Visibility:
Public.

Description

Issue 1393 - Block element dialog suggests decoded URLs

Patch Set 1 #

Total comments: 6

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -6 lines) Patch
M include.postload.js View 1 2 2 chunks +4 lines, -6 lines 8 comments Download

Messages

Total messages: 19
Thomas Greiner
Sept. 18, 2014, 3:12 p.m. (2014-09-18 15:12:55 UTC) #1
Sebastian Noack
LGTM
Sept. 18, 2014, 3:21 p.m. (2014-09-18 15:21:21 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js#newcode390 include.postload.js:390: url = encodeURI(url); What if the URL we get ...
Sept. 18, 2014, 6:53 p.m. (2014-09-18 18:53:02 UTC) #3
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js#newcode390 include.postload.js:390: url = encodeURI(url); On 2014/09/18 18:53:02, Wladimir Palant wrote: ...
Sept. 19, 2014, 10:13 a.m. (2014-09-19 10:13:31 UTC) #4
Thomas Greiner
The problem with retrieving the value from Element.src is that we're getting a whitespace instead ...
Sept. 19, 2014, 10:31 a.m. (2014-09-19 10:31:12 UTC) #5
Thomas Greiner
http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js#newcode390 include.postload.js:390: url = encodeURI(url); Now I see what you mean. ...
Sept. 19, 2014, 10:47 a.m. (2014-09-19 10:47:12 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js#newcode390 include.postload.js:390: url = encodeURI(url); On 2014/09/19 10:47:13, Thomas Greiner wrote: ...
Sept. 19, 2014, 11:14 a.m. (2014-09-19 11:14:50 UTC) #7
Wladimir Palant
http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js#newcode390 include.postload.js:390: url = encodeURI(url); On 2014/09/19 11:14:50, Sebastian Noack wrote: ...
Sept. 21, 2014, 6:37 p.m. (2014-09-21 18:37:42 UTC) #8
Thomas Greiner
http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5629499534213120/include.postload.js#newcode390 include.postload.js:390: url = encodeURI(url); On 2014/09/21 18:37:42, Wladimir Palant wrote: ...
Sept. 22, 2014, 9:37 a.m. (2014-09-22 09:37:33 UTC) #9
Wladimir Palant
http://codereview.adblockplus.org/4567408790470656/diff/5750085036015616/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5750085036015616/include.postload.js#newcode349 include.postload.js:349: // The URL is not always normalized, so do ...
Sept. 22, 2014, 9:57 a.m. (2014-09-22 09:57:02 UTC) #10
Thomas Greiner
http://codereview.adblockplus.org/4567408790470656/diff/5750085036015616/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5750085036015616/include.postload.js#newcode349 include.postload.js:349: // The URL is not always normalized, so do ...
Sept. 22, 2014, 1:13 p.m. (2014-09-22 13:13:48 UTC) #11
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); I just realized that Chrome preserves ...
Sept. 22, 2014, 1:27 p.m. (2014-09-22 13:27:48 UTC) #12
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); Also unencoded special characters aren't encoded ...
Sept. 22, 2014, 1:37 p.m. (2014-09-22 13:37:36 UTC) #13
Thomas Greiner
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); On 2014/09/22 13:27:49, Sebastian Noack wrote: ...
Sept. 22, 2014, 3:08 p.m. (2014-09-22 15:08:28 UTC) #14
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); On 2014/09/22 15:08:28, Thomas Greiner wrote: ...
Sept. 22, 2014, 3:25 p.m. (2014-09-22 15:25:16 UTC) #15
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); On 2014/09/22 15:25:16, Sebastian Noack wrote: ...
Sept. 22, 2014, 3:50 p.m. (2014-09-22 15:50:59 UTC) #16
Wladimir Palant
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); On 2014/09/22 15:50:59, Sebastian Noack wrote: ...
Sept. 22, 2014, 5:39 p.m. (2014-09-22 17:39:21 UTC) #17
Sebastian Noack
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/include.postload.js#newcode349 include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url)); On 2014/09/22 17:39:21, Wladimir Palant wrote: ...
Sept. 23, 2014, 9:40 a.m. (2014-09-23 09:40:33 UTC) #18
Sebastian Noack
Sept. 25, 2014, 7:53 a.m. (2014-09-25 07:53:18 UTC) #19
http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/incl...
include.postload.js:349: url = normalizeURL(relativeToAbsoluteUrl(url));
On 2014/09/23 09:40:34, Sebastian Noack wrote:
> On 2014/09/22 17:39:21, Wladimir Palant wrote:
> > On 2014/09/22 15:50:59, Sebastian Noack wrote:
> > > Actually using an <iframe> element is notable faster (for some reason).
Also
> > > caching the element gives a significant boost:
> http://jsperf.com/normailzeurl
> > 
> > That should be because Chrome does some caching of its own for frames,
please
> > compare: http://jsperf.com/normalizeurl
> > 
> > So I'd be in favor of using the <a> element, if only to avoid the impression
> > that something is being downloaded there.
> 
> Very interesting. So I agree with your point for using an <a> element here.
> Though <iframe> elements still seem to be faster when cached, while caching
the
> <a> element doesn't make much a difference, on Chrome:
> http://jsperf.com/normalizeurl/2
> 
> However, caching the <a> element might still be worth for Safari, also
> considering that we have to resolve the URL for every request there. 
> 
> > Note that even then we won't get it entirely correct: in case of the
<object>
> > and <applet> tags the URL resolution is being performed by the plugin, and
it
> > might use a different way to resolve URLs from what the browser does (I've
hit
> > that issue with Java before). Plus, there can be different base URLs in a
> > document (see Node.baseURI). Still, this approach is a lot better than the
> > current hack.
> 
> That is true, but there is probably no way to resolve the URL correctly for
> every possible plugin. And reusing the built-in URL normalization of the
> browser, instead rolling our own, reduces at least complexity and is also
> faster.

LGTM. I created a separate issue, for using an <a> element, when explicitly
resolving URLs: https://issues.adblockplus.org/ticket/1441

Powered by Google App Engine
This is Rietveld