Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 1077483002: Truncate long URL fragments in Android page info popup (Closed)

Created:
4 years, 1 month ago by tsergeant
Modified:
4 years, 1 month ago
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Truncate long URL fragments in Android page info popup This replaces percent-encoding whitespace as a method to combat the use of crafted URL fragments to inject messages into the page info popup. By truncating the URL so that at most two lines of the fragment are shown, we prevent lengthy messages from being injected while minimising the effect on most URLs. BUG=466351 Committed: https://crrev.com/5bf8a2dccf4777c5f065d918d1d9a2bbaa013f9f Cr-Commit-Position: refs/heads/master@{#325358}

Patch Set 1 #

Patch Set 2 : Remove test #

Patch Set 3 : Formatting #

Total comments: 2

Patch Set 4 : Handle URLs with no origin truncation #

Total comments: 2

Patch Set 5 : Actually handle URLs with no origin truncation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -81 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java View 1 2 3 4 7 chunks +41 lines, -41 lines 0 comments Download
D chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java View 1 1 chunk +0 lines, -40 lines 0 comments Download

Messages

Total messages: 13 (3 generated)
tsergeant
dtrainor@, can you please review? sashab@, can you take a quick look if you get ...
4 years, 1 month ago (2015-04-15 01:10:58 UTC) #2
juhonurm
https://codereview.chromium.org/1077483002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1077483002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode135 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:135: int originEndIndex = OmniboxUrlEmphasizer.getOriginEndIndex(urlText, mProfile); OmniboxUrlEmphasizer.getOriginEndIndex only works with ...
4 years, 1 month ago (2015-04-15 05:00:41 UTC) #4
tsergeant
Thanks, good pickup. https://codereview.chromium.org/1077483002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1077483002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode135 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:135: int originEndIndex = OmniboxUrlEmphasizer.getOriginEndIndex(urlText, mProfile); On ...
4 years, 1 month ago (2015-04-15 05:47:20 UTC) #5
juhonurm
https://codereview.chromium.org/1077483002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1077483002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode150 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:150: fragmentStartIndex = urlText.length(); Now the original issue is ignored ...
4 years, 1 month ago (2015-04-15 06:07:58 UTC) #6
tsergeant
/facepalm Thanks, actually fixed this time. https://codereview.chromium.org/1077483002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1077483002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode150 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:150: fragmentStartIndex = urlText.length(); ...
4 years, 1 month ago (2015-04-15 07:13:54 UTC) #7
juhonurm
lgtm
4 years, 1 month ago (2015-04-15 13:26:08 UTC) #8
David Trainor- moved to gerrit
lgtm
4 years, 1 month ago (2015-04-15 16:30:04 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1077483002/80001
4 years, 1 month ago (2015-04-16 00:12:37 UTC) #11
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 1 month ago (2015-04-16 00:36:30 UTC) #12
commit-bot: I haz the power
4 years, 1 month ago (2015-04-16 00:37:21 UTC) #13
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/5bf8a2dccf4777c5f065d918d1d9a2bbaa013f9f
Cr-Commit-Position: refs/heads/master@{#325358}

Powered by Google App Engine
This is Rietveld 408576698