Bug 65192 - [EFL] Do not crash if the Cairo surface cannot be created.
Summary: [EFL] Do not crash if the Cairo surface cannot be created.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-26 11:04 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2011-09-06 10:48 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.61 KB, patch)
2011-07-26 11:09 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2011-07-27 12:08 PDT, Raphael Kubo da Costa (:rakuco)
leandro: review-
leandro: commit-queue-
Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2011-07-27 13:30 PDT, Leandro Pereira
no flags Details | Formatted Diff | Diff
Patch (3.87 KB, patch)
2011-09-02 05:45 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Be less strict about what sizes we consider to be invalid (3.71 KB, patch)
2011-09-06 07:55 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2011-07-26 11:04:36 PDT
[EFL] Do not crash if the Cairo surface cannot be created.
Comment 1 Raphael Kubo da Costa (:rakuco) 2011-07-26 11:09:07 PDT
Created attachment 102028 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-07-26 11:10:35 PDT
CC'ing mrobinson instead of tkent, as this is cairo-related.
Comment 3 Gyuyoung Kim 2011-07-26 11:21:44 PDT
Comment on attachment 102028 [details]
Patch

Attachment 102028 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9251384
Comment 4 Martin Robinson 2011-07-26 14:40:09 PDT
Wouldn't it be better to use the current clip to figure out how big the surface should be rather than to just fail?
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-07-27 12:08:39 PDT
Created attachment 102164 [details]
Patch
Comment 6 Leandro Pereira 2011-07-27 12:14:36 PDT
Comment on attachment 102164 [details]
Patch

The 'return false'  substituting 'return true'  in themePartCacheEntrySurfaceCreate() shouldn't be there.
Comment 7 Leandro Pereira 2011-07-27 13:30:37 PDT
Created attachment 102179 [details]
Patch
Comment 8 Martin Robinson 2011-07-27 19:38:47 PDT
Comment on attachment 102179 [details]
Patch

By my comment I actually was asking whether or not you could simply cache only what's within the clipping region, versus allocating a cache area the size of the entire widget.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-08-26 10:56:24 PDT
Sorry for leaving this one behind for so long.

I was talking to Leandro about your question/suggestion a few minutes ago. If we were to cache what's visible, wouldn't we need to keep track of viewport size changes and all that, making the code much bigger?
Comment 10 Martin Robinson 2011-08-26 13:42:08 PDT
(In reply to comment #9)
> Sorry for leaving this one behind for so long.
> 
> I was talking to Leandro about your question/suggestion a few minutes ago. If we were to cache what's visible, wouldn't we need to keep track of viewport size changes and all that, making the code much bigger?

I don't know exactly how your theme cache works, but it seems that if you should already be checking whether the widget changes size each time you draw. You'd just need to add one more condition determining whether or not the clip region has moved or changed size. I'm not sure how feasible that is.
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-09-02 05:41:07 PDT
(In reply to comment #10)
> I don't know exactly how your theme cache works, but it seems that if you should already be checking whether the widget changes size each time you draw. You'd just need to add one more condition determining whether or not the clip region has moved or changed size. I'm not sure how feasible that is.

After spending some time on this, it looks like implementing this suggestion could be feasible, but would require quite some effort. On the one hand, Edje (which actually renders the theme) has a hardcoded limit of 20000x20000 pixels for a widget's dimensions; on the other hand, the way RenderThemeEfl is currently implemented makes it a bit difficult to track the clipping region and decide where exactly in the cairo context to paint the rendered element (when scrolling, scaling and other things are taken into account it becomes even harder).

For now, I think it's OK to just not display elements which are too big (a 1-month-old GtkLauncher build I have here just displays the top of the textarea in the mentioned testcase, and my distro's Chromium displays mostly garbage after some point too).

I'm submitting a new patch with a better heuristic in a few minutes.
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-09-02 05:45:37 PDT
Created attachment 106118 [details]
Patch
Comment 13 Raphael Kubo da Costa (:rakuco) 2011-09-06 07:55:00 PDT
Created attachment 106419 [details]
Be less strict about what sizes we consider to be invalid
Comment 14 WebKit Review Bot 2011-09-06 10:48:47 PDT
Comment on attachment 106419 [details]
Be less strict about what sizes we consider to be invalid

Clearing flags on attachment: 106419

Committed r94578: <http://trac.webkit.org/changeset/94578>
Comment 15 WebKit Review Bot 2011-09-06 10:48:53 PDT
All reviewed patches have been landed.  Closing bug.