Bug 48576 - Let WebKit2 client know when a frame is a frameset
: Let WebKit2 client know when a frame is a frameset
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-10-28 16:25 PST by
Modified: 2010-10-29 13:28 PST (History)


Attachments
proposed patch (24.30 KB, patch)
2010-10-28 16:50 PST, Alexey Proskuryakov
darin: review+
Review Patch | Details | Formatted Diff | Diff
updated patch (25.29 KB, patch)
2010-10-28 17:25 PST, Alexey Proskuryakov
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-10-28 16:25:11 PST
Clients want to know that to present different UI on pages with frames.
------- Comment #1 From 2010-10-28 16:50:37 PST -------
Created an attachment (id=72259) [details]
proposed patch
------- Comment #2 From 2010-10-28 16:54:45 PST -------
(From update of attachment 72259 [details])
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.
------- Comment #3 From 2010-10-28 16:57:47 PST -------
I can easily add a boolean for the last sent value, but it didn't seem important to me. Should I?
------- Comment #4 From 2010-10-28 16:59:45 PST -------
(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.
------- Comment #5 From 2010-10-28 17:08:19 PST -------
Sam says it's fine, but he also says that I should be sending a page message, not a process one.
------- Comment #6 From 2010-10-28 17:25:43 PST -------
Created an attachment (id=72267) [details]
updated patch
------- Comment #7 From 2010-10-28 17:29:55 PST -------
(From update of attachment 72267 [details])
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
> +2010-10-28  Alexey Proskuryakov  <ap@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).

Double change log.
------- Comment #8 From 2010-10-28 17:41:53 PST -------
Attachment 72259 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4826065
------- Comment #9 From 2010-10-28 18:11:49 PST -------
Attachment 72267 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4857057
------- Comment #10 From 2010-10-28 20:00:36 PST -------
Attachment 72267 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4866022
------- Comment #11 From 2010-10-29 06:50:38 PST -------
Attachment 72267 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4789077
------- Comment #12 From 2010-10-29 10:28:32 PST -------
Committed (with build fixes and comments addressed) in <http://trac.webkit.org/changeset/70894>.
------- Comment #13 From 2010-10-29 13:23:16 PST -------
(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?
------- Comment #14 From 2010-10-29 13:26:47 PST -------
(From update of attachment 72267 [details])
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.
------- Comment #15 From 2010-10-29 13:28:29 PST -------
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...