Bug 48576 – Let WebKit2 client know when a frame is a frameset

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Clients want to know that to present different UI on pages with frames.

[Alexey Proskuryakov]

Created attachment 72259 [details]
proposed patch

[Darin Adler]

Comment on attachment 72259 [details]
proposed patch

It looks to me like this will send the message more times than it needs to. I can browse pages without any framesets all day long and it seems the message will be sent often during that time.

[Alexey Proskuryakov]

I can easily add a boolean for the last sent value, but it didn't seem important to me. Should I?

[Darin Adler]

(In reply to comment #3)
> I can easily add a boolean for the last sent value, but it didn't seem important to me. Should I?

I’d ask the WebKit2 architects that question. I’m not sure how important it is to avoid sending unneeded messages between processes, but my assumption is that doing so is worthwhile.

[Alexey Proskuryakov]

Sam says it's fine, but he also says that I should be sending a page message, not a process one.

[Alexey Proskuryakov]

Created attachment 72267 [details]
updated patch

[Darin Adler]

Comment on attachment 72267 [details]
updated patch

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

> WebCore/loader/FrameLoaderClient.h:227
> +        virtual void dispatchDidBecomeFrameset(bool) = 0; // Can change back due to navigation or DOM modification.

Not sure the word “back” here is helpful.

> WebKit2/ChangeLog:31
> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).

Double change log.

[Early Warning System Bot]

Attachment 72259 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4826065

[Early Warning System Bot]

Attachment 72267 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4857057

[Eric Seidel (no email)]

Attachment 72267 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4866022

[WebKit Review Bot]

Attachment 72267 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4789077

[Alexey Proskuryakov]

Committed (with build fixes and comments addressed) in <http://trac.webkit.org/changeset/70894>.

[Nikolas Zimmermann]

(In reply to comment #12)
> Committed (with build fixes and comments addressed) in <http://trac.webkit.org/changeset/70894>.

This patch broke lots of tests on the bots. Alexey are you already investigating?

[Nikolas Zimmermann]

Comment on attachment 72267 [details]
updated patch

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

> WebCore/html/HTMLFrameSetElement.cpp:211
> +    if (Frame* frame = document()->frame())

Hm, a call to HTMLElement::insertedIntoDocument is missing here.

> WebCore/html/HTMLFrameSetElement.cpp:217
> +    if (Frame* frame = document()->frame())

And here a call to HTMLElement::removedFromDocument is missing.

[Nikolas Zimmermann]

Sorry, for the noise. Just read about commit r70907, which fixed the world again. Sorry, just spotted breakage on build.webkit.org/waterfall and had a look at this patch...