Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(34)

Issue 2469983007: Fix document title with LINE SEPARATOR/PARAGRAPH SEPARATOR/LINE TABULATION (Closed)

Created:
4 years, 1 month ago by xing.xu
Modified:
4 years, 1 month ago
Reviewers:
tkent, skobes, foolip
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix document title with LINE SEPARATOR/PARAGRAPH SEPARATOR/LINE TABULATION This fix is to align document title with FF(IE fails) and spec. From the spec, "it must replace any sequence of one or more consecutive space characters in that string with a single U+0020 SPACE character". Here "space characters" refer to "U+0020 SPACE, U+0009 CHARACTER TABULATION (tab), U+000A LINE FEED (LF), U+000C FORM FEED (FF), and U+000D CARRIAGE RETURN (CR)". U+2028 LINE SEPARATOR/ U+2029 PARAGRAPH SEPARATOR/ U+000B LINE TABULATION are not listed and should be supported in title. Note, for case fast/dom/Document/document-title-get.html there still exists inconsistency among IE/FF(Win)/FF(Ubuntu). IE: FAIL document.title should be one\u000b\u000c space. Was one space. FF(Win): FAIL document.title should be one\u000b\u000c space. Was one\u000b space. FF(Ubuntu): FAIL document.title should be one\u000b space. Was one\u000b\u000cspace. Spec: https://html.spec.whatwg.org/multipage/dom.html#document.title https://html.spec.whatwg.org/multipage/infrastructure.html#strip-and-collapse-whitespace https://html.spec.whatwg.org/multipage/infrastructure.html#space-character BUG=661859 TEST=imported/wpt/html/dom/documents/dom-tree-accessors/document.title-05.html Committed: https://crrev.com/e27088bd97ee3aecb9d04c56743c629a43b7f621 Cr-Commit-Position: refs/heads/master@{#430508}

Patch Set 1 #

Patch Set 2 : fix dom test case #

Total comments: 1

Patch Set 3 : change magic number to named constants #

Patch Set 4 : upload with --similarity=100 --no-find-copies #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -29 lines) Patch
M third_party/WebKit/LayoutTests/fast/dom/Document/document-title-get-expected.txt View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/Document/script-tests/document-title-get.js View 1 1 chunk +1 line, -1 line 0 comments Download
D third_party/WebKit/LayoutTests/imported/wpt/html/dom/documents/dom-tree-accessors/document.title-05-expected.txt View 1 chunk +0 lines, -24 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 1 chunk +5 lines, -3 lines 1 comment Download

Messages

Total messages: 47 (38 generated)
xing.xu
PTAL, thanks
4 years, 1 month ago (2016-11-03 08:35:47 UTC) #12
skobes
I don't see the test case in the patch, and document.title-05-expected.txt gives me a Rietveld ...
4 years, 1 month ago (2016-11-03 14:31:41 UTC) #15
xing.xu
On 2016/11/03 14:31:41, skobes wrote: > I don't see the test case in the patch, ...
4 years, 1 month ago (2016-11-07 04:53:59 UTC) #29
skobes
lgtm (I filed http://crbug.com/663244 for the Rietveld error.)
4 years, 1 month ago (2016-11-08 03:32:33 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2469983007/60001
4 years, 1 month ago (2016-11-08 03:38:49 UTC) #40
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 1 month ago (2016-11-08 03:45:28 UTC) #42
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/e27088bd97ee3aecb9d04c56743c629a43b7f621 Cr-Commit-Position: refs/heads/master@{#430508}
4 years, 1 month ago (2016-11-08 03:55:19 UTC) #44
tkent
https://codereview.chromium.org/2469983007/diff/60001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2469983007/diff/60001/third_party/WebKit/Source/core/dom/Document.cpp#newcode1316 third_party/WebKit/Source/core/dom/Document.cpp:1316: const UChar lineTabulationCharacter = 0x0b; Please use symbols defined ...
4 years, 1 month ago (2016-11-08 03:57:15 UTC) #46
xing.xu
4 years, 1 month ago (2016-11-09 01:14:42 UTC) #47
Message was sent while issue was closed.
On 2016/11/08 03:57:15, tkent wrote:
>
https://codereview.chromium.org/2469983007/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2469983007/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:1316: const UChar
> lineTabulationCharacter = 0x0b;
> Please use symbols defined in wtf/text/CharacterNames.h.  If it doesn't have
> these character names, please add them to it.

Done by this: https://codereview.chromium.org/2476293003/

Powered by Google App Engine
This is Rietveld 408576698