Bug 90063 - [EFL] Move BatteryClientEfl from WebKit to WebCore
Summary: [EFL] Move BatteryClientEfl from WebKit to WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 90069
  Show dependency treegraph
 
Reported: 2012-06-27 04:50 PDT by Chris Dumez
Modified: 2012-07-03 22:50 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.28 KB, patch)
2012-06-27 05:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.26 KB, patch)
2012-06-27 06:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-06-27 04:50:59 PDT
We should move the BatteryClientEfl class from WebKit/efl/WebCoreSupport to WebCore/platform/efl to that it can be reused for WebKit2.
This will avoid code duplication.
Comment 1 Chris Dumez 2012-06-27 05:38:11 PDT
Created attachment 149735 [details]
Patch
Comment 2 Chris Dumez 2012-06-27 06:01:46 PDT
Created attachment 149736 [details]
Patch
Comment 3 Gyuyoung Kim 2012-06-27 17:59:43 PDT
If there is no interaction with user(or application), I think we can move this from WebKit layer to WebCore. But, if there is need to interaction with user, we can't move it.

CC'ing Kihong. kihong, how do you think about this ?
Comment 4 Kihong Kwon 2012-06-27 19:30:06 PDT
I am not sure about this.
Although we don't need interaction with user(browser or email application) for this because we used edbus library, I don't know how other ports implement this.

IMHO, if we want to apply this, we need to arrange implementation with other ports.
Some ports are already written batteryClientXXX in the WebKit, and I am not sure about their needs exactly.
Comment 5 Gyuyoung Kim 2012-06-27 19:36:01 PDT
(In reply to comment #4)
> I am not sure about this.
> Although we don't need interaction with user(browser or email application) for this because we used edbus library, I don't know how other ports implement this.
> 
> IMHO, if we want to apply this, we need to arrange implementation with other ports.
> Some ports are already written batteryClientXXX in the WebKit, and I am not sure about their needs exactly.

If EFL port doesn't interact with user, I think we can move this from WebKit to WebCore layer. Other ports will decide this issue according to its port implementation.
Comment 6 Kihong Kwon 2012-06-27 21:21:15 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > I am not sure about this.
> > Although we don't need interaction with user(browser or email application) for this because we used edbus library, I don't know how other ports implement this.
> > 
> > IMHO, if we want to apply this, we need to arrange implementation with other ports.
> > Some ports are already written batteryClientXXX in the WebKit, and I am not sure about their needs exactly.
> 
> If EFL port doesn't interact with user, I think we can move this from WebKit to WebCore layer. Other ports will decide this issue according to its port implementation.

If we use EFL on the other OS, maybe we need to re-move this under WebKit.
But, if you don't mind about this, I don't have a problem with this.
Comment 7 Kenneth Rohde Christiansen 2012-06-27 22:19:54 PDT
Comment on attachment 149736 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Move BatteryClientEfl class from WebKit to WebCore
> +        so that it can be reused in WebKit2.

The clients are really supposed to be implemented on the API side (thus WebKit) so in a way it is sad to move it to WebCore for sharing and not to a Source/WebKitShared directory or similar. Anyway we did the same for Qt. I am just wondering whether it is the time to discuss this and figuring out the right way of doing this; sharing WebKit API side code across API ports.
Comment 8 Simon Hausmann 2012-06-27 23:06:15 PDT
(In reply to comment #7)
> (From update of attachment 149736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149736&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Move BatteryClientEfl class from WebKit to WebCore
> > +        so that it can be reused in WebKit2.
> 
> The clients are really supposed to be implemented on the API side (thus WebKit) so in a way it is sad to move it to WebCore for sharing and not to a Source/WebKitShared directory or similar. Anyway we did the same for Qt. I am just wondering whether it is the time to discuss this and figuring out the right way of doing this; sharing WebKit API side code across API ports.

I'm not actually sure why APIs like these go through the WebKit layer when it's clearly something that's delivered by the underlying platform.

That said, for WebKit2 there appears often to be a need to delegate things over to the UIProcess to retain the sandbox protection, so for that WebKit seems like a more fitting place.
Comment 9 Kenneth Rohde Christiansen 2012-06-27 23:13:31 PDT
> > The clients are really supposed to be implemented on the API side (thus WebKit) so in a way it is sad to move it to WebCore for sharing and not to a Source/WebKitShared directory or similar. Anyway we did the same for Qt. I am just wondering whether it is the time to discuss this and figuring out the right way of doing this; sharing WebKit API side code across API ports.
> 
> I'm not actually sure why APIs like these go through the WebKit layer when it's clearly something that's delivered by the underlying platform.
> 
> That said, for WebKit2 there appears often to be a need to delegate things over to the UIProcess to retain the sandbox protection, so for that WebKit seems like a more fitting place.


Client are meant to be implemented as part of the API layer (WebKit/). For some ports this might not make sense if they don't allow embedders any API to modify this, but it does make sense in the case of WebKit2.

So basically what we need is a shared (wk1 + wk2) WebCoreSupport. I am OK with moving this to WebCore for now though.
Comment 10 Simon Hausmann 2012-06-28 00:30:31 PDT
(In reply to comment #9)
> So basically what we need is a shared (wk1 + wk2) WebCoreSupport. I am OK with moving this to WebCore for now though.

I really like that.
Comment 11 Chris Dumez 2012-06-28 00:37:52 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > So basically what we need is a shared (wk1 + wk2) WebCoreSupport. I am OK with moving this to WebCore for now though.
> 
> I really like that.

+1
Comment 12 Gyuyoung Kim 2012-07-01 19:20:16 PDT
I agree to land this patch for EFL WK1 and WK2.
Comment 13 Chris Dumez 2012-07-02 04:10:39 PDT
Can we land this patch first? If we decide to do refactoring, we can move it again later. In the meantime, this allows us to reuse the code in WK2.
Comment 14 WebKit Review Bot 2012-07-03 22:50:00 PDT
Comment on attachment 149736 [details]
Patch

Clearing flags on attachment: 149736

Committed r121828: <http://trac.webkit.org/changeset/121828>
Comment 15 WebKit Review Bot 2012-07-03 22:50:06 PDT
All reviewed patches have been landed.  Closing bug.