Attached patch adds EFL-specific code to WebCore/platform/Widget.h.
Created attachment 51069 [details] Add EFL-specific code to Widget.h
Attachment 51069 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/Widget.h:84: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 51069 [details] Add EFL-specific code to Widget.h > +#if PLATFORM(EFL) > + virtual void frameRectsChanged(); > +#else > virtual void frameRectsChanged() {} > +#endif What was this one introduced for? Maybe that should be part of the patch that introduced it?
Comment on attachment 51069 [details] Add EFL-specific code to Widget.h Insufficient explaination in the ChagneLog for me to make sense of this patch. See http://webkit.org/coding/contributing.html
(In reply to comment #3) > (From update of attachment 51069 [details]) > > > +#if PLATFORM(EFL) > > + virtual void frameRectsChanged(); > > +#else > > virtual void frameRectsChanged() {} > > +#endif > > What was this one introduced for? Maybe that should be part of the patch that > introduced it? It is used to move our scrollbars and other widgets like plugins. Our platform is bit different and requires this.
(In reply to comment #5) > > What was this one introduced for? Maybe that should be part of the patch that > > introduced it? > > It is used to move our scrollbars and other widgets like plugins. Our platform > is bit different and requires this. I think this should go in a port-specific file rather than on the cross-platform one. I would mimic what is done in, say, ScrollbarGtk.h. These functions are virtual exactly so they can be overriden.
(In reply to comment #6) > (In reply to comment #5) > > > What was this one introduced for? Maybe that should be part of the patch that > > > introduced it? > > > > It is used to move our scrollbars and other widgets like plugins. Our platform > > is bit different and requires this. > > I think this should go in a port-specific file rather than on the > cross-platform one. I would mimic what is done in, say, ScrollbarGtk.h. These > functions are virtual exactly so they can be overriden. We need this at the widget level, we must be informed about the frameRect changed so we can act. Unlike other ports, our plugins and other visual elements (scrollbars) are not moved automatically. We also need to update our clippers if we use backing store that is bigger than the current viewport. Also, note that this patch just add an exception for EFL, it does not change the behavior of old platforms. We just need to be able to implement our own frameRectChanged() in WidgetEfl.cpp, see http://trac.webkit.org/browser/trunk/WebCore/platform/efl/WidgetEfl.cpp#L183 If we don't have this, then moving our object into the window will leave stuff behind :-/ Note that EFL is just managing objects into a scene, it is windowless, there is no X11 windows being moved that move their children automatically.
(In reply to comment #7) > We need this at the widget level, we must be informed about the frameRect > changed so we can act. Unlike other ports, our plugins and other visual > elements (scrollbars) are not moved automatically. We also need to update our > clippers if we use backing store that is bigger than the current viewport. Sorry, I may not have been clear enough. I am not suggesting you move it outside of the Widget level. I'm just suggesting you use a port specific WidgetEfl.h, like other classes do. I haven't looked at how difficult that would be, but I don't think it should be too difficult.
(In reply to comment #8) > (In reply to comment #7) > > We need this at the widget level, we must be informed about the frameRect > > changed so we can act. Unlike other ports, our plugins and other visual > > elements (scrollbars) are not moved automatically. We also need to update our > > clippers if we use backing store that is bigger than the current viewport. > > Sorry, I may not have been clear enough. I am not suggesting you move it > outside of the Widget level. I'm just suggesting you use a port specific > WidgetEfl.h, like other classes do. I haven't looked at how difficult that > would be, but I don't think it should be too difficult. But the patch would be as intrusive, no? What is good in leaving the #ifndef in Widget.h and the #ifdef in the WidgetEfl.h? I guess it is overcomplicating for no greater benefit. MAC already have similar pattern down in the same file.
There would be no #if's in that case. But that was just a suggestion. If you fix the changelog to detail the change as requested by Eric, and perhaps a comment to make it obvious, I have no problem with this patch going in.
Created attachment 53205 [details] Add EFL-specific code to Widget.h, with comments. Add comments as requested by Gustavo Noronha (kov).
Attachment 53205 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #12) > Attachment 53205 [details] did not pass style-queue: > > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 > Total errors found: 0 in 0 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. These errors were caused by EFL's type using the format like "Evas_Object" and "Ecore_Evas".
Comment on attachment 53205 [details] Add EFL-specific code to Widget.h, with comments. r=me
Comment on attachment 53205 [details] Add EFL-specific code to Widget.h, with comments. Rejecting patch 53205 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html
Something is wrong with teh commit-queue. Will ix.
svn-apply seems to think that this patch is invalid: % curl "https://bugs.webkit.org/attachment.cgi?id=53205" --location | svn-apply Failed to find 'Index:' in: ------------------------------------------------------------------- --- WebCore/ChangeLog +++ WebCore/ChangeLog @@ -1,3 +1,16 @@ +2010-03-18 Leandro Pereira <leandro@profusion.mobi> Died at WebKitTools/Scripts/svn-apply line 337, <> line 94. % echo $? 255 I'm not sure why the commit-queue got all the way to the landing stage if svn-apply failed as such.
(In reply to comment #17) > svn-apply seems to think that this patch is invalid: > > > % curl "https://bugs.webkit.org/attachment.cgi?id=53205" --location | svn-apply > Failed to find 'Index:' in: > ------------------------------------------------------------------- > --- WebCore/ChangeLog > +++ WebCore/ChangeLog > @@ -1,3 +1,16 @@ > +2010-03-18 Leandro Pereira <leandro@profusion.mobi> > > Died at WebKitTools/Scripts/svn-apply line 337, <> line 94. > % echo $? > 255 > > I'm not sure why the commit-queue got all the way to the landing stage if > svn-apply failed as such. The submitted patch has no "Index:" line (as the svn-apply error states): --- WebCore/ChangeLog +++ WebCore/ChangeLog @@ -1,3 +1,16 @@ +2010-03-18 Leandro Pereira <leandro@profusion.mobi> + + Reviewed by NOBODY (OOPS!). + + Add EFL-specific code to platform/Widget.h. We need (from https://bugs.webkit.org/attachment.cgi?id=53205 ) FYI, Leandro seems to have had similar problems in the past: https://bugs.webkit.org/show_bug.cgi?id=35234#c2 https://bugs.webkit.org/show_bug.cgi?id=37478#c10 Leandro, are you using svn-create-patch, git diff, or an equivalent? Are you modifying your patches by hand in some way prior to submitting?
(In reply to comment #18) > FYI, Leandro seems to have had similar problems in the past: > > https://bugs.webkit.org/show_bug.cgi?id=35234#c2 > https://bugs.webkit.org/show_bug.cgi?id=37478#c10 > > Leandro, are you using svn-create-patch, git diff, or an equivalent? Are you > modifying your patches by hand in some way prior to submitting? FYI, here is a previous note to Leandro requesting that our tools be used to create patches: https://bugs.webkit.org/show_bug.cgi?id=35749#c2
Created attachment 53328 [details] Add EFL-specific code to Widget.h. The previous patch was made by Gustavo because we're in a hurry -- he didn't know about the WebKit tools. This patch has been already r+'d by Hyatt, it's just updated both to fix the patch format, and to update to a newer Widget.h.
Comment on attachment 53328 [details] Add EFL-specific code to Widget.h. I'm confused. Why wouldn't EFL subclass Widget like how WidgetMac does?
Comment on attachment 53328 [details] Add EFL-specific code to Widget.h. I have no idea why Hyatt r+'d this before. Two reviewers have now suggested using WidgetEFL, and there has been no explanation from teh author why we would not, neither in the bug nor the ChangeLog. Please update the ChangeLog to explain why this change is done this way, or fix ti to use WidgetEFL (assuming that's the modern way to do this type of change?). #ifdefs here seem super-ugly.
(In reply to comment #21) > (From update of attachment 53328 [details]) > I'm confused. Why wouldn't EFL subclass Widget like how WidgetMac does? No other port override this method, not even WidgetMac: leandro:~/git/webkit/WebCore/platform$ grep 'void\s*Widget::frameRectsChanged' * -r efl/WidgetEfl.cpp:void Widget::frameRectsChanged() leandro:~/git/webkit/WebCore/platform$ Widget.h declares an empty method for all ports, but we do need to override this at widget level. I thought that the ChangeLog message was clear enough.
It delcares an empty virtual method, which could be overridden by a virtual method in a Widget subclass, no?
Comment on attachment 53328 [details] Add EFL-specific code to Widget.h. OK. I think this can be OK. We talked about this at length over IRC. I'd like to see a patch posted with either // FIXME: comments or just normal comments expaining why EFL is adding all this junk to Widget. +#if PLATFORM(EFL) + // EFL needs to know whenever any object was moved or resized, so a + // null frameRectsChanged() implementation can't be used. + virtual void frameRectsChanged(); +#else virtual void frameRectsChanged() {} +#endif Should be done with a WidgetEFL.cpp file, like all the other overrides in Widget.cpp are done. +#if PLATFORM(EFL) + Evas* evas() const; + + void setEvasObject(Evas_Object*); + Evas_Object* evasObject() const; + + const String edjeTheme() const; + void setEdjeTheme(const String &); + const String edjeThemeRecursive() const; +#endif Shouldn't these just be free methods in some sort of WidgetEFL.h file? They coudl take the Widget or PlatformWidget as an argument since I"m sure that's what you're using to compute this data. Free methods are very c-like after all. :) I'm not sure what this is: + Ecore_Evas* ecoreEvas() const;
Created attachment 53361 [details] Add EFL-specific code to Widget.h. After some discussion with eseidel on IRC, we agreed to include the patch as-is, but with some comments indicating what should be done to have as few platform-specific stuff as possible inside Widget.h. This revised patch includes these FIXME comments.
Comment on attachment 53361 [details] Add EFL-specific code to Widget.h. EFL port should subclass the Widget class and 205 // override this method. That's wrong. I was originally confused. Widget doesn't get subclassed it appears. But Widget.cpp could add an empty implementation which was wrapped in the #ifdef and WidgetEFL.cpp would have the implementation which was non-empty. I'm actually not sure why we don't do that instead of writing the FIXME as that would actually be fewer lines of code. :)
Created attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp Thanks.
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp Rejecting patch 53369 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53369', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Last 500 characters of output: date_status return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file)) File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run self._check_for_timeout() File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout raise NetworkTimeout() webkitpy.common.net.networktransaction.NetworkTimeout
(In reply to comment #30) > (From update of attachment 53369 [details]) > Rejecting patch 53369 from commit-queue. > > Unexpected failure when landing patch! Please file a bug against webkit-patch. > Failed to run "['WebKitTools/Scripts/webkit-patch', > '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', > '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', > '--build-style=both', '--quiet', '53369', '--parent-command=commit-queue', > '--no-update']" exit_code: 1 > Last 500 characters of output: > date_status > return NetworkTransaction().run(lambda: > self._post_status_to_server(queue_name, status, patch, results_file)) > File > "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", > line 56, in run > self._check_for_timeout() > File > "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", > line 63, in _check_for_timeout > raise NetworkTimeout() > webkitpy.common.net.networktransaction.NetworkTimeout This bug is cursed, it is the only explanation I can see :-D
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp Trying again.
I suspect this is attempting to post a huge log and timing out. I suspect it breaks mac somehow.
(In reply to comment #34) > I suspect this is attempting to post a huge log and timing out. I suspect it > breaks mac somehow. I've built WebCore (from r57639) on my personal Mac today with this patch and it built successfully. Any other clues on what might be happening? FWIW: MacBook 2,1, OSX 10.6.3, Xcode 3.2.1, default x86_64|Release build by running "make" on the top level source directory.
Comment on attachment 53205 [details] Add EFL-specific code to Widget.h, with comments. Cleared David Hyatt's review+ from obsolete attachment 53205 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp Rejecting patch 53369 from commit-queue. jesus@webkit.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your committer rights.
(In reply to comment #38) > (From update of attachment 53369 [details]) > Rejecting patch 53369 from commit-queue. > > Unexpected failure when landing patch! Please file a bug against webkit-patch. > Failed to run "['WebKitTools/Scripts/webkit-patch', > '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', > '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', > '--build-style=both', '--quiet', '53369', '--parent-command=commit-queue', > '--no-update']" exit_code: 1 > Last 500 characters of output: > date_status > return NetworkTransaction().run(lambda: > self._post_status_to_server(queue_name, status, patch, results_file)) > File > "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", > line 56, in run > self._check_for_timeout() > File > "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", > line 63, in _check_for_timeout > raise NetworkTimeout() > webkitpy.common.net.networktransaction.NetworkTimeout Eric, what to do for this? This patch is hanging around for some time and the error is with python scripts to check it. You previously mentioned it may be a problem with some port, producing huge logs, but could you confirm if that is the problem and if so which problem it is so we can fix?
Restarted the cq. Jesus should be able to cq+ things now.
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp I'll look at the cq logs to find out bout the network transaction issue.
Created attachment 53940 [details] The full log from the commit-queue Unfortunately the commit-queue is not currently configured to keep full build logs. I should fix that. But from the log we can see that it is the build which is failing here.
Eric, from your log I see that what failed is: platform/mac/accessibility/aria-liveregion-on-image.html -> failed Looking at the attached patch, every change except by the following are done inside #if PLATFORM(EFL): WebCore/platform/Widget.h: - virtual void frameRectsChanged() {} + virtual void frameRectsChanged(); WebCore/platform/Widget.cpp: +#if !PLATFORM(EFL) +void Widget::frameRectsChanged() +{ +} +#endif Would you or anyone from Mac review how these would fail such accessibility test? I guess this happened due some other patch that was landed at the time. No?
(In reply to comment #43) > Would you or anyone from Mac review how these would fail such accessibility > test? > > I guess this happened due some other patch that was landed at the time. No? It's either flakyness or your reason. I don't see how this could possibly break mac, as WebCore.xcodeproj includes both Widget.cpp and Widget.h, so your patch looks fine to me.
Any updates as to why this patch is failing to be applied?
The very last failure in that log is: Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1 Which means it failed to build on Mac. Our upload size is too large for App Engine, so we failed to upload the log.
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp and yet another change, as requested by Gustavo Barbieri on IRC
Comment on attachment 53369 [details] Add EFL-specific code to Widget.h and Widget.cpp Rejecting patch 53369 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 53369, '--parent-command=commit-queue', '--no-update']" exit_code: 1 Last 500 characters of output: date_status return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file)) File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run self._check_for_timeout() File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout raise NetworkTimeout() webkitpy.common.net.networktransaction.NetworkTimeout
(In reply to comment #48) > (From update of attachment 53369 [details]) > Rejecting patch 53369 from commit-queue. Again :~( Could anyone from Apple offer help to check what fails in Mac build? We checked it here and seems nothing fails, the patch is rather simple.
Ld /Users/eseidel/Projects/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/WebKit normal i386 cd /Users/eseidel/Projects/WebKit/WebKit /Developer/usr/bin/g++-4.2 -arch i386 -dynamiclib -L/Users/eseidel/Projects/WebKit/WebKitBuild/Release -F/Users/eseidel/Projects/WebKit/WebKitBuild/Release -F/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/Quartz.framework/Frameworks -F/System/Library/PrivateFrameworks -filelist /Users/eseidel/Projects/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/i386/WebKit.LinkFileList -Wl,--no-demangle -exported_symbols_list mac/WebKit.exp -install_name /Users/eseidel/Projects/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/WebKit -mmacosx-version-min=10.5 -Wl,-dead_strip -sub_umbrella WebCore -lWebKitSystemInterfaceLeopard -framework Carbon -framework Cocoa -framework JavaScriptCore -licucore -framework OpenGL -framework QuartzCore -framework Security -framework WebCore -Wl,-single_module -compatibility_version 1 -current_version 533.7 -o /Users/eseidel/Projects/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/WebKit Undefined symbols: "__ZN7WebCore6Widget17frameRectsChangedEv", referenced from: __ZTV20NetscapePluginWidget in WebFrameLoaderClient.o ld: symbol(s) not found collect2: ld returned 1 exit status
Created attachment 54622 [details] Add Widget.h and Widget.cpp changes to enable EFL This is a new version that modifies WebCore.base.exp to export the new frameRectsChanged() so Mac builds will not fail anymore.
Comment on attachment 54622 [details] Add Widget.h and Widget.cpp changes to enable EFL Clearing flags on attachment: 54622 Committed r58487: <http://trac.webkit.org/changeset/58487>
All reviewed patches have been landed. Closing bug.