Bug 63013 - [EFL] Page Cache was not Enabled(Enable Page Cache)
Summary: [EFL] Page Cache was not Enabled(Enable Page Cache)
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 67355 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-20 14:29 PDT by DongJae KIM
Modified: 2012-07-05 16:55 PDT (History)
7 users (show)

See Also:


Attachments
patch for efl port back/forward cache enable (5.62 KB, patch)
2011-06-26 23:29 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.65 KB, patch)
2011-06-27 16:52 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.55 KB, patch)
2011-06-27 17:36 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.58 KB, patch)
2011-06-27 17:54 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.58 KB, patch)
2011-06-27 18:06 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.53 KB, patch)
2011-06-28 14:27 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.59 KB, patch)
2011-06-28 15:40 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.90 KB, patch)
2011-06-29 17:32 PDT, DongJae KIM
gyuyoung.kim: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.50 KB, patch)
2011-07-03 22:41 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.47 KB, patch)
2011-07-03 23:34 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.47 KB, patch)
2011-07-04 00:58 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.47 KB, patch)
2011-07-05 01:49 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.47 KB, patch)
2011-07-20 00:24 PDT, DongJae KIM
tonikitoo: review-
Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.42 KB, patch)
2011-08-08 16:53 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
patch for efl port back/forward cache enable (6.42 KB, patch)
2011-08-08 21:36 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff
Enable EFL Port PageCache (5.74 KB, patch)
2011-09-01 00:22 PDT, DongJae KIM
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description DongJae KIM 2011-06-20 14:29:38 PDT
On EFL Port, Page Cache was disabled, 

bool FrameLoaderClientEfl::canCachePage() const
{
    return false;
}
Comment 1 DongJae KIM 2011-06-26 23:29:01 PDT
Created attachment 98662 [details]
patch for efl port back/forward cache enable

Add Enable back/forward Cache code for efl port.
Comment 2 WebKit Review Bot 2011-06-26 23:32:02 PDT
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.
Comment 3 Gyuyoung Kim 2011-06-26 23:35:02 PDT
(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 4 Gyuyoung Kim 2011-06-26 23:38:31 PDT
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.
Comment 5 Ryuan Choi 2011-06-26 23:45:19 PDT
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?
Comment 6 Gyuyoung Kim 2011-06-27 00:02:17 PDT
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

> Source/WebKit/efl/ewk/ewk_frame.cpp:-1906
> -        return;

Why do you remove this code ?
Comment 7 DongJae KIM 2011-06-27 00:30:11 PDT
(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.
Comment 8 DongJae KIM 2011-06-27 00:36:35 PDT
(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
Comment 9 DongJae KIM 2011-06-27 00:40:29 PDT
(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.
Comment 10 Gyuyoung Kim 2011-06-27 00:49:12 PDT
(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.
Comment 11 Ryuan Choi 2011-06-27 01:13:12 PDT
(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.
Comment 12 Ryuan Choi 2011-06-27 03:55:52 PDT
(In reply to comment #9)
> (In reply to comment #5)
> > > 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.

Thanks, I checked it.
You're right.
Comment 13 Raphael Kubo da Costa (:rakuco) 2011-06-27 05:52:24 PDT
>> 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.
Comment 14 DongJae KIM 2011-06-27 14:18:22 PDT
(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?
Comment 15 DongJae KIM 2011-06-27 14:18:49 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > > 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.
> 
> Thanks, I checked it.
> You're right.

Thanks.
Comment 16 Ryuan Choi 2011-06-27 14:50:10 PDT
(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. :)
Comment 17 DongJae KIM 2011-06-27 15:40:53 PDT
(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 18 DongJae KIM 2011-06-27 16:52:16 PDT
Created attachment 98814 [details]
patch for efl port back/forward cache enable

Second patch for check comments
Comment 19 Ryuan Choi 2011-06-27 17:07:40 PDT
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
Comment 20 DongJae KIM 2011-06-27 17:13:36 PDT
(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 21 Gyuyoung Kim 2011-06-27 17:18:54 PDT
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 22 Gyuyoung Kim 2011-06-27 17:25:31 PDT
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.
Comment 23 DongJae KIM 2011-06-27 17:36:32 PDT
Created attachment 98828 [details]
patch for efl port back/forward cache enable

Check ryuan.choi's comment
Comment 24 DongJae KIM 2011-06-27 17:54:57 PDT
Created attachment 98831 [details]
patch for efl port back/forward cache enable

check Gyuyoung Kim comment
Comment 25 DongJae KIM 2011-06-27 17:56:15 PDT
(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 26 DongJae KIM 2011-06-27 18:06:16 PDT
Created attachment 98834 [details]
patch for efl port back/forward cache enable
Comment 27 Gyuyoung Kim 2011-06-27 18:09:36 PDT
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.
Comment 28 DongJae KIM 2011-06-27 18:27:12 PDT
(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?
Comment 29 Gyuyoung Kim 2011-06-27 18:29:54 PDT
(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.
Comment 30 Raphael Kubo da Costa (:rakuco) 2011-06-28 06:03:29 PDT
> 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
Comment 31 DongJae KIM 2011-06-28 14:26:32 PDT
(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.
Comment 32 DongJae KIM 2011-06-28 14:27:03 PDT
Created attachment 98972 [details]
patch for efl port back/forward cache enable
Comment 33 DongJae KIM 2011-06-28 15:40:55 PDT
Created attachment 98983 [details]
patch for efl port back/forward cache enable
Comment 34 DongJae KIM 2011-06-28 15:56:20 PDT
Comment on attachment 98972 [details]
patch for efl port back/forward cache enable

There are some conflict things.
Comment 35 Raphael Kubo da Costa (:rakuco) 2011-06-29 06:04:55 PDT
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.
Comment 36 DongJae KIM 2011-06-29 15:00:18 PDT
(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.
Comment 37 DongJae KIM 2011-06-29 17:25:26 PDT
(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.
Comment 38 DongJae KIM 2011-06-29 17:32:36 PDT
Created attachment 99192 [details]
patch for efl port back/forward cache enable
Comment 39 Gyuyoung Kim 2011-07-03 22:03:48 PDT
Comment on attachment 99192 [details]
patch for efl port back/forward cache enable

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

> Source/WebKit/efl/ChangeLog:40
> +>>>>>>> .r90072

Remove this.

> Tools/ChangeLog:280
> +>>>>>>> .r90071

ditto.
Comment 40 DongJae KIM 2011-07-03 22:22:55 PDT
(In reply to comment #39)
> (From update of attachment 99192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99192&action=review
> 
> > Source/WebKit/efl/ChangeLog:40
> > +>>>>>>> .r90072
> 
> Remove this.
> 
> > Tools/ChangeLog:280
> > +>>>>>>> .r90071
> 
> ditto.

Oh.. sorry..I will be fix it.
Thanks.
Comment 41 DongJae KIM 2011-07-03 22:41:41 PDT
Created attachment 99597 [details]
patch for efl port back/forward cache enable

Fix Change Log.
Comment 42 DongJae KIM 2011-07-03 23:34:55 PDT
Created attachment 99601 [details]
patch for efl port back/forward cache enable

Change Log Fix.
Comment 43 Gyuyoung Kim 2011-07-03 23:51:34 PDT
Comment on attachment 99601 [details]
patch for efl port back/forward cache enable

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

> Source/WebKit/efl/ewk/ewk_view.cpp:4592
> + * @internal

You have to add summary for this API first to here. Make reference to other APIs.
Comment 44 Gyuyoung Kim 2011-07-03 23:55:53 PDT
(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.
Comment 45 DongJae KIM 2011-07-04 00:58:42 PDT
Created attachment 99605 [details]
patch for efl port back/forward cache enable

Fixe ewk_view.cpp comment.(apply Gyuyoung's comment)
Comment 46 DongJae KIM 2011-07-04 01:41:56 PDT
(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 47 Raphael Kubo da Costa (:rakuco) 2011-07-04 07:17:54 PDT
LGTM.
Comment 48 Gyuyoung Kim 2011-07-05 00:40:14 PDT
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 49 DongJae KIM 2011-07-05 01:49:24 PDT
Created attachment 99681 [details]
 patch for efl port back/forward cache enable

Change patch, Fix Gyuyoung's comment
Thanks.
Comment 50 DongJae KIM 2011-07-05 01:49:25 PDT
Created attachment 99682


Change patch, Fix Gyuyoung's comment
Thanks.
Comment 51 Martin Robinson 2011-07-19 19:35:06 PDT
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? :)
Comment 52 DongJae KIM 2011-07-20 00:24:47 PDT
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 53 Antonio Gomes 2011-07-22 07:57:23 PDT
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.
Comment 54 DongJae KIM 2011-08-08 16:27:32 PDT
(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 55 DongJae KIM 2011-08-08 16:53:02 PDT
Created attachment 103315 [details]
patch for efl port back/forward cache enable
Comment 56 Antonio Gomes 2011-08-08 20:02:19 PDT
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.
Comment 57 DongJae KIM 2011-08-08 21:09:59 PDT
(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 58 DongJae KIM 2011-08-08 21:36:38 PDT
Created attachment 103329 [details]
patch for efl port back/forward cache enable
Comment 59 Antonio Gomes 2011-08-29 07:41:47 PDT
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
Comment 60 DongJae KIM 2011-08-31 17:37:05 PDT
(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.
Comment 61 DongJae KIM 2011-08-31 20:54:16 PDT
(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.
Comment 62 DongJae KIM 2011-08-31 21:14:03 PDT
*** Bug 67355 has been marked as a duplicate of this bug. ***
Comment 63 DongJae KIM 2011-09-01 00:22:33 PDT
Created attachment 105910 [details]
Enable EFL Port PageCache
Comment 64 Kenneth Rohde Christiansen 2011-09-12 16:00:36 PDT
Comment on attachment 105910 [details]
Enable EFL Port PageCache

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

> Source/WebKit/efl/ChangeLog:8
> +2011-09-01  DongJae KIM  <dongjae1.kim@samsung.com>
> +
> +        [EFL] Enable EFL Port Page Cache function.
> +        https://bugs.webkit.org/show_bug.cgi?id=63013
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Enable back/forward cache for EFL port.

We dont seems to need something like ewk_view_cached_page_restore in public API for Qt, so why is this really needed? This doesn't seem like the right solution.
Comment 65 Raphael Kubo da Costa (:rakuco) 2011-12-25 18:02:36 PST
Comment on attachment 105910 [details]
Enable EFL Port PageCache

Clearing r? flag while Kenneth's question is not answered.
Comment 66 Raphael Kubo da Costa (:rakuco) 2012-01-13 09:26:43 PST
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).
Comment 67 Ryuan Choi 2012-06-17 17:37:55 PDT
(In reply to comment #66)
> 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).

DongJae,
Can you address whether we still need this?

page cashe looks already enabled after r116341.
http://trac.webkit.org/browser/trunk/Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp#L935
Comment 68 Ryuan Choi 2012-07-05 16:55:16 PDT
Close this bug because page cache is already enabled.