Bug 234324 – Clean-up: Adopt Page::forEachDocument in some missed spots

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

We missed some spots after Bug 203811 (and Darin's refactoring work in Bug 205567). We should use a consistent idiom for these types of loops.

<rdar://problem/85443831>

[Brent Fulgham]

Created attachment 447172 [details]
Patch

[Brent Fulgham]

Comment on attachment 447172 [details]
Patch

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

> Source/WebCore/page/ios/FrameIOS.mm:641
> +static void forEachDocument(Frame& startFrame, const Function<void(Document&)>& functor)

@Darin: Would it be better to use m_page->forEachDocument instead of this local implementation? I wasn't sure if we would always have a page object at the time these calls were made, and Frame::document() doesn't pass through m_page to do its work, so it wasn't obvious that it was always the same behavior.

[Brent Fulgham]

Created attachment 447176 [details]
Patch

[Brent Fulgham]

Created attachment 447180 [details]
Patch

[Darin Adler]

Comment on attachment 447172 [details]
Patch

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

>> Source/WebCore/page/ios/FrameIOS.mm:641
>> +static void forEachDocument(Frame& startFrame, const Function<void(Document&)>& functor)
> 
> @Darin: Would it be better to use m_page->forEachDocument instead of this local implementation? I wasn't sure if we would always have a page object at the time these calls were made, and Frame::document() doesn't pass through m_page to do its work, so it wasn't obvious that it was always the same behavior.

We should not have two copies of the function.

We can refactor the Page one so you can also call it on a main frame, I suppose. I think what I would do is expose a function:

    Page::forEachDocumentFromMainFrame(Frame& ...)

Keep it in Page rather than moving it here to Frame. And certainly not to FrameIOS. It would be nice some day to not need it at all. Then Page can use forEachDocumentFromMainFrame to implement forEachDocument. Refactor them like that.

While it might seem strange to have the function there, I think it’s a good place for it. If we had the MainFrame class (something I added and then removed), maybe we’d keep it there instead.

I’m pretty sure we don’t really need to dispatch pagehide or pageshow events to frames without a page, so I suspect that eventually we can just delete those functions. It also seems messy to have functions on Frame like dispatchPageHideEventBeforePause and dispatchPageShowEventBeforeResume that are only to be called on main frames. We try to put functions like that on Page instead. So I think in the end we will move them there and then be able to get rid of Page::forEachDocumentFromMainFrame.

[Brent Fulgham]

Created attachment 447275 [details]
Patch

[Brent Fulgham]

This patch attempts to adopt Darin's recommended approach. Let's see what EWS thinks.

[Brent Fulgham]

Created attachment 447278 [details]
Patch

[Darin Adler]

Comment on attachment 447278 [details]
Patch

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

Looks really good.

> Source/WebCore/page/Page.h:895
> +    static void forEachDocumentFromFrame(const Frame&, const Function<void(Document&)>&);

The callers all seem to be passing the main frame, although orientationChanged doesn’t make it explicit. I suggest we say "FromMainFrame" here.

> Source/WebCore/page/Page.h:896
> +    void forEachFrameFromMainFrame(const Function<void(Frame&)>&);

This could just be named forEachFrame without mention of the main frame; I don’t see any main frame here.

[Brent Fulgham]

Created attachment 447285 [details]
Patch for landing

[EWS]

Committed r287110 (?): <https://commits.webkit.org/r287110>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447285 [details].

[Simon Fraser (smfr)]

Comment on attachment 447285 [details]
Patch for landing

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

> Source/WebCore/page/Page.h:896
> +    void forEachFrameFromMainFrame(const Function<void(Frame&)>&);

This adds ambiguity. Why is it different than forEachDocument()? Should it be forEachFrameFromFrame() and not assume the passed Frame is the main frame?

> Source/WebCore/page/ios/FrameIOS.mm:649
> +    Page::forEachDocumentFromMainFrame(*this, [pagehideEvent = eventNames().pagehideEvent, mainDocument = document()](Document& document) {
> +        document.domWindow()->dispatchEvent(PageTransitionEvent::create(pagehideEvent, true), mainDocument);
> +    });

Why doesn't this just use forEachDocument()? Is there some subtlety where mainFrame() is something different than usual?

> Source/WebCore/page/ios/FrameIOS.mm:660
> +    Page::forEachDocumentFromMainFrame(*this, [pageshowEvent = eventNames().pageshowEvent, mainDocument = document()](Document& document) {
> +        document.domWindow()->dispatchEvent(PageTransitionEvent::create(pageshowEvent, true), mainDocument);
> +    });

Same question

[Brent Fulgham]

Comment on attachment 447285 [details]
Patch for landing

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

>> Source/WebCore/page/Page.h:896
>> +    void forEachFrameFromMainFrame(const Function<void(Frame&)>&);
> 
> This adds ambiguity. Why is it different than forEachDocument()? Should it be forEachFrameFromFrame() and not assume the passed Frame is the main frame?

I made the naming match the new static method we added, since it always starts from the main frame, not an arbitrary frame. It might have been good to name the 'forEachDocument' method 'forEachDocumentFromMainFrame'.

>> Source/WebCore/page/ios/FrameIOS.mm:649
>> +    });
> 
> Why doesn't this just use forEachDocument()? Is there some subtlety where mainFrame() is something different than usual?

forEachDocument is a method on the Page class, and 'm_page' may be nullptr. I suppose this page hide event is only likely to be called if it is non-null, but it wasn't clear.

[Darin Adler]

Comment on attachment 447285 [details]
Patch for landing

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

>>> Source/WebCore/page/Page.h:896
>>> +    void forEachFrameFromMainFrame(const Function<void(Frame&)>&);
>> 
>> This adds ambiguity. Why is it different than forEachDocument()? Should it be forEachFrameFromFrame() and not assume the passed Frame is the main frame?
> 
> I made the naming match the new static method we added, since it always starts from the main frame, not an arbitrary frame. It might have been good to name the 'forEachDocument' method 'forEachDocumentFromMainFrame'.

The reason I suggested "FromMainFrame" for the new function is to explain why the caller has to pass a frame.

I agree with Simon: The core functions themselves need not mention the "main frame". They iterate all frames and all documents. The role of the main frame is an unimportant implementation detail that should not be in the function names.

We should immediately rename forEachFrameFromMainFrame to just forEachFrame. The odd function out is forEachDocumentFromMainFrame, and it’s something we should intend to remove (see below).

>>> Source/WebCore/page/ios/FrameIOS.mm:649
>>> +    });
>> 
>> Why doesn't this just use forEachDocument()? Is there some subtlety where mainFrame() is something different than usual?
> 
> forEachDocument is a method on the Page class, and 'm_page' may be nullptr. I suppose this page hide event is only likely to be called if it is non-null, but it wasn't clear.

Our ambition should be to soon change this to use Page::forEachDocument, and delete forEachDocumentFromMainFrame. We are adding this mainly to bridge us for now without making a riskier change at this moment about whether this works when page is null.