WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
185958
NavigationAction does not need to hold initiating DOM Event
https://bugs.webkit.org/show_bug.cgi?id=185958
Summary
NavigationAction does not need to hold initiating DOM Event
Daniel Bates
Reported
2018-05-24 13:06:48 PDT
Simon Fraser mentioned in
bug 185712, comment 4
that NavigationAction retains a DOM Event, which ultimately keeps the document associated with the DOM Event alive. It is sufficient for NavigationAction to own details of the initiating DOM Event as opposed to the DOM Event itself.
Attachments
Patch
(20.82 KB, patch)
2018-05-24 13:22 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch
(20.82 KB, patch)
2018-05-25 16:52 PDT
,
Daniel Bates
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-05-24 13:07:24 PDT
<
rdar://problem/40531539
>
Daniel Bates
Comment 2
2018-05-24 13:22:29 PDT
Created
attachment 341221
[details]
Patch This patch depends on the patch for
bug #185712
.
Daniel Bates
Comment 3
2018-05-25 16:52:14 PDT
Created
attachment 341356
[details]
Patch
Simon Fraser (smfr)
Comment 4
2018-05-25 17:08:39 PDT
Comment on
attachment 341356
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=341356&action=review
> Source/WebCore/loader/NavigationAction.cpp:96 > NavigationAction::NavigationAction(Document& requester, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, NavigationType type, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute)
Maybe add a comment in the header file to say that this class shouldn't hold on to any objects from the originating document.
> Source/WebCore/loader/NavigationAction.h:93 > + bool buttonDown; > + unsigned short button; > + unsigned short syntheticClickType; > + LayoutPoint absoluteLocation; > + FloatPoint locationInRootViewCoordinates;
This would pack better with the larger types first.
Daniel Bates
Comment 5
2018-05-30 16:23:50 PDT
Comment on
attachment 341356
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=341356&action=review
>> Source/WebCore/loader/NavigationAction.cpp:96 >> NavigationAction::NavigationAction(Document& requester, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, NavigationType type, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute) > > Maybe add a comment in the header file to say that this class shouldn't hold on to any objects from the originating document.
Will add the following comment above the class definition in the header before landing as formatted: // NavigationAction should never hold a strong reference to the originating document either directly // or indirectly as doing so prevents its destruction even after navigating away from it because // DocumentLoader keeps around the NavigationAction for the last navigation. Will also add the following comment below the "private" section in the header before landing as formatted: // Do not add a strong reference to the originating document or a subobject that holds the // originating document. See comment above the class for more details.
>> Source/WebCore/loader/NavigationAction.h:93 >> + FloatPoint locationInRootViewCoordinates; > > This would pack better with the larger types first.
Will order these fields as follows: locationInRootViewCoordinates, absoluteLocation, button, syntheticClickType, buttonDown.
Daniel Bates
Comment 6
2018-05-30 16:30:21 PDT
Committed
r232316
: <
https://trac.webkit.org/changeset/232316
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug