Bug 40876 – [EFL] Implement windowRect() and setWindowRect() in ChromeClientEfl.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

There are no implementation in windowRect() and setWindowRect() of ChromeClientEfl.cpp yet.

[Gyuyoung Kim]

Created attachment 59181 [details]
patch for windowRect/setWindowRect

I try to make a patch for this.

[WebKit Review Bot]

Attachment 59181 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:54:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:92:  Declaration has space between type name and * in Ecore_Evas *ee  [whitespace/declaration] [3]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:92:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:95:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:95:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:102:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:109:  Declaration has space between type name and * in Ecore_Evas *ee  [whitespace/declaration] [3]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:109:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:112:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:112:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 11 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 59183 [details]
patch for windowRect/setWindowRect

I modified this patch to fix style errors.

[WebKit Review Bot]

Attachment 59183 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:53:  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.

[Gyuyoung Kim]

(In reply to comment #4)
> Attachment 59183 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 2 files

The style bot points me out lines below,

53 : + #include <Ecore_Evas.h>
54 : + #include <Ecore_X.h>

I don't know what is wrong. Could you please let me know what is wrong ?

[Gyuyoung Kim]

Created attachment 59240 [details]
patch for windowRect and setWindowRect

I modified this patch.

[Lucas De Marchi]

(In reply to comment #6)
> Created an attachment (id=59240) [details]
> patch for windowRect and setWindowRect
> 
> I modified this patch.

For future reference, you could either run WebKitTools/Scripts/check-webkit-style or use webkit-patch, that will check the style automatically for you before actually sending the patch.

[Rafael Antognolli]

(In reply to comment #6)
> Created an attachment (id=59240) [details]
> patch for windowRect and setWindowRect
> 
> I modified this patch.

Hi Gyuyoung,

Do you have a good reason for using Ecore_X_Window instead of just Ecore_Evas? I think you could just get the window size as ecore_evas_geometry_get(). It would be more portable and give you the correct values anyway.

Of course, if Ecore_X_Window is necessary, you should put some #ifdef's as in http://trac.webkit.org/browser/trunk/WebCore/platform/efl/WidgetEfl.cpp#L209 (and the same for the header Ecore_X.h)

[Gyuyoung Kim]

Created attachment 59323 [details]
patch for windowRect and setWindowRect

Rafael, thank you for your comment. At latst, there are no style error. 

The reason I use Ecore_X_Window is to change window size. When I tested setWindowRect() on WebKitGTK, window size was changed.
So, I need to use Ecore_X_Window in order to change window size. Of course, I also think that windowRect() can use Ecore_Evas.
But, I used same efl object.

If I have any misunderstanding, please let me know.

Thanks.

[Rafael Antognolli]

Well, sorry for keep saying this, but I still highly recommend you to use ecore_evas_geometry_get, ecore_evas_move and ecore_evas_resize. It's almost the same API, so the changes are minimal, but you'll get the portability. And you still will be able to resize the window as requested.

As for the current patch, you are still missing #ifdef HAVE_ECORE_X inside the functions. But you don't need them or the header Ecore_X.h if you change to those ecore_evas_* functions as I told you.

[Gyuyoung Kim]

Created attachment 59332 [details]
patch for windowRect and setWindowRect

Ecore_Evas also supports to change window size. I use Ecore_Evas instead of Ecore_X.

Thanks.

[WebKit Review Bot]

Attachment 59332 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:53:  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.

[Gyuyoung Kim]

(In reply to comment #12)
> Attachment 59332 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:53:  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.

Hi Rafael,

I don't understand why style error happen. According to the url you let me know, they use to include <Ecore_Evas.h> without the macro as below,

43	#include <Ecore.h>
44	#include <Ecore_Evas.h>
45	#include <Edje.h>
46	#include <Evas.h>

Do you know the reason ? Should do I use the macro for this?

[Rafael Antognolli]

(In reply to comment #13)
> I don't understand why style error happen. According to the url you let me know, they use to include <Ecore_Evas.h> without the macro as below,
> 
> 43    #include <Ecore.h>
> 44    #include <Ecore_Evas.h>
> 45    #include <Edje.h>
> 46    #include <Evas.h>
> 
> Do you know the reason ? Should do I use the macro for this?

Yes, you are right, you don't need the macro for this. But the problem now is the alphabetical order (as the Review Bot said). You should include Ecore_Evas.h just before <wtf/text/CString.h>, to keep it correct. Even the last header, Evas.h, is wrong, because it should be before <wtf/text/CString.h> as well.

For more information about this, check http://webkit.org/coding/coding-style.html, session #include Statements.

[Gyuyoung Kim]

Created attachment 59466 [details]
patch for windowRect and setWindowRect

Thank you Rafael,

I fix the style errors with the <Evas.h> include.

[Rafael Antognolli]

(In reply to comment #15)
> Created an attachment (id=59466) [details]
> patch for windowRect and setWindowRect
> 
> Thank you Rafael,
> 
> I fix the style errors with the <Evas.h> include.

Hi Gyuyoung,

The patch seems ok to me now. And thank you for fixing Evas.h too.

[Leandro Pereira]

(In reply to comment #16)
> 
> The patch seems ok to me now. And thank you for fixing Evas.h too.

My only gripe are the ChangeLog comments: they're as generic as it gets and don't state briefly what was performed. Either leave them empty or use something more descriptive (preferred).

[Gyuyoung Kim]

Created attachment 59601 [details]
patch for windowRect and setWindowRect

Thanks, I fix the comment in changelog.

[Lucas De Marchi]

It seems good to me, too.

[Kenneth Rohde Christiansen]

Comment on attachment 59601 [details]
patch for windowRect and setWindowRect

Please read the comment in the Qt implementation. I believe this is supposed to be the device rect, as it is used by the viewport meta tag.

[Rafael Antognolli]

(In reply to comment #20)
> (From update of attachment 59601 [details])
> Please read the comment in the Qt implementation. I believe this is supposed to be the device rect, as it is used by the viewport meta tag.

Hmm... sorry Kenneth but I didn't get it. From what I understood, it's exactly what his patch is doing.

[Kenneth Rohde Christiansen]

Comment on attachment 59601 [details]
patch for windowRect and setWindowRect

Might be, I don't remember well the EFL API. so you are getting the size of the canvas? That seems right then. If not, please tell me.

[Rafael Antognolli]

Yes. Actually, this is the size of the window, which in all current backends also means the size of the canvas.

[WebKit Commit Bot]

Comment on attachment 59601 [details]
patch for windowRect and setWindowRect

Rejecting patch 59601 from commit-queue.

Unexpected failure when processing 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', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 59601, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 60, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 68, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

[Gyuyoung Kim]

Created attachment 60112 [details]
patch for windowRect/setWindowRect

It seems to me that there is no review in changelog. I add reviewer to changelog. 

Should I request review again ?

[WebKit Commit Bot]

Comment on attachment 60112 [details]
patch for windowRect/setWindowRect

Clearing flags on attachment: 60112

Committed r62180: <http://trac.webkit.org/changeset/62180>

[Leandro Pereira]

Comment on attachment 59601 [details]
patch for windowRect and setWindowRect

Newer patch was applied; clearing flags on this one.