Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566523003:
Fix meta-theme-color not working outside <head> (Closed)
DescriptionFix meta-theme-color not working outside <head>
Document::themeColor() only looks for meta-theme-color inside <head>, however,
the spec for meta-theme-color does not restrict it to <head>.
BUG=671823
Committed: https://crrev.com/b0dcd96c51e12739407d11a0259e338d7630e150
Cr-Commit-Position: refs/heads/master@{#437619}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add tests #
Total comments: 4
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : update #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpy@chromium.org changed reviewers: + tkent@chromium.org
PTAL.
Please add a test. Probably C++ unit test. https://codereview.chromium.org/2566523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2566523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5400: for (HTMLElement* child = Do you need nested-|for|s? Why don't you do something like: for (HTMLMetaElement* metaElement : ElementTraversal<HTMLMetaElement>::descendantsOf(*documentElement()) { ...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/09 06:07:25, tkent wrote: > Please add a test. Probably C++ unit test. Where should I put the test, somewhere in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF... I guess?
On 2016/12/09 at 07:14:26, lpy wrote: > On 2016/12/09 06:07:25, tkent wrote: > > Please add a test. Probably C++ unit test. > > Where should I put the test, somewhere in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF... I guess? I think you can add it to DocumentTest.cpp.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I updated the patch, ptal. https://codereview.chromium.org/2566523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2566523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5400: for (HTMLElement* child = On 2016/12/09 06:07:25, tkent wrote: > Do you need nested-|for|s? Why don't you do something like: > > for (HTMLMetaElement* metaElement : > ElementTraversal<HTMLMetaElement>::descendantsOf(*documentElement()) { ... Done.
https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5401: Traversal<HTMLMetaElement>::descendantsOf(*documentElement())) { Crash if documentElement() is nullptr. https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right): https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentTest.cpp:601: EXPECT_EQ(themeColor.red(), 0); The first argument should be an expected value. Also, we can simplify these lines like: EXPECT_EQ(Color(0, 255, 0), document().themeColor()) << "Theme color should be bright green.";
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, please take another look. https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5401: Traversal<HTMLMetaElement>::descendantsOf(*documentElement())) { On 2016/12/09 08:48:38, tkent wrote: > Crash if documentElement() is nullptr. Done. https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right): https://codereview.chromium.org/2566523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentTest.cpp:601: EXPECT_EQ(themeColor.red(), 0); On 2016/12/09 08:48:38, tkent wrote: > The first argument should be an expected value. > Also, we can simplify these lines like: > EXPECT_EQ(Color(0, 255, 0), document().themeColor()) << "Theme color should be > bright green."; Done.
lgtm https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5400: auto de = documentElement(); 'de' isn't a good name. 'rootElement' or something. https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right): https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentTest.cpp:599: "Theme color should be bright green."); This sentence isn't necessary. https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentTest.cpp:608: "Theme color should be bright green."); This sentence isn't necessary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5400: auto de = documentElement(); On 2016/12/09 09:20:53, tkent wrote: > 'de' isn't a good name. 'rootElement' or something. Done. https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right): https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentTest.cpp:599: "Theme color should be bright green."); On 2016/12/09 09:20:53, tkent wrote: > This sentence isn't necessary. Done. https://codereview.chromium.org/2566523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentTest.cpp:608: "Theme color should be bright green."); On 2016/12/09 09:20:53, tkent wrote: > This sentence isn't necessary. Done.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org Link to the patchset: https://codereview.chromium.org/2566523003/#ps60001 (title: "update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481306021287750,
"parent_rev": "5a9af98d8041c9bb98c545a5507ec71d809acb8a", "commit_rev":
"fe2273aaca10d22c335f976779db35627640f6e1"}
Message was sent while issue was closed.
Description was changed from ========== Fix meta-theme-color not working outside <head> Document::themeColor() only looks for meta-theme-color inside <head>, however, the spec for meta-theme-color does not restrict it to <head>. BUG=671823 ========== to ========== Fix meta-theme-color not working outside <head> Document::themeColor() only looks for meta-theme-color inside <head>, however, the spec for meta-theme-color does not restrict it to <head>. BUG=671823 Review-Url: https://codereview.chromium.org/2566523003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix meta-theme-color not working outside <head> Document::themeColor() only looks for meta-theme-color inside <head>, however, the spec for meta-theme-color does not restrict it to <head>. BUG=671823 Review-Url: https://codereview.chromium.org/2566523003 ========== to ========== Fix meta-theme-color not working outside <head> Document::themeColor() only looks for meta-theme-color inside <head>, however, the spec for meta-theme-color does not restrict it to <head>. BUG=671823 Committed: https://crrev.com/b0dcd96c51e12739407d11a0259e338d7630e150 Cr-Commit-Position: refs/heads/master@{#437619} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b0dcd96c51e12739407d11a0259e338d7630e150 Cr-Commit-Position: refs/heads/master@{#437619} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
