Attachment 98662[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Tools/ChangeLog:5: Line contains tab character. [whitespace/tab] [5]
Tools/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:5: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ChangeLog:12: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/efl/ewk/ewk_view.cpp:4520: Missing space before ( in if( [whitespace/parens] [5]
Source/WebKit/efl/ewk/ewk_view.cpp:4522: Missing space before ( in if( [whitespace/parens] [5]
Source/WebKit/efl/ewk/ewk_view.cpp:4525: Missing space before ( in if( [whitespace/parens] [5]
Source/WebKit/efl/ewk/ewk_view.cpp:4527: One line control clauses should not use braces. [whitespace/braces] [4]
Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:606: Should have a space between // and comment [whitespace/comments] [4]
Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:607: Missing space after , [whitespace/comma] [3]
Total errors found: 15 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2)
> Attachment 98662[details] did not pass style-queue:
>
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
>
> Tools/ChangeLog:5: Line contains tab character. [whitespace/tab] [5]
> Tools/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:5: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ChangeLog:12: Line contains tab character. [whitespace/tab] [5]
> Source/WebKit/efl/ewk/ewk_view.cpp:4520: Missing space before ( in if( [whitespace/parens] [5]
> Source/WebKit/efl/ewk/ewk_view.cpp:4522: Missing space before ( in if( [whitespace/parens] [5]
> Source/WebKit/efl/ewk/ewk_view.cpp:4525: Missing space before ( in if( [whitespace/parens] [5]
> Source/WebKit/efl/ewk/ewk_view.cpp:4527: One line control clauses should not use braces. [whitespace/braces] [4]
> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:606: Should have a space between // and comment [whitespace/comments] [4]
> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:607: Missing space after , [whitespace/comma] [3]
> Total errors found: 15 in 6 files
>
>
> If any of these errors are false positives, please file a bug against check-webkit-style.
You SHOULD run your patch with *Tools/Scripts/check-webkit-style* before uploading your patch.
Comment on attachment 98662[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=98662&action=review
Informal review - on my side.
> Source/WebKit/efl/ewk/ewk_view.cpp:61
> +#include "HistoryItem.h"
Move above line to #include "HistoryItem.h" as below,
37 #include "GraphicsContext.h"
38 #include "HistoryItem.h"
39 #include "HTMLElement.h"
> Source/WebKit/efl/ewk/ewk_view.cpp:4512
> +void ewk_view_frame_restore_cachedpage(Evas_Object* o, Evas_Object* frame)
I think ewk_view_cached_page_restore is better.
Great, it's what I wish for a long time.
I commented small things in addition to gyuyoung's comment.
View in context: https://bugs.webkit.org/attachment.cgi?id=98662&action=review> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:607
> + //This method must be called before WebCore::FrameView::layout()
> + ewk_view_frame_restore_cachedpage(m_view,m_frame);
m_view, m_frame);
And is this comment necessary?
> Source/WebKit/efl/ewk/ewk_private.h:181
> +void ewk_view_frame_restore_cachedpage(Evas_Object* o, Evas_Object* frame);
> +
How do you think about moving it below of ewk_view_frame_create ?
> Source/WebKit/efl/ewk/ewk_view.cpp:4517
> + WebCore::Frame* main_frame = ewk_frame_core_get(frame);
I think that sd already know main frame.
> Source/WebKit/efl/ewk/ewk_view.cpp:4521
> + WebCore::HistoryItem* item = main_frame->loader()->history()->currentItem();
> +
> + if(!main_frame)
> + return;
It checked after using main_frame.
> Tools/EWebLauncher/main.c:279
> - ewk_view_fixed_layout_size_set(app->browser, app->viewport.w, app->viewport.h);
> - ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
> if (!ewk_view_zoom_range_set(app->browser, app->viewport.minScale, app->viewport.maxScale))
> info(" Fail to set zoom range. minScale = %f, maxScale = %f\n", app->viewport.minScale, app->viewport.maxScale);
> ewk_view_user_scalable_set(app->browser, app->viewport.userScalable);
> +
> + ewk_view_fixed_layout_size_set(app->browser, app->viewport.w, app->viewport.h);
> + ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
> +
Can I know why you moved?
(In reply to comment #4)
> (From update of attachment 98662[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98662&action=review
>
> Informal review - on my side.
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:61
> > +#include "HistoryItem.h"
>
> Move above line to #include "HistoryItem.h" as below,
>
> 37 #include "GraphicsContext.h"
> 38 #include "HistoryItem.h"
> 39 #include "HTMLElement.h"
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4512
> > +void ewk_view_frame_restore_cachedpage(Evas_Object* o, Evas_Object* frame)
>
> I think ewk_view_cached_page_restore is better.
I agree you comment. it will be changed.
(In reply to comment #6)
> (From update of attachment 98662[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98662&action=review
>
> > Source/WebKit/efl/ewk/ewk_frame.cpp:-1906
> > - return;
>
> Why do you remove this code ?
when user add 4th cached page, first cached page will be destroy.
then frame was released and has null reference assert.
so sd->frame->view() was create always.
test case ::
1. add 3 cached page
2. add one more cached page
3. scroll page
4. assert
Program received signal SIGSEGV, Segmentation fault.
0x00b671ed in WebCore::EventHandler::setFrameWasScrolledByUser (this=0x4f0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2997
2997 FrameView* v = m_frame->view();
(gdb) bt
#0 0x00b671ed in WebCore::EventHandler::setFrameWasScrolledByUser (this=0x4f0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2997
#1 0x00b6714d in WebCore::EventHandler::sendScrollEvent (this=0x4f0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2990
#2 0x00b7af83 in WebCore::FrameView::scrollPositionChanged (this=0x80b98e0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/FrameView.cpp:1468
#3 0x00b7d3ff in WebCore::FrameView::scrollTo (this=0x80b98e0, newOffset=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/FrameView.cpp:2110
#4 0x00bd65a7 in WebCore::ScrollView::setScrollOffset (this=0x80b98e0, offset=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollView.cpp:351
#5 0x00bd2592 in WebCore::ScrollableArea::setScrollOffsetFromAnimation (this=0x80b990c, offset=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollableArea.cpp:133
#6 0x0127e999 in WebCore::ScrollAnimator::notityPositionChanged (this=0x80935e0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollAnimator.cpp:129
#7 0x0127e4a9 in WebCore::ScrollAnimator::scroll (this=0x80935e0, orientation=WebCore::VerticalScrollbar, step=1, multiplier=40)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollAnimator.cpp:70
#8 0x0127e8a4 in WebCore::ScrollAnimator::handleWheelEvent (this=0x80935e0, e=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollAnimator.cpp:110
#9 0x00bd2563 in WebCore::ScrollableArea::handleWheelEvent (this=0x80b990c, wheelEvent=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollableArea.cpp:120
#10 0x00bd8558 in WebCore::ScrollView::wheelEvent (this=0x80b98e0, e=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollView.cpp:800
#11 0x00b6437a in WebCore::EventHandler::handleWheelEvent (this=0x808de18, e=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2138
#12 0x008b0d3a in ewk_frame_feed_mouse_wheel (o=0x80b8fc0, ev=0xbfffdfc0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebKit/efl/ewk/ewk_frame.cpp:1405
#13 0x008c57cf in _ewk_view_smart_mouse_wheel (sd=0x808f338, ev=0xbfffdfc0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebKit/efl/ewk/ewk_view.cpp:376
#14 0x008c5e95 in _ewk_view_on_mouse_wheel (data=0x808f338, e=0x80a2f28, o=0x808f1f8, event_info=0xbfffdfc0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebKit/efl/ewk/ewk_view.cpp:474
#15 0x0021d4a2 in evas_object_event_callback_call (obj=0x808f1f8, type=EVAS_CALLBACK_MOUSE_WHEEL, event_info=0xbfffdfc0)
at evas_callbacks.c:222
#16 0x0021d57f in evas_object_event_callback_call (obj=0x80b8ea0, type=EVAS_CALLBACK_MOUSE_WHEEL, event_info=0xbfffdfc0)
at evas_callbacks.c:251
#17 0x002203d5 in evas_event_feed_mouse_wheel (e=0x80a2f28, direction=0, z=1, timestamp=9067795, data=0x0) at evas_events.c:599
#18 0x035f2bd3 in ecore_event_evas_mouse_wheel (data=0x0, type=17, event=0x8b34c50) at ecore_input_evas.c:310
#19 0x002c12d9 in _ecore_event_call () at ecore_events.c:641
#20 0x002c67f7 in _ecore_main_loop_iterate_internal (once_only=0) at ecore_main.c:1518
#21 0x002c69f7 in ecore_main_loop_begin () at ecore_main.c:663
#22 0x0804c7c7 in main (argc=2, argv=0xbffff254) at /home/dongjae/Projects/WebKit_EFL/Tools/EWebLauncher/main.c:890
(In reply to comment #5)
> Great, it's what I wish for a long time.
> I commented small things in addition to gyuyoung's comment.
>
> View in context: https://bugs.webkit.org/attachment.cgi?id=98662&action=review
>
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:607
> > + //This method must be called before WebCore::FrameView::layout()
> > + ewk_view_frame_restore_cachedpage(m_view,m_frame);
>
> m_view, m_frame);
> And is this comment necessary?
ok, I will delete it.
> > Source/WebKit/efl/ewk/ewk_private.h:181
> > +void ewk_view_frame_restore_cachedpage(Evas_Object* o, Evas_Object* frame);
> > +
>
> How do you think about moving it below of ewk_view_frame_create ?
>
Please tell me why? there are any rules?
> > Source/WebKit/efl/ewk/ewk_view.cpp:4517
> > + WebCore::Frame* main_frame = ewk_frame_core_get(frame);
>
> I think that sd already know main frame.
>
ok I will be change it(using sd->main_frame)
> > Source/WebKit/efl/ewk/ewk_view.cpp:4521
> > + WebCore::HistoryItem* item = main_frame->loader()->history()->currentItem();
> > +
> > + if(!main_frame)
> > + return;
>
> It checked after using main_frame.
>
Ok, I will change it.
> > Tools/EWebLauncher/main.c:279
> > - ewk_view_fixed_layout_size_set(app->browser, app->viewport.w, app->viewport.h);
> > - ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
> > if (!ewk_view_zoom_range_set(app->browser, app->viewport.minScale, app->viewport.maxScale))
> > info(" Fail to set zoom range. minScale = %f, maxScale = %f\n", app->viewport.minScale, app->viewport.maxScale);
> > ewk_view_user_scalable_set(app->browser, app->viewport.userScalable);
> > +
> > + ewk_view_fixed_layout_size_set(app->browser, app->viewport.w, app->viewport.h);
> > + ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
> > +
>
> Can I know why you moved?
when view port changed, zoom level was set. ewk_view_zoom_set check zoom range.
but zoom range value was old one.
so i changed setting sequence.
(In reply to comment #8)
> when user add 4th cached page, first cached page will be destroy.
> then frame was released and has null reference assert.
>
> so sd->frame->view() was create always.
If we don't use cached page or frame is not null, I think we need to check the condition. If frame already has a view instance, we don't need to create new view. Please consider both normal case and your case.
(In reply to comment #9)
> (In reply to comment #5)
> > > Source/WebKit/efl/ewk/ewk_private.h:181
> > > +void ewk_view_frame_restore_cachedpage(Evas_Object* o, Evas_Object* frame);
> > > +
> >
> > How do you think about moving it below of ewk_view_frame_create ?
> >
>
> Please tell me why? there are any rules?
At least, I don't know rules.
But, I think that we'd better to split APIs related ewk_view.cpp and ewk_frame.cpp and so on.
IMO, we can refactor this file more readable.
>> Source/WebKit/efl/ChangeLog:8
>> + add CachedPage's content size update func becuase CachedPage 's content size was delete when it was cached.
>
> Line contains tab character. [whitespace/tab] [5]
The English here is a bit hard do understand. Could you try to rephrase your description?
> Source/WebKit/efl/ewk/ewk_view.cpp:4508
> + * @param[in] o a Evas_Object m_view
There is no need for the '[in'] here.
> Source/WebKit/efl/ewk/ewk_view.cpp:4509
> + * @param[in] frame a Evas_Object m_frame
Ditto.
> Source/WebKit/efl/ewk/ewk_view.cpp:4510
> + * @return void
There is no need for this if the function does not return anything.
It'd also be good to add an "@internal" tag in the apidox as well.
>> Tools/ChangeLog:6
>> + when view port attribute was set, zoom range and user scalable value must setted before fixed size set
>
> Line contains tab character. [whitespace/tab] [5]
The English here could be cleaned up a little too.
(In reply to comment #10)
> (In reply to comment #8)
>
> > when user add 4th cached page, first cached page will be destroy.
> > then frame was released and has null reference assert.
> >
> > so sd->frame->view() was create always.
>
> If we don't use cached page or frame is not null, I think we need to check the condition. If frame already has a view instance, we don't need to create new view. Please consider both normal case and your case.
Currently, EWeblauncher Browser has some crash when use click some link.(Using Current Revision)
please see below call stack.
Program received signal SIGSEGV, Segmentation fault.
0x0090db99 in WebCore::Document::didRemoveWheelEventHandler (this=0x81a5b48)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/dom/Document.cpp:5051
5051 ASSERT(m_wheelEventHandlerCount > 0);
(gdb) bt
#0 0x0090db99 in WebCore::Document::didRemoveWheelEventHandler (this=0x81a5b48)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/dom/Document.cpp:5051
#1 0x00b96c8a in WebCore::FrameView::willRemoveHorizontalScrollbar (this=0x80ba4c8, scrollbar=0x8166f78)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/page/FrameView.cpp:323
#2 0x00bf6bc6 in WebCore::ScrollView::setHasHorizontalScrollbar (this=0x80ba4c8, hasBar=false)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/platform/ScrollView.cpp:100
#3 0x00bf8041 in WebCore::ScrollView::updateScrollbars (this=0x80ba4c8, desiredOffset=...)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/platform/ScrollView.cpp:476
#4 0x00bf6fbd in WebCore::ScrollView::setScrollbarModes (this=0x80ba4c8, horizontalMode=WebCore::ScrollbarAlwaysOff,
verticalMode=WebCore::ScrollbarAlwaysOn, horizontalLock=false, verticalLock=false)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/platform/ScrollView.cpp:162
#5 0x00b9fb37 in WebCore::ScrollView::setHorizontalScrollbarMode (this=0x80ba4c8, mode=WebCore::ScrollbarAlwaysOff, lock=false)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/platform/ScrollView.h:94
#6 0x00b98c42 in WebCore::FrameView::layout (this=0x80ba4c8, allowSubtree=false)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/page/FrameView.cpp:979
#7 0x00b9e3d3 in WebCore::FrameView::forceLayout (this=0x80ba4c8, allowSubtree=false)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebCore/page/FrameView.cpp:2603
#8 0x008d88fc in ewk_view_fixed_layout_size_set (o=0x808e6d8, w=980, h=729)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebKit/efl/ewk/ewk_view.cpp:1159
#9 0x0804acf7 in viewport_set () at /home/dongjae/Projects/WebKit-Open/WebKit/Tools/EWebLauncher/main.c:277
#10 0x0804b3b4 in on_viewport_changed (user_data=0x8064490, webview=0x808e6d8, event_info=0x0)
at /home/dongjae/Projects/WebKit-Open/WebKit/Tools/EWebLauncher/main.c:475
#11 0x00238be6 in evas_object_smart_callback_call (obj=0x808e6d8, event=0x17c24c6 "viewport,changed", event_info=0x0)
at evas_object_smart.c:523
#12 0x008f31ae in ewk_view_viewport_attributes_set (o=0x808e6d8, arguments=...)
at /home/dongjae/Projects/WebKit-Open/WebKit/Source/WebKit/efl/ewk/ewk_view.cpp:4259
This crash will be fixed.
I think, create WebCore::FrameView is common patch.
How about it?
(In reply to comment #14)
> (In reply to comment #10)
> > (In reply to comment #8)
> >
> > > when user add 4th cached page, first cached page will be destroy.
> > > then frame was released and has null reference assert.
> > >
> > > so sd->frame->view() was create always.
> >
> > If we don't use cached page or frame is not null, I think we need to check the condition. If frame already has a view instance, we don't need to create new view. Please consider both normal case and your case.
>
> Currently, EWeblauncher Browser has some crash when use click some link.(Using Current Revision)
Great. I couldn't catch how to fix it.
I wish new patch. :)
(In reply to comment #13)
> >> Source/WebKit/efl/ChangeLog:8
> >> + add CachedPage's content size update func becuase CachedPage 's content size was delete when it was cached.
> >
> > Line contains tab character. [whitespace/tab] [5]
>
> The English here is a bit hard do understand. Could you try to rephrase your description?
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4508
> > + * @param[in] o a Evas_Object m_view
>
> There is no need for the '[in'] here.
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4509
> > + * @param[in] frame a Evas_Object m_frame
>
> Ditto.
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4510
> > + * @return void
>
> There is no need for this if the function does not return anything.
>
> It'd also be good to add an "@internal" tag in the apidox as well.
>
> >> Tools/ChangeLog:6
> >> + when view port attribute was set, zoom range and user scalable value must setted before fixed size set
> >
> > Line contains tab character. [whitespace/tab] [5]
>
> The English here could be cleaned up a little too.
Thanks your comments.
Comment on attachment 98814[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=98814&action=review> Source/WebKit/efl/ewk/ewk_view.cpp:4515
> + WebCore::Frame* main_frame = ewk_frame_core_get(sd->main_frame);
> + if (!main_frame)
> + return;
Sorry, you can use priv->main_frame. I confused.
I think that you don't need to check whether it is null.
> Tools/ChangeLog:8
> + when view port changed, zoom level was set. ewk_view_zoom_set() check zoom range.
When
(In reply to comment #19)
> (From update of attachment 98814[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98814&action=review
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4515
> > + WebCore::Frame* main_frame = ewk_frame_core_get(sd->main_frame);
> > + if (!main_frame)
> > + return;
>
> Sorry, you can use priv->main_frame. I confused.
> I think that you don't need to check whether it is null.
>
> > Tools/ChangeLog:8
> > + when view port changed, zoom level was set. ewk_view_zoom_set() check zoom range.
>
> When
did you mean " sd->_priv->main_frame->loader()->history()->currentItem(); "?
Comment on attachment 98814[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=98814&action=review> Source/WebKit/efl/ChangeLog:13
> + because, WebCore::FrameView will be create always when user go to new page.
Adhere indentation in here
> Source/WebKit/efl/ewk/ewk_view.cpp:4516
> +
You already get priv instance from sd. So, you can use priv->main_frame. And also you already check if it is null or not in EWK_VIEW_PRIV_GET_OR_RETURN().
Comment on attachment 98814[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=98814&action=review> Source/WebKit/efl/ewk/ewk_view.cpp:4506
> + * @param o a Evas_Object m_view
Change a with an. And also, it is better to use below expression.
* @param o view object to get cached page.
> Source/WebKit/efl/ewk/ewk_view.cpp:4519
> + return;
Move return to item below in order to adhere indentation.
> Source/WebKit/efl/ewk/ewk_view.cpp:4522
> + main_frame->view()->adjustViewSize();
ditto.
> Tools/EWebLauncher/main.c:276
> +
Unnecessary blank line.
(In reply to comment #22)
> (From update of attachment 98814[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98814&action=review
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4506
> > + * @param o a Evas_Object m_view
>
> Change a with an. And also, it is better to use below expression.
>
> * @param o view object to get cached page.
>
change description
> > Source/WebKit/efl/ewk/ewk_view.cpp:4519
> > + return;
>
> Move return to item below in order to adhere indentation.
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4522
> > + main_frame->view()->adjustViewSize();
>
> ditto.
>
Indentation was right, please check it again.
> > Tools/EWebLauncher/main.c:276
> > +
>
> Unnecessary blank line.
remove blank line.
Comment on attachment 98834[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=98834&action=review> Tools/EWebLauncher/main.c:277
> + ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
If we move fixed_layout_set and zoom_set to after zoom_range_set, user_scalable_set, we may not set init width, height and initial zoom level when website defines wrong zoom range. I mean we need to set initial width,height and initial scale value before setting zoom range.
(In reply to comment #27)
> (From update of attachment 98834[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98834&action=review
>
> > Tools/EWebLauncher/main.c:277
> > + ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
>
> If we move fixed_layout_set and zoom_set to after zoom_range_set, user_scalable_set, we may not set init width, height and initial zoom level when website defines wrong zoom range. I mean we need to set initial width,height and initial scale value before setting zoom range.
In my patch, back/forward zoom value was wrong so i changed.
I think this is different issue. create new bug is better.
How about?
(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 98834[details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=98834&action=review
> >
> > > Tools/EWebLauncher/main.c:277
> > > + ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
> >
> > If we move fixed_layout_set and zoom_set to after zoom_range_set, user_scalable_set, we may not set init width, height and initial zoom level when website defines wrong zoom range. I mean we need to set initial width,height and initial scale value before setting zoom range.
>
> In my patch, back/forward zoom value was wrong so i changed.
>
> I think this is different issue. create new bug is better.
>
> How about?
Ok, file a new bug.
> Source/WebKit/efl/ChangeLog:8
> + 1) Add contentsSize update function for cached page resotre ( ewk_view_cached_page_restore() )
resotre -> restore
The spaces in the parentheses are not necessary.
> Source/WebKit/efl/ChangeLog:9
> + 2) Change return value for enable EFL Port back/forward Cache ( FrameLoaderClientEfl::canCachePage() )
Same thing about spaces.
> Source/WebKit/efl/ChangeLog:13
> + because, WebCore::FrameView will be create always when user go to new page.
The comma after because is not needed.
I'd rephrase it as "WebCore::FrameView will always be created when a new page is loaded" (or opened).
> Source/WebKit/efl/ChangeLog:14
> + Currently, EWebLauncher browser has crashes.
I'd rephrase this section as "This patch also ends up fixing two crashes in EWebLauncher: one when loading the 4th cached page and another triggered when a user clicks any link".
> Source/WebKit/efl/ewk/ewk_view.cpp:4502
> +/**
Missing blank line before this. Please also add an "@internal" tag, as this function is defined in ewk_private.h
(In reply to comment #30)
> > Source/WebKit/efl/ChangeLog:8
> > + 1) Add contentsSize update function for cached page resotre ( ewk_view_cached_page_restore() )
>
> resotre -> restore
> The spaces in the parentheses are not necessary.
>
> > Source/WebKit/efl/ChangeLog:9
> > + 2) Change return value for enable EFL Port back/forward Cache ( FrameLoaderClientEfl::canCachePage() )
>
> Same thing about spaces.
>
> > Source/WebKit/efl/ChangeLog:13
> > + because, WebCore::FrameView will be create always when user go to new page.
>
> The comma after because is not needed.
> I'd rephrase it as "WebCore::FrameView will always be created when a new page is loaded" (or opened).
>
> > Source/WebKit/efl/ChangeLog:14
> > + Currently, EWebLauncher browser has crashes.
>
> I'd rephrase this section as "This patch also ends up fixing two crashes in EWebLauncher: one when loading the 4th cached page and another triggered when a user clicks any link".
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4502
> > +/**
>
> Missing blank line before this. Please also add an "@internal" tag, as this function is defined in ewk_private.h
Thanks your comment.
There are some "( foo )" instead of "(foo)" in the first ChangeLog, and the text after 3) is indented weirdly.
Other than that, r+ from my side after the Windows bot stops complaining.
(In reply to comment #35)
> There are some "( foo )" instead of "(foo)" in the first ChangeLog, and the text after 3) is indented weirdly.
>
I cann't understand your comment.
How about below?
=====================================
1) Add contentsSize update function for cached page restore
2) Change return value for enable EFL Port back/forward Cache
3) Delete below code in ewk_frame_view_create_for_view()
if (sd->frame->view())
return;
because WebCore::FrameView will always be created when a new page is opened.
This patch also ends up fixing two crashes in EWebLauncher: one when loading the 4th cached page
and another triggered when a user clicks any link
=====================================
> Other than that, r+ from my side after the Windows bot stops complaining.
what r+? please tell me more details.
Thanks your comments.
(In reply to comment #35)
> There are some "( foo )" instead of "(foo)" in the first ChangeLog, and the text after 3) is indented weirdly.
>
> Other than that, r+ from my side after the Windows bot stops complaining.
I change my ChangeLog.
(In reply to comment #10)
> (In reply to comment #8)
>
> > when user add 4th cached page, first cached page will be destroy.
> > then frame was released and has null reference assert.
> >
> > so sd->frame->view() was create always.
>
> If we don't use cached page or frame is not null, I think we need to check the condition. If frame already has a view instance, we don't need to create new view. Please consider both normal case and your case.
Did you reply my question ? I can't find your reply this thread.
(In reply to comment #44)
> (In reply to comment #10)
> > (In reply to comment #8)
> >
> > > when user add 4th cached page, first cached page will be destroy.
> > > then frame was released and has null reference assert.
> > >
> > > so sd->frame->view() was create always.
> >
> > If we don't use cached page or frame is not null, I think we need to check the condition. If frame already has a view instance, we don't need to create new view. Please consider both normal case and your case.
>
> Did you reply my question ? I can't find your reply this thread.
My answer is Comment#14.
Additional Comment:: when create view, WebCore::Frame::CreateView was call below api for create FrameView
setView(0) -> view = FrameView::createView() ->setView(view);
so, there are no problem to create new page.
Comment on attachment 99605[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=99605&action=review> Source/WebKit/efl/ewk/ewk_view.cpp:4593
> + * Restore cached page contentsSize
First, %s/contentsSize/content size/g
Second, missing a period end of line.
Third, add a blank between summary and description.
Comment on attachment 99681[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=99681&action=review> Source/WebKit/efl/ChangeLog:24
> + 1) Add contentsSize update function for cached page restore
> + 2) Change return value for enable EFL Port back/forward Cache
> + 3) Delete below code in ewk_frame_view_create_for_view()
> + if (sd->frame->view())
> + return;
> + because WebCore::FrameView will always be created when a new page is opened.
> + This patch also ends up fixing two crashes in EWebLauncher: one when loading the 4th cached page
> + and another triggered when a user clicks any link
> +
> + * WebCoreSupport/FrameLoaderClientEfl.cpp:
> + (WebCore::FrameLoaderClientEfl::dispatchDidCommitLoad):
> + (WebCore::FrameLoaderClientEfl::canCachePage):
> + * ewk/ewk_frame.cpp:
> + (ewk_frame_view_create_for_view):
> + * ewk/ewk_private.h:
> + * ewk/ewk_view.cpp:
> + (ewk_view_cached_page_restore):
Instead of putting the summary of what you did above the empty method name template, why not just fill out the template with the appropriate information? :)
Created attachment 101431[details]
patch for efl port back/forward cache enable
Check Martin's comment about my changes. and modify patch.
Thank Martin Robinson.
Comment on attachment 101431[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=101431&action=review
Needs one more interaction for clearance.
> Source/WebKit/efl/ewk/ewk_view.cpp:4595
> + * Restore cached page contents size.
> + *
> + * This method must be called before WebCore::FrameView::layout().
How does the code ensure that? It is too error-prone, imo.
> Tools/ChangeLog:10
> + When view port changed, zoom level was set. ewk_view_zoom_set() check zoom range
> + but zoom range value was old one.
> + So, I changed setting sequence.
typo: checkS*
Not sure if I understood this sentence.
(In reply to comment #53)
> (From update of attachment 101431[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101431&action=review
>
> Needs one more interaction for clearance.
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:4595
> > + * Restore cached page contents size.
> > + *
> > + * This method must be called before WebCore::FrameView::layout().
>
> How does the code ensure that? It is too error-prone, imo.
>
> > Tools/ChangeLog:10
> > + When view port changed, zoom level was set. ewk_view_zoom_set() check zoom range
> > + but zoom range value was old one.
> > + So, I changed setting sequence.
>
> typo: checkS*
>
> Not sure if I understood this sentence.
Thanks your comment.
> > Source/WebKit/efl/ewk/ewk_view.cpp:4595
> > + * Restore cached page contents size.
> > + *
> > + * This method must be called before WebCore::FrameView::layout().
>
> How does the code ensure that? It is too error-prone, imo.
:: It is must be called before cached page restore. I will be change this comment more clearly.
> > + When view port changed, zoom level was set. ewk_view_zoom_set() check zoom range
> > + but zoom range value was old one.
> > + So, I changed setting sequence.
>
> typo: checkS*
>
> Not sure if I understood this sentence.
Currently, EWebLaunch browser view port change function has some bugs.
Zoom range must be setted before zoom set, but not. so I changed it.
Comment on attachment 103315[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=103315&action=review> Source/WebKit/efl/ChangeLog:22
> + (ewk_view_cached_page_restore): Added. adjust view size for restored cached page.
> +
> +
too many empty lines.
> Source/WebKit/efl/ewk/ewk_frame.cpp:-1459
> - if (sd->frame->view())
> - return;
> -
how is this related? Could you explain?
> Source/WebKit/efl/ewk/ewk_view.cpp:3664
> +* Restore cached page contents size.
RestoreS
> Tools/ChangeLog:10
> + Zoom range value must be setted before zoom set, but not in EWebLauncher.
> +
> +
too many empty lines.
(In reply to comment #56)
> (From update of attachment 103315[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103315&action=review
>
> > Source/WebKit/efl/ChangeLog:22
> > + (ewk_view_cached_page_restore): Added. adjust view size for restored cached page.
> > +
> > +
>
> too many empty lines.
>
> > Source/WebKit/efl/ewk/ewk_frame.cpp:-1459
> > - if (sd->frame->view())
> > - return;
> > -
>
> how is this related? Could you explain?
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:3664
> > +* Restore cached page contents size.
>
> RestoreS
>
> > Tools/ChangeLog:10
> > + Zoom range value must be setted before zoom set, but not in EWebLauncher.
> > +
> > +
>
> too many empty lines.
Thanks, I will remove empty lines.
> > Source/WebKit/efl/ewk/ewk_frame.cpp:-1459
> > - if (sd->frame->view())
> > - return;
> > -
>
EFL Port, FrameView was created, it will be re-use other page, but I think FrameView will be create always when use go to new page, isn't it?
when user add 4th cached page, first cached page will be destroy.
then frame was released and has null reference assert.
so sd->frame->view() was create always.
test case ::
1. add 3 cached page
2. add one more cached page
3. scroll page
4. assert
Program received signal SIGSEGV, Segmentation fault.
0x00b671ed in WebCore::EventHandler::setFrameWasScrolledByUser (this=0x4f0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2997
2997 FrameView* v = m_frame->view();
(gdb) bt
#0 0x00b671ed in WebCore::EventHandler::setFrameWasScrolledByUser (this=0x4f0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2997
#1 0x00b6714d in WebCore::EventHandler::sendScrollEvent (this=0x4f0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/EventHandler.cpp:2990
#2 0x00b7af83 in WebCore::FrameView::scrollPositionChanged (this=0x80b98e0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/FrameView.cpp:1468
#3 0x00b7d3ff in WebCore::FrameView::scrollTo (this=0x80b98e0, newOffset=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/page/FrameView.cpp:2110
#4 0x00bd65a7 in WebCore::ScrollView::setScrollOffset (this=0x80b98e0, offset=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollView.cpp:351
#5 0x00bd2592 in WebCore::ScrollableArea::setScrollOffsetFromAnimation (this=0x80b990c, offset=...)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollableArea.cpp:133
#6 0x0127e999 in WebCore::ScrollAnimator::notityPositionChanged (this=0x80935e0)
at /home/dongjae/Projects/WebKit_EFL/Source/WebCore/platform/ScrollAnimator.cpp:129
#7 0x0127e4a9 in WebCore::ScrollAnimator::scroll (this=0x80935e0, orientation=WebCore::VerticalScrollbar, step=1, multiplier=40)
Comment on attachment 103329[details]
patch for efl port back/forward cache enable
View in context: https://bugs.webkit.org/attachment.cgi?id=103329&action=review> Source/WebKit/efl/ChangeLog:15
> + (ewk_frame_view_create_for_view): Delete WebCore::FrameView check code
> + because WebCore::FrameView will always be created when a new page is opened but not in EFL port.
It is really strange. FrameView being persisted cross page loads.
> Tools/ChangeLog:8
> + Zoom range value must be setted before zoom set, but not in EWebLauncher.
setted = set
(In reply to comment #59)
> (From update of attachment 103329[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103329&action=review
>
> > Source/WebKit/efl/ChangeLog:15
> > + (ewk_frame_view_create_for_view): Delete WebCore::FrameView check code
> > + because WebCore::FrameView will always be created when a new page is opened but not in EFL port.
>
> It is really strange. FrameView being persisted cross page loads.
GTK and QT port create FrameView always when go to new page.
and when forth page cached, there are crash. in this case, there no FrameView.
>
> > Tools/ChangeLog:8
> > + Zoom range value must be setted before zoom set, but not in EWebLauncher.
>
> setted = set
I will be changed.
Thanks. your comment.
(In reply to comment #60)
> (In reply to comment #59)
> > (From update of attachment 103329[details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=103329&action=review
> >
> > > Source/WebKit/efl/ChangeLog:15
> > > + (ewk_frame_view_create_for_view): Delete WebCore::FrameView check code
> > > + because WebCore::FrameView will always be created when a new page is opened but not in EFL port.
> >
> > It is really strange. FrameView being persisted cross page loads.
>
> GTK and QT port create FrameView always when go to new page.
>
> and when forth page cached, there are crash. in this case, there no FrameView.
This code already merged main branch.
Please check "https://bugs.webkit.org/show_bug.cgi?id=66690"
>
> >
> > > Tools/ChangeLog:8
> > > + Zoom range value must be setted before zoom set, but not in EWebLauncher.
> >
> > setted = set
>
> I will be changed.
>
> Thanks. your comment.
Reverting from RESOLVED/LATER to UNCONFIRMED. This has not been resolved yet, so the bug shouldn't usually be closed (the fact that the patch has no flags is enough to make it not show up for reviewers, for example).
2011-06-26 23:29 PDT, DongJae KIM
2011-06-27 16:52 PDT, DongJae KIM
2011-06-27 17:36 PDT, DongJae KIM
2011-06-27 17:54 PDT, DongJae KIM
2011-06-27 18:06 PDT, DongJae KIM
2011-06-28 14:27 PDT, DongJae KIM
2011-06-28 15:40 PDT, DongJae KIM
2011-06-29 17:32 PDT, DongJae KIM
gyuyoung.kim: commit-queue-
2011-07-03 22:41 PDT, DongJae KIM
2011-07-03 23:34 PDT, DongJae KIM
2011-07-04 00:58 PDT, DongJae KIM
2011-07-05 01:49 PDT, DongJae KIM
2011-07-20 00:24 PDT, DongJae KIM
2011-08-08 16:53 PDT, DongJae KIM
2011-08-08 21:36 PDT, DongJae KIM
2011-09-01 00:22 PDT, DongJae KIM