Bug 46165 - Focused frame information needed in Chrome and ChromeClient
Summary: Focused frame information needed in Chrome and ChromeClient
Status: RESOLVED DUPLICATE of bug 48685
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 23:22 PDT by Juha Savolainen
Modified: 2010-12-23 10:18 PST (History)
8 users (show)

See Also:


Attachments
Added a new method to Chrome and ChromeClient which is called when focused frame has changed (2.70 KB, patch)
2010-09-20 23:33 PDT, Juha Savolainen
sam: review-
Details | Formatted Diff | Diff
Add focusedFrameChanged-method to ChromeClients (20.17 KB, patch)
2010-09-23 02:58 PDT, Juha Savolainen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juha Savolainen 2010-09-20 23:22:07 PDT
Chrome and ChromeClient should support method which is called when focused frame has changed.
In WebKit2 we need this information when accessing current frame. With this change we can avoid
synchronized call from UIProcess to WebProcess when using frames.
Comment 1 Juha Savolainen 2010-09-20 23:33:57 PDT
Created attachment 68192 [details]
Added a new method to Chrome and ChromeClient which is called when focused frame has changed
Comment 2 Antonio Gomes 2010-09-21 06:29:59 PDT
I think the patch is straightforward and reasonable. Question:

1) Do we need this for WebKit1 too?
2) Should the various ChromeClient in WebKit/ get updated?
3) Should we pass the Focused frame as parameter?
4) Can we test that somehow? even if it is an autotest ... (of course layout test preferred)
Comment 3 Sam Weinig 2010-09-21 07:21:11 PDT
Comment on attachment 68192 [details]
Added a new method to Chrome and ChromeClient which is called when focused frame has changed

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

> WebCore/page/ChromeClient.h:90
> +        virtual void focusedFrameChanged() {};

I would prefer if this was pure-virtual like most of the others in this file.


Can you also explain the use case a little more about the use case for having this information in the UIProcess. Why is it necessary to know which frame has focus?
Comment 4 Juha Savolainen 2010-09-21 23:06:14 PDT
(In reply to comment #2)
> I think the patch is straightforward and reasonable. Question:
> 
> 1) Do we need this for WebKit1 too?

Well, I think no. This notification was added because of WebKit2 architecture(UIProcess/WebProcess). In WebKit1 it is quite simply to ask focused frame from WebCore and return that to the user immeadiately. Ofcourse this could be also used in WebKit1 but I think  that there isn't such benefit than in WebKit2.

> 2) Should the various ChromeClient in WebKit/ get updated?

My idea is that this only affect to WebKit2-side.

> 3) Should we pass the Focused frame as parameter?

Maybe not. We only need to resolve WebKit2::WebFrame.frameID which is focused. Also a little bit later I will introduce WebPage::currentFrameID() method in WebKit2 which returns ID of focused WebFrame. Because this method dosen't take parameter WebKit::Frame* this could be used allways to resolve focused framedID, even if we don't know which WebKit::Frame is currently activated. In our case, when focused frame has changed and FocusController calls WebChromeClient::focusedFrameChanged() we just call currentFrameID() method and send frameID to UIProcess.

> 4) Can we test that somehow? even if it is an autotest ... (of course layout test preferred)

