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
Product: WebKit
Classification: Unclassified
Component: WebKit2
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexey Proskuryakov
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-28 16:25 PDT by Alexey Proskuryakov
Modified: 2010-10-29 13:28 PDT (History)
7 users (show)

See Also:


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

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

Double change log.
Comment 8 Early Warning System Bot 2010-10-28 17:41:53 PDT
Attachment 72259 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4826065
Comment 9 Early Warning System Bot 2010-10-28 18:11:49 PDT
Attachment 72267 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4857057
Comment 10 Eric Seidel 2010-10-28 20:00:36 PDT
Attachment 72267 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4866022
Comment 11 WebKit Review Bot 2010-10-29 06:50:38 PDT
Attachment 72267 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4789077
Comment 12 Alexey Proskuryakov 2010-10-29 10:28:32 PDT
Committed (with build fixes and comments addressed) in <http://trac.webkit.org/changeset/70894>.
Comment 13 Nikolas Zimmermann 2010-10-29 13:23:16 PDT
(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 Nikolas Zimmermann 2010-10-29 13:26:47 PDT
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.
Comment 15 Nikolas Zimmermann 2010-10-29 13:28:29 PDT
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...