Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(71)

Issue 5564089086509056: Issue 1801 - Use URL objects to process URLs in the background page (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by Sebastian Noack
Modified:
5 years ago
Reviewers:
Wladimir Palant, kzar
Visibility:
Public.

Description

Currently we pass URL strings around, using regular expressions (e.g. to check for the protocol) or parsing them every time we need to extract the domain. In regard to correct handling of IDN domains (currently most of the code ignores them, for more details read the issue) things become even less optimal with the current approach. With this patch, URLs are parsed immediately, and are stored/passed as URL object. Therefore I removed lib/basedomain.js, and moved/added helper functions to lib/url.js, which operate on URL objects and handle IDN domains correctly.

Patch Set 1 : #

Total comments: 23

Patch Set 2 : Rebased and addressed comments #

Total comments: 8

Patch Set 3 : Rebased and addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+335 lines, -334 lines) Patch
M background.js View 4 chunks +15 lines, -6 lines 0 comments Download
M chrome/ext/background.js View 1 2 8 chunks +8 lines, -7 lines 0 comments Download
R lib/basedomain.js View 1 chunk +0 lines, -173 lines 0 comments Download
M lib/url.js View 1 2 1 chunk +113 lines, -0 lines 0 comments Download
M lib/whitelisting.js View 1 4 chunks +26 lines, -18 lines 0 comments Download
M metadata.common View 1 chunk +0 lines, -1 line 0 comments Download
M popup.js View 5 chunks +9 lines, -9 lines 0 comments Download
M popupBlocker.js View 2 chunks +12 lines, -12 lines 0 comments Download
M qunit/index.html View 1 chunk +1 line, -1 line 0 comments Download
R qunit/tests/baseDomain.js View 1 chunk +0 lines, -90 lines 0 comments Download
A qunit/tests/url.js View 1 2 1 chunk +136 lines, -0 lines 0 comments Download
M safari/ext/background.js View 1 2 4 chunks +8 lines, -8 lines 0 comments Download
M safari/ext/content.js View 2 chunks +2 lines, -4 lines 0 comments Download
M webrequest.js View 4 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 10
Sebastian Noack
5 years ago (2015-01-22 14:42:57 UTC) #1
kzar
I like these changes, mostly seems to make the code easier to read :) Can't ...
5 years ago (2015-01-26 10:27:38 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js#newcode76 lib/url.js:76: for (; frame; frame = frame.parent) On 2015/01/26 10:27:38, ...
5 years ago (2015-01-26 10:38:39 UTC) #3
kzar
Again, think Wladimir should check this (and also comment on the loop) but LGTM. http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js ...
5 years ago (2015-01-26 17:19:05 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js#newcode76 lib/url.js:76: for (; frame; frame = frame.parent) On 2015/01/26 17:19:05, ...
5 years ago (2015-01-26 17:26:30 UTC) #5
Wladimir Palant
http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chrome/ext/background.js#newcode32 chrome/ext/background.js:32: Object.defineProperty(this, "url", {value: new URL(tab.url), enumerable: true}); Code that ...
5 years ago (2015-02-09 12:54:29 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chrome/ext/background.js#newcode32 chrome/ext/background.js:32: Object.defineProperty(this, "url", {value: new URL(tab.url), enumerable: true}); On 2015/02/09 ...
5 years ago (2015-02-11 10:55:51 UTC) #7
Wladimir Palant
http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/url.js#newcode76 lib/url.js:76: for (; frame; frame = frame.parent) On 2015/01/26 17:19:05, ...
5 years ago (2015-02-11 13:45:45 UTC) #8
Sebastian Noack
http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chrome/ext/background.js#newcode33 chrome/ext/background.js:33: // usually our Page objects are created from Chrome's ...
5 years ago (2015-02-11 17:07:06 UTC) #9
Wladimir Palant
5 years ago (2015-02-11 18:33:11 UTC) #10
LGTM
Sign in to reply to this message.

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