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

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

Created:
4 years, 4 months ago by tsergeant
Modified:
4 years, 4 months 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, 4 months 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, 4 months 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, 4 months 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, 4 months 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, 4 months ago (2015-04-15 07:13:54 UTC) #7
juhonurm
lgtm
4 years, 4 months ago (2015-04-15 13:26:08 UTC) #8
David Trainor- moved to gerrit
lgtm
4 years, 4 months 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, 4 months ago (2015-04-16 00:12:37 UTC) #11
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 4 months ago (2015-04-16 00:36:30 UTC) #12
commit-bot: I haz the power
4 years, 4 months 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