Summary: | Visited link hash should be computed only once | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, darin, ews-watchlist, ggaren, rniwa | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2019-04-24 03:09:18 PDT
Created attachment 368116 [details]
wip
Attachment 368116 [details] did not pass style-queue:
ERROR: Source/WebCore/svg/SVGAElement.h:25: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/dom/VisitedLinkState.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 368121 [details]
patch
Created attachment 368122 [details]
patch
Comment on attachment 368122 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=368122&action=review > Source/WebCore/html/HTMLAnchorElement.h:119 > + // This is computed only once and must not be affected by subsequent URL changes. > + mutable Optional<SharedStringHash> m_storedVisitedLinkHash; Would mutable Optional<const SharedStringHash> work? > Would mutable Optional<const SharedStringHash> work?
Doesn't compile.
Comment on attachment 368122 [details] patch Rejecting attachment 368122 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 368122, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Traceback (most recent call last): File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: https://webkit-queues.webkit.org/results/11983672 Created attachment 368219 [details]
patch
Comment on attachment 368219 [details] patch Rejecting attachment 368219 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 368219, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Traceback (most recent call last): File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: https://webkit-queues.webkit.org/results/11993303 Created attachment 368223 [details]
patch
Comment on attachment 368223 [details] patch Clearing flags on attachment: 368223 Committed r244642: <https://trac.webkit.org/changeset/244642> All reviewed patches have been landed. Closing bug. Is this change OK from a user experience point of view? We believe the user experience won't change because we believe that non-attacker pages don't reassign the href value after rendering a link element. If we're right, this policy is the clearest, and maybe even the easiest to specify in the future. The most likely failure mode we've imagined is a page where all the links turn purple because they start out as href="" and then JavaScript fills in non-empty href values later. ("" is considered visited because it resolves to the current page.) If we do find pages with links that start out with href="a.html" and change to href="b.html", alternatives we've discussed are: (a) throttle style changes triggered by href changes using a timer; (b) unconditionally force a full style recalc for all href changes, even if nothing has changed. Note that href mutations did not reliably produce correct visited styling before this change either, because we didn't actually invalidate the style. It only worked if some other change triggered a style recalc (like in the test case). |