|
|
Created:
4 years ago by lpy Modified:
4 years ago Reviewers:
tkent CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, gavinp+prerender_chromium.org, rwlbuis, sof, Yoav Weiss Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix empty href for favicon link element causing page to be fetched twice.
Accroding to HTML spec
https://html.spec.whatwg.org/multipage/semantics.html#the-link-element, if the
href attribute in link element is empty, then the element does not define a link
and should be ignored.
BUG=669351
Committed: https://crrev.com/24d87e2f1a18e9cc58a37126b85c921ea37ba695
Cr-Commit-Position: refs/heads/master@{#438762}
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 8
Patch Set 3 : Update test #
Total comments: 2
Patch Set 4 : Update test #
Messages
Total messages: 35 (24 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #1 (id:1) has been deleted
lpy@chromium.org changed reviewers: + tkent@chromium.org
PTAL. I assume I should put the test in html/, but I am not 100% sure where to put it. Any suggestion?
> I assume I should put the test in html/, but I am not 100% sure where to put it. Any suggestion? This CL should add core/html/HTMLLinkElementTest.cpp.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
On 2016/12/15 01:02:23, tkent wrote: > > I assume I should put the test in html/, but I am not 100% sure where to put > it. Any suggestion? > > This CL should add core/html/HTMLLinkElementTest.cpp. Thanks, ptal.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp (right): https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:11: Remove the blank line https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:21: void TearDown() override { ThreadState::current()->collectAllGarbage(); } We don't need collectAllGarbage() here. There are no tests sensitive to GC. https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:37: document().view()->updateAllLifecyclePhases(); updateAllLifecyclePhases() is unnecessary. Then, HTMLLinkElementTest::setHtmlInnerHTML() would be unnecessary and setInnerHTML() should be called directly. https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:42: TEST_F(HTMLLinkElementTest, href) { The test name should be more specific. e.g. TEST_F(HTMLLinkElementTest, EmptyHrefAttribute)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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/2575273002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp (right): https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:11: On 2016/12/15 01:20:03, tkent wrote: > Remove the blank line Done. https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:21: void TearDown() override { ThreadState::current()->collectAllGarbage(); } On 2016/12/15 01:20:03, tkent wrote: > We don't need collectAllGarbage() here. There are no tests sensitive to GC. > Done. https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:37: document().view()->updateAllLifecyclePhases(); On 2016/12/15 01:20:03, tkent wrote: > updateAllLifecyclePhases() is unnecessary. Then, > HTMLLinkElementTest::setHtmlInnerHTML() would be unnecessary and setInnerHTML() > should be called directly. Done. https://codereview.chromium.org/2575273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:42: TEST_F(HTMLLinkElementTest, href) { On 2016/12/15 01:20:03, tkent wrote: > The test name should be more specific. e.g. TEST_F(HTMLLinkElementTest, > EmptyHrefAttribute) Done.
https://codereview.chromium.org/2575273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp (right): https://codereview.chromium.org/2575273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:19: void TearDown() override {} Remove this line. You don't need to override it.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2575273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp (right): https://codereview.chromium.org/2575273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElementTest.cpp:19: void TearDown() override {} On 2016/12/15 01:29:11, tkent wrote: > Remove this line. You don't need to override it. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org
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": 80001, "attempt_start_ts": 1481780987984720, "parent_rev": "b88a832c470691605b477b55426216316ef93a0b", "commit_rev": "700d8b1e119ba57653245c2f4d282c9b10be38c5"}
Message was sent while issue was closed.
Description was changed from ========== Fix empty href for favicon link element causing page to be fetched twice. Accroding to HTML spec https://html.spec.whatwg.org/multipage/semantics.html#the-link-element, if the href attribute in link element is empty, then the element does not define a link and should be ignored. BUG=669351 ========== to ========== Fix empty href for favicon link element causing page to be fetched twice. Accroding to HTML spec https://html.spec.whatwg.org/multipage/semantics.html#the-link-element, if the href attribute in link element is empty, then the element does not define a link and should be ignored. BUG=669351 Review-Url: https://codereview.chromium.org/2575273002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix empty href for favicon link element causing page to be fetched twice. Accroding to HTML spec https://html.spec.whatwg.org/multipage/semantics.html#the-link-element, if the href attribute in link element is empty, then the element does not define a link and should be ignored. BUG=669351 Review-Url: https://codereview.chromium.org/2575273002 ========== to ========== Fix empty href for favicon link element causing page to be fetched twice. Accroding to HTML spec https://html.spec.whatwg.org/multipage/semantics.html#the-link-element, if the href attribute in link element is empty, then the element does not define a link and should be ignored. BUG=669351 Committed: https://crrev.com/24d87e2f1a18e9cc58a37126b85c921ea37ba695 Cr-Commit-Position: refs/heads/master@{#438762} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/24d87e2f1a18e9cc58a37126b85c921ea37ba695 Cr-Commit-Position: refs/heads/master@{#438762} |