Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1910433002:
Invalidate LinkHash state for links in shadow roots (Closed)
|
Created:
4 years, 8 months ago by kochi Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, hayato, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInvalidate LinkHash state for links in shadow roots
When link history state changed, :visited style can change, but
it used to work only for links in document node tree.
This CL fixes to make it work also for links in shadow node trees.
Writing a layout test for this is harder than expected; tried using
<iframe> to change history state, but it did not working as expected.
Here's a manual test for this case.
http://jsbin.com/yipozul/6/edit?html,output
BUG=405707
TEST=manual
Committed: https://crrev.com/62f710730c5aa2fa021a6eee7487c5d799e867f3
Cr-Commit-Position: refs/heads/master@{#388710}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Add missing include. #
Total comments: 4
Patch Set 4 : fix for comments #
Total comments: 2
Messages
Total messages: 27 (11 generated)
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910433002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910433002/1
kochi@chromium.org changed reviewers: + tkent@chromium.org
tkent-san, could you review?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
the compile failure was due to a missing include (ElementShadow.h) which defined a inline method (Node::youngestShadowRoot()). Now compilation should be fine. tkent-san, could you take a look?
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910433002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm. I understand making a test for this is hard. As for C++ unit test, "checking if pseudoStateChanged() was called" looks difficult to me. https://codereview.chromium.org/1910433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/VisitedLinkState.cpp (right): https://codereview.chromium.org/1910433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:82: if (m_linksCheckedForVisitedState.isEmpty()) nit: if (!m_linksCheckedForVisitedState.isEmpty()) invalidateStyleForAllLinksRecursively(*document().firstChild(), invalidateVisitedLinkHashes); is simpler. https://codereview.chromium.org/1910433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:103: if (!m_linksCheckedForVisitedState.contains(linkHash)) nit: if (m_linksCheckedForVisitedState.contains(linkHash)) invalidateStyleForLinkRecursively(*document().firstChild(), linkHash); is simpler.
Thanks for the review. https://codereview.chromium.org/1910433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/VisitedLinkState.cpp (right): https://codereview.chromium.org/1910433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:82: if (m_linksCheckedForVisitedState.isEmpty()) On 2016/04/21 03:55:23, tkent wrote: > nit: > if (!m_linksCheckedForVisitedState.isEmpty()) > invalidateStyleForAllLinksRecursively(*document().firstChild(), > invalidateVisitedLinkHashes); > > is simpler. Done. https://codereview.chromium.org/1910433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:103: if (!m_linksCheckedForVisitedState.contains(linkHash)) On 2016/04/21 03:55:23, tkent wrote: > nit: > if (m_linksCheckedForVisitedState.contains(linkHash)) > invalidateStyleForLinkRecursively(*document().firstChild(), linkHash); > is simpler. Done.
The CQ bit was checked by kochi@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/1910433002/#ps60001 (title: "fix for comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910433002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910433002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kochi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910433002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910433002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
You can remove all the kids from a Document before this IPC shows up. You can't assume a Document has kids. https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/VisitedLinkState.cpp (right): https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:83: invalidateStyleForAllLinksRecursively(*document().firstChild(), invalidateVisitedLinkHashes); I think the document firstChild can be null here. This would crash. https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:103: invalidateStyleForLinkRecursively(*document().firstChild(), linkHash); Ditto.
Message was sent while issue was closed.
On 2016/04/21 08:14:04, esprehn wrote: > You can remove all the kids from a Document before this IPC shows up. You can't > assume a Document has kids. > > https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/VisitedLinkState.cpp (right): > > https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:83: > invalidateStyleForAllLinksRecursively(*document().firstChild(), > invalidateVisitedLinkHashes); > I think the document firstChild can be null here. This would crash. > > https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:103: > invalidateStyleForLinkRecursively(*document().firstChild(), linkHash); > Ditto. Thanks for the comment, will work on another CL immediately.
Message was sent while issue was closed.
On 2016/04/21 08:59:18, kochi wrote: > On 2016/04/21 08:14:04, esprehn wrote: > > You can remove all the kids from a Document before this IPC shows up. You > can't > > assume a Document has kids. > > > > > https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/VisitedLinkState.cpp (right): > > > > > https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:83: > > invalidateStyleForAllLinksRecursively(*document().firstChild(), > > invalidateVisitedLinkHashes); > > I think the document firstChild can be null here. This would crash. > > > > > https://codereview.chromium.org/1910433002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:103: > > invalidateStyleForLinkRecursively(*document().firstChild(), linkHash); > > Ditto. > > Thanks for the comment, will work on another CL immediately. https://codereview.chromium.org/1910433002/
Message was sent while issue was closed.
Description was changed from ========== Invalidate LinkHash state for links in shadow roots When link history state changed, :visited style can change, but it used to work only for links in document node tree. This CL fixes to make it work also for links in shadow node trees. Writing a layout test for this is harder than expected; tried using <iframe> to change history state, but it did not working as expected. Here's a manual test for this case. http://jsbin.com/yipozul/6/edit?html,output BUG=405707 TEST=manual ========== to ========== Invalidate LinkHash state for links in shadow roots When link history state changed, :visited style can change, but it used to work only for links in document node tree. This CL fixes to make it work also for links in shadow node trees. Writing a layout test for this is harder than expected; tried using <iframe> to change history state, but it did not working as expected. Here's a manual test for this case. http://jsbin.com/yipozul/6/edit?html,output BUG=405707 TEST=manual Committed: https://crrev.com/62f710730c5aa2fa021a6eee7487c5d799e867f3 Cr-Commit-Position: refs/heads/master@{#388710} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/62f710730c5aa2fa021a6eee7487c5d799e867f3 Cr-Commit-Position: refs/heads/master@{#388710} |