Cannot answer right now, I need investigate testing a little bit more how this kind of functions is tested earlyer.
Comment 5 Juha Savolainen 2010-09-21 23:41:47 PDT
(In reply to comment #3)
> (From update of attachment 68192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68192&action=review
> 
> > WebCore/page/ChromeClient.h:90
> > +        virtual void focusedFrameChanged() {};
> 
> I would prefer if this was pure-virtual like most of the others in this file.

Pure virtual means that it must be implemented in derived class and I thought that we need this method only in WebKit2-side. If this is changed to pure virtual method then it must be added to all ChromeClients. And implementation would be empty.

> Can you also explain the use case a little more about the use case for having this information in the UIProcess. Why is it necessary to know which frame has focus?

Yes, sure.
Main point is that this is needed because I have done QWKFrame API(initial version, just 5 method) to WebKit2 which is similar than QWebFrame API. The rest patches will come later.

I will add a new class, QWKFrame which have eg. frameName-method which returns frame name. 
The use case: user wants to know name of the current frame(=focused frame), currently WebKit2 C-API dosen't support any other than mainframe. If we don't do this notification, then we must do synchronized call from UIProcess to WebProcess everytime when someone wants access to frames. I think we should send frameID to UIProcess when it's changed so it will be there allready.
Comment 6 Sam Weinig 2010-09-22 06:25:24 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 68192 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68192&action=review
> > 
> > > WebCore/page/ChromeClient.h:90
> > > +        virtual void focusedFrameChanged() {};
> > 
> > I would prefer if this was pure-virtual like most of the others in this file.
> 
> Pure virtual means that it must be implemented in derived class and I thought that we need this method only in WebKit2-side. If this is changed to pure virtual method then it must be added to all ChromeClients. And implementation would be empty.

Yes. That is the normal way we do things.

> 
> > Can you also explain the use case a little more about the use case for having this information in the UIProcess. Why is it necessary to know which frame has focus?
> 
> Yes, sure.
> Main point is that this is needed because I have done QWKFrame API(initial version, just 5 method) to WebKit2 which is similar than QWebFrame API. The rest patches will come later.
> 
> I will add a new class, QWKFrame which have eg. frameName-method which returns frame name. 
> The use case: user wants to know name of the current frame(=focused frame), currently WebKit2 C-API dosen't support any other than mainframe. If we don't do this notification, then we must do synchronized call from UIProcess to WebProcess everytime when someone wants access to frames. I think we should send frameID to UIProcess when it's changed so it will be there allready.

I am still not clear on why the user needs to know the name of the focused frame?  What can they do with that information?
Comment 7 Juha Savolainen 2010-09-22 08:36:38 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 68192 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=68192&action=review
> > > 
> > > > WebCore/page/ChromeClient.h:90
> > > > +        virtual void focusedFrameChanged() {};
> > > 
> > > I would prefer if this was pure-virtual like most of the others in this file.
> > 
> > Pure virtual means that it must be implemented in derived class and I thought that we need this method only in WebKit2-side. If this is changed to pure virtual method then it must be added to all ChromeClients. And implementation would be empty.
> 
> Yes. That is the normal way we do things.

Ok, if that is rule then it should fix to pure virtual altought I have a little bit different opinion.

> I am still not clear on why the user needs to know the name of the focused frame?  What can they do with that information?

Sorry, my mistake. I meaned focused frame url.
Comment 8 Juha Savolainen 2010-09-23 02:58:02 PDT
Created attachment 68506 [details]
Add focusedFrameChanged-method to ChromeClients
Comment 9 Antonio Gomes 2010-09-23 08:06:56 PDT
Comment on attachment 68506 [details]
Add focusedFrameChanged-method to ChromeClients

It looks sane to me. Sam?
Comment 10 Sam Weinig 2010-09-23 23:56:07 PDT
> 
> > I am still not clear on why the user needs to know the name of the focused frame?  What can they do with that information?
> 
> Sorry, my mistake. I meaned focused frame url.

I am still not sure what the use case is.
Comment 11 Juha Savolainen 2010-09-24 00:33:19 PDT
(In reply to comment #10)
> > 
> > > I am still not clear on why the user needs to know the name of the focused frame?  What can they do with that information?
> > 
> > Sorry, my mistake. I meaned focused frame url.
> 
> I am still not sure what the use case is.

Html-page contains 3 frames(main frame + 2 other) and the user wants to know 
url of any other frame than a mainframe, it's impossible. 
WebKit1: QWebPage->currentFrame()->url()

Html-page contains 3 frames(main frame + 2 other) and the user just wants to
access the current frame, it's impossible. 
WebKit1: QWebPage->currentFrame();

In WebKit2, there is only possibility to operate with the mainframe, 
not focused or any other than mainframe.
Comment 12 Sam Weinig 2010-09-24 07:25:48 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > > 
> > > > I am still not clear on why the user needs to know the name of the focused frame?  What can they do with that information?
> > > 
> > > Sorry, my mistake. I meaned focused frame url.
> > 
> > I am still not sure what the use case is.
> 
> Html-page contains 3 frames(main frame + 2 other) and the user wants to know 
> url of any other frame than a mainframe, it's impossible. 
> WebKit1: QWebPage->currentFrame()->url()
> 
> Html-page contains 3 frames(main frame + 2 other) and the user just wants to
> access the current frame, it's impossible. 
> WebKit1: QWebPage->currentFrame();
> 
> In WebKit2, there is only possibility to operate with the mainframe, 
> not focused or any other than mainframe.

I understand how it can be used, but *why* does a user need to know which frame is focused from the UIProcess.  The model we have been trying to work with is to have almost everything be based on the Page, and have frames act almost as handles to the load state.
Comment 13 Juha Savolainen 2010-09-27 22:48:54 PDT
(In reply to comment #12)

> I understand how it can be used, but *why* does a user need to know which frame is focused from the UIProcess.  The model we have been trying to work with is to have almost everything be based on the Page, and have frames act almost as handles to the load state.

Thank you for comments. This would related to new API for frames, could you say your opinion about that API?
Comment 14 Kenneth Rohde Christiansen 2010-09-28 04:53:42 PDT
Hi Simon, do you know the reason why we originally added the currentFrame method? That seems to be before I joined the project.
Comment 15 Alexey Proskuryakov 2010-12-23 10:18:03 PST
This never had a use case mentioned, but we ended up needing to know the focused frame in Safari.

*** This bug has been marked as a duplicate of bug 48685 ***