Bug 75592

Created attachment 121198 [details] Patch

CC'ing kenneth and joone.

View in context: https://bugs.webkit.org/attachment.cgi?id=121198&action=review > Source/WebKit/efl/ChangeLog:12 > + * CMakeListsEfl.txt: Could you add more description about changes that you have made in your patch here? > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:60 > + int desktopWidth = 980; What this magic number means? Why it is 980 or not 982? Missing comment. > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:125 > + You may use new operator insted of calloc. Please read below url. http://trac.webkit.org/wiki/EFLWebKitCodingStyle > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:27 > +#endif I wonder if there is a appropriate to use extern "C" in all private header now since all files are c++ files that are compiled by c++ compiler... > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:36 > + Where is function that delete Ewk_Viewport_Private_Data* object? Or where at least viewportAttributes form _Ewk_View_Private_Data is deleted? It looks like mem leak.

Created attachment 121236 [details] Patch

(In reply to comment #3) > View in context: https://bugs.webkit.org/attachment.cgi?id=121198&action=review > > > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:60 > > + int desktopWidth = 980; AFAIK, 980 works well for most web pages for desktop browsers. Other ports(gtk, qt and so on) are using this value as well. > > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:27 > > +#endif > > I wonder if there is a appropriate to use extern "C" in all private header now since all files are c++ files that are compiled by c++ compiler... You right. this header file doesn't need to use extern "C". > > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:36 > > + > > Where is function that delete Ewk_Viewport_Private_Data* object? Or where at least viewportAttributes form _Ewk_View_Private_Data is deleted? It looks like mem leak. I missed to delete private data from ewk view's private data. I fix it. Thank you for your review. Previous patch was incomplete. I update this patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=121236&action=review > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:120 > + if (!ewkView) or EINA_SAFETY_ON_NULL_RETURN could be used > Source/WebKit/efl/ewk/ewk_viewport_attributes.h:36 > + Is it possible to move this declaration to ewk_viewport_attributes_private.h? > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:28 > + Is extern "c" is needed here?

Comment on attachment 121236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121236&action=review > Source/WebKit/efl/ewk/ewk_private.h:38 > +#include "ewk_viewport_attributes_private.h" IMO, we can move this into cpp files which use this header. > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:121 > + if (!ewkView) > + return 0; how do you think about ASSERT(ewkView); > Source/WebKit/efl/ewk/ewk_viewport_attributes.h:35 > +typedef struct _Ewk_Viewport_Attributes Ewk_Viewport_Attributes; Should we open _Ewk_Viewport_Attributes ? And I think that extern "C" is needed. > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:27 > +#ifdef __cplusplus > +extern "C" { > +#endif extern "C" is not required.

Created attachment 121249 [details] Patch

Created attachment 121250 [details] Patch

(In reply to comment #7) > (From update of attachment 121236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121236&action=review > > > Source/WebKit/efl/ewk/ewk_private.h:38 > > +#include "ewk_viewport_attributes_private.h" > > IMO, we can move this into cpp files which use this header. Yes, that file should be included by cpp file directly. > > > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:121 > > + if (!ewkView) > > + return 0; > > how do you think about ASSERT(ewkView); I also think ASSERT is better. > > Source/WebKit/efl/ewk/ewk_viewport_attributes.h:35 > > +typedef struct _Ewk_Viewport_Attributes Ewk_Viewport_Attributes; > > Should we open _Ewk_Viewport_Attributes ? > > And I think that extern "C" is needed. > > > Source/WebKit/efl/ewk/ewk_viewport_attributes_private.h:27 > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > extern "C" is not required. This is my mistake. Thank you for your review.

(In reply to comment #10) > (In reply to comment #7) > > > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:121 > > > + if (!ewkView) > > > + return 0; > > > > how do you think about ASSERT(ewkView); > > I also think ASSERT is better. I think current implementation is quite good. I don't think if this is a good solution. Assert should be never used in that context. Moreover, it only work in debug code.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #7) > > > > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:121 > > > > + if (!ewkView) > > > > + return 0; > > > > > > how do you think about ASSERT(ewkView); > > > > I also think ASSERT is better. > I think current implementation is quite good. I don't think if this is a good solution. Assert should be never used in that context. Moreover, it only work in debug code. It seems to me that Ryuan think ewkView is almost exist. Because, that function is invoked by ChromeClientEfl.cpp. But, IMO, if other functions call this function, we're able to need to check if ewkView is null. Ryuan, how do you think ?

Comment on attachment 121250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121250&action=review This new architecture is weird and does not seem to simplify things very much. We have moved from a situation in which ChromeClientEfl called code in ewk_view to one in which ChromClientEfl uses a view to get private viewport data that is then passed to another viewport function, and all these parts have pointers to the view. Plus, it looks like the user cannot retrieve the viewport data anymore. The situation at stake here is quite simple: ChromeClientEfl needs to notify that the viewport's properties have changed, and ewk needs to provide access to the viewport's properties (ie. convert the WebCore data types to user-visible data types). This all sounds like view-specific stuff to me. I agree that refactoring and breaking up ewk_view and ewk_frame is good (they are huge), but this specific refactoring does not help much. > Source/WebKit/efl/ewk/ewk_view.cpp:-3553 > -void ewk_view_viewport_attributes_get(const Evas_Object* ewkView, int* width, int* height, float* initScale, float* maxScale, float* minScale, float* devicePixelRatio, Eina_Bool* userScalable) How does a user get this information now? > Source/WebKit/efl/ewk/ewk_view.h:-83 > - * - "viewport,changed", void: reports that viewport was changed. The signal is still emitted by the view. > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:24 > +#include "ChromeClientEfl.h" ewk shouldn't access WebCoreSupport. > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:128 > + if (!priv) { > + CRITICAL("could not allocate Ewk_Viewport_Private_Data"); > + return 0; > + } new never returns 0. > Source/WebKit/efl/ewk/ewk_viewport_attributes.h:24 > +#include <Eina.h> > +#include <Evas.h> Not needed.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #7) > > > > > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:121 > > > > > + if (!ewkView) > > > > > + return 0; > > > > > > > > how do you think about ASSERT(ewkView); > > > > > > I also think ASSERT is better. > > I think current implementation is quite good. I don't think if this is a good solution. Assert should be never used in that context. Moreover, it only work in debug code. > > It seems to me that Ryuan think ewkView is almost exist. Because, that function is invoked by ChromeClientEfl.cpp. But, IMO, if other functions call this function, we're able to need to check if ewkView is null. Ryuan, how do you think ? IMO, ewkView should not be null, whoever call this internal function. It make sense for me to be crashed if someone call this as wrong way. As similar reason, ChromeClientEfl should be created with non-null ewkView.

Created attachment 121393 [details] Patch

Created attachment 121395 [details] Patch

(In reply to comment #13) > (From update of attachment 121250 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121250&action=review > > This new architecture is weird and does not seem to simplify things very much. We have moved from a situation in which ChromeClientEfl called code in ewk_view to one in which ChromClientEfl uses a view to get private viewport data that is then passed to another viewport function, and all these parts have pointers to the view. Plus, it looks like the user cannot retrieve the viewport data anymore. User can get viewport meta tag by signal callback. evas_object_smart_callback_call(priv->ewkView, "viewport,changed", &attributes); In addition, I will add _get function as well. But, in this patch, I would like to concentrate on refactoring. In addition, GTK port has similar structure. GTK port also has webkitviewportattributesXXX files. It has a view reference. http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp#L814 http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitviewportattributesprivate.h#L31 The viewport meta tag is *very* important for mobile domain. Although EFL port has simple implementation now, more features will be added because viewport meta tag specification is being improved further. After this patch is landed, I or my co-workers will improve viewport functions. So, I decided to move viewport functions to new files. > I agree that refactoring and breaking up ewk_view and ewk_frame is good (they are huge), but this specific refactoring does not help much. > Yes, ewk_view, ewk_frame is too huge. So, this patch will be able to become an example, how to break up ewk_view, ewk_frame. Ryuan, Why don't we file a bug for your suggestion? I'd like to concentrate on viewport meta tag in this bug.

(In reply to comment #17) > (In reply to comment #13) > > This new architecture is weird and does not seem to simplify things very much. We have moved from a situation in which ChromeClientEfl called code in ewk_view to one in which ChromClientEfl uses a view to get private viewport data that is then passed to another viewport function, and all these parts have pointers to the view. Plus, it looks like the user cannot retrieve the viewport data anymore. > > User can get viewport meta tag by signal callback. > evas_object_smart_callback_call(priv->ewkView, "viewport,changed", &attributes); Yet there is no way for a user to query this `attributes' object, as its definition is private and there are no functions to retrieve its data anymore. > In addition, I will add _get function as well. But, in this patch, I would like to concentrate on refactoring. In this case, the refactoring work is also removing functionality without adding a replacement anywhere. This is a no-go. > In addition, GTK port has similar structure. GTK port also has webkitviewportattributesXXX files. It has a view reference. > http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp#L814 > http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitviewportattributesprivate.h#L31 Still, the way the current patch tries to replicate the structure used by the GTK+ port is cumbersome: the naming scheme and the data types do not match (eg. Ewk_Viewport_Private_Data and ewk_viewport_attributes_new) and there are many more private functions and data types than needed. Assuming it makes sense to split this stuff out of ewk_view, right now one only needs the following: - ewk_view_viewport_attributes_get (or ewk_view_viewport_get etc), similar to GTK's webkit_web_view_get_viewport_attributes, which is public and returns a viewport object. - getters for the information stored in a viewport object. - A way for the ChromeClient to recalculate the viewport data and emitting the appropriate signal. Getting rid of _Ewk_Viewport_Private_Data, making the compute function also emit the "viewport,changed" signal and keeping all ewk_view functions in ewk_view.{cpp,h}, for example, would reduce the size of the patch and make things easier to read and accept. > The viewport meta tag is *very* important for mobile domain. Although EFL port has simple implementation now, more features will be added because viewport meta tag specification is being improved further. After this patch is landed, I or my co-workers will improve viewport functions. So, I decided to move viewport functions to new files. The importance of the feature is orthogonal to the discussion -- newer features and refactoring are indeed welcome. But this does not mean the refactoring in this patch does not need to be improved before being landed. As it is, it needlessly complicates things and will make maintenance needlessly harder. It would be nice to hear what the plans for new viewport features are, so we can also see if they contribute to the case of moving this code out of ewk_view.

Comment on attachment 121395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121395&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:133 > + Ewk_Viewport_Private_Data* viewportPrivateData; You could use an OwnPtr here to avoid having to manage the memory manually. > Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:56 > + WebCore::ViewportArguments arguments; It may not be necessary to keep this object around if you recompute everything anyway.

This patch is not needed anymore.