Bug 236871

Summary: Optimize DOM storage event dispatch
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ews-watchlist, ggaren, hi, japhet, joepeck, pangle, sam, sihui_liu, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 236985    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2022-02-18 16:13:43 PST 
Optimize DOM storage event dispatch.
Comment 1 Chris Dumez 2022-02-18 16:18:03 PST 
Created attachment 452601 [details]
Patch
Comment 2 Chris Dumez 2022-02-19 12:34:56 PST 
Created attachment 452650 [details]
Patch
Comment 3 Sihui Liu 2022-02-19 20:19:41 PST 
Comment on attachment 452650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452650&action=review

r=me

> Source/WebCore/storage/StorageEventDispatcher.cpp:55
> +        if (!frame->document() || !frame->window())
>              continue;
> -        if (sourceFrame != frame && frame->document()->securityOrigin().equal(securityOrigin.securityOrigin().ptr()))
> -            frames.append(frame);
> +        if (!frame->window()->hasEventListeners(eventNames().storageEvent))

window() also checks document(). maybe something like:
if (auto* window = frame->window(); !window || !window->hasEventListeners(eventNames().storageEvent)) continue;

> Source/WebCore/storage/StorageEventDispatcher.cpp:77
> +            if (!frame->document() || !frame->window())
> +                continue;
> +            if (!frame->window()->hasEventListeners(eventNames().storageEvent))

Ditto.
Comment 4 Chris Dumez 2022-02-19 20:35:55 PST 
(In reply to Sihui Liu from comment #3)
> Comment on attachment 452650 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452650&action=review
> 
> r=me

No r+? :)

> > Source/WebCore/storage/StorageEventDispatcher.cpp:55
> > +        if (!frame->document() || !frame->window())
> >              continue;
> > -        if (sourceFrame != frame && frame->document()->securityOrigin().equal(securityOrigin.securityOrigin().ptr()))
> > -            frames.append(frame);
> > +        if (!frame->window()->hasEventListeners(eventNames().storageEvent))
> 
> window() also checks document(). maybe something like:
> if (auto* window = frame->window(); !window ||
> !window->hasEventListeners(eventNames().storageEvent)) continue;
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:77
> > +            if (!frame->document() || !frame->window())
> > +                continue;
> > +            if (!frame->window()->hasEventListeners(eventNames().storageEvent))
> 
> Ditto.
Comment 5 Chris Dumez 2022-02-19 20:42:01 PST 
Created attachment 452669 [details]
Patch
Comment 6 Chris Dumez 2022-02-19 20:42:42 PST 
(In reply to Chris Dumez from comment #4)
> (In reply to Sihui Liu from comment #3)
> > Comment on attachment 452650 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=452650&action=review
> > 
> > r=me
> 
> No r+? :)
> 
> > > Source/WebCore/storage/StorageEventDispatcher.cpp:55
> > > +        if (!frame->document() || !frame->window())
> > >              continue;
> > > -        if (sourceFrame != frame && frame->document()->securityOrigin().equal(securityOrigin.securityOrigin().ptr()))
> > > -            frames.append(frame);
> > > +        if (!frame->window()->hasEventListeners(eventNames().storageEvent))
> > 
> > window() also checks document(). maybe something like:
> > if (auto* window = frame->window(); !window ||
> > !window->hasEventListeners(eventNames().storageEvent)) continue;
> > 
> > > Source/WebCore/storage/StorageEventDispatcher.cpp:77
> > > +            if (!frame->document() || !frame->window())
> > > +                continue;
> > > +            if (!frame->window()->hasEventListeners(eventNames().storageEvent))
> > 
> > Ditto.

Thanks for reviewing, I made the suggested changes.
Comment 7 Sihui Liu 2022-02-19 20:51:38 PST 
(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Sihui Liu from comment #3)
> > > Comment on attachment 452650 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=452650&action=review
> > > 
> > > r=me
> > 
> > No r+? :)

r+

> > 
> > > > Source/WebCore/storage/StorageEventDispatcher.cpp:55
> > > > +        if (!frame->document() || !frame->window())
> > > >              continue;
> > > > -        if (sourceFrame != frame && frame->document()->securityOrigin().equal(securityOrigin.securityOrigin().ptr()))
> > > > -            frames.append(frame);
> > > > +        if (!frame->window()->hasEventListeners(eventNames().storageEvent))
> > > 
> > > window() also checks document(). maybe something like:
> > > if (auto* window = frame->window(); !window ||
> > > !window->hasEventListeners(eventNames().storageEvent)) continue;
> > > 
> > > > Source/WebCore/storage/StorageEventDispatcher.cpp:77
> > > > +            if (!frame->document() || !frame->window())
> > > > +                continue;
> > > > +            if (!frame->window()->hasEventListeners(eventNames().storageEvent))
> > > 
> > > Ditto.
> 
> Thanks for reviewing, I made the suggested changes.
Comment 8 EWS 2022-02-19 23:13:40 PST 
Committed r290223 (247549@main): <https://commits.webkit.org/247549@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452669 [details].
Comment 9 Radar WebKit Bug Importer 2022-02-19 23:14:17 PST 
<rdar://problem/89197481>