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

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:
4 years, 12 months ago by Sebastian Noack
Modified:
4 years, 11 months 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
4 years, 12 months 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 ...
4 years, 11 months 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, ...
4 years, 11 months 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 ...
4 years, 11 months 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, ...
4 years, 11 months 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 ...
4 years, 11 months 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 ...
4 years, 11 months 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, ...
4 years, 11 months 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 ...
4 years, 11 months ago (2015-02-11 17:07:06 UTC) #9
Wladimir Palant
4 years, 11 months 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