Created attachment 196383 [details] [review]
Add interactive icon generation for web application.

I had a quick look, not much but I guess a bit of feedback in an early stage is not a bad thing:

- I didn't understand the interaction very much. I see it's possible to move around the webpage in the preview in order to find something suitable for the icon. But can I also shrink or expand the preview to fit something bigger/smaller in the preview?

- The black background color looks a bit alien. Maybe you should try to get some feedback from the designers on how this should look.

- While you drag the mouse the cursor changes to the "I-beam" pointer. Maybe the drag one would suit better.

- I got stuff like this, afterwards ephy got very sluggish:
 (epiphany:31629): GLib-GIO-CRITICAL **: g_app_info_launch: assertion `G_IS_APP_INFO (appinfo)' failed
*** glibc detected *** epiphany: malloc(): smallbin double linked list corrupted: 0x0000000001426e10 ***

- The cancel button doesn't work.

- I think you could split the patch in different ones so that it's easier to review. For instance, the rework of web application saving could be one, the methods you add to ephy-web-application-utils another one, the new API for dom maniupulation the next one, and so on.

Hi Claudio,
Thanks for the review. As you noted, this is far from being commitable, just wanted
to get some early feedback like yours.

(In reply to comment #2)
> I had a quick look, not much but I guess a bit of feedback in an early stage is
> not a bad thing:
> 
> - I didn't understand the interaction very much. I see it's possible to move
> around the webpage in the preview in order to find something suitable for the
> icon. But can I also shrink or expand the preview to fit something
> bigger/smaller in the preview?

The intended interaction is to make the screengrab of the web app pannable, so the user can choose a realy distinguishing part of the website as the background of the
icon.
We might add a zooming factor if that is what you are referring to. In that case the
current approach might prove to pixelled or blurred... I'll try to see if we can fit an iframe with actual content instead of a screenshot.

> 
> - The black background color looks a bit alien. Maybe you should try to get
> some feedback from the designers on how this should look.

Yep, this is a side effect of making the webview transparent. The alpa is way of. Maybe I need to put a GtkFrame behind to have our lovely grey back :)

> 
> - While you drag the mouse the cursor changes to the "I-beam" pointer. Maybe
> the drag one would suit better.
> 

Yeah, I need to fix that up in the js. Also the stacking of the svg elements sometime steal the mouse event somehow.

> - I got stuff like this, afterwards ephy got very sluggish:
>  (epiphany:31629): GLib-GIO-CRITICAL **: g_app_info_launch: assertion
> `G_IS_APP_INFO (appinfo)' failed
> *** glibc detected *** epiphany: malloc(): smallbin double linked list
> corrupted: 0x0000000001426e10 ***
> 

Could you provide a stacktrace ?

> - The cancel button doesn't work.

I think it never did since the introduction of the feature.

> - I think you could split the patch in different ones so that it's easier to
> review. For instance, the rework of web application saving could be one, the
> methods you add to ephy-web-application-utils another one, the new API for dom
> maniupulation the next one, and so on.

Well the refactoring part of the saving method should indeed go in a separate patch. Not sure about the dom part, as it has currently no other use than this patch.
Also after this patch I'd like to put all the web app dialog code in its own file, windows-commands is frithening as-is. (most of its code should go in GtkAction anyway, IMHO).

Happy coding!

Created attachment 196629 [details] [review]
Generalize and extract widget screengrabbing from EphyWebView.

Created attachment 196631 [details] [review]
Add DOM helpers for attributes manipulation.

Created attachment 196632 [details] [review]
Get the title text entry only once.

Created attachment 196633 [details] [review]
Move the existence checking logic for webapp from window-commands to web-apps-utils.

Created attachment 196634 [details] [review]
Modify web app saving callback by removing some indentation levels.

Created attachment 196635 [details] [review]
Extract notification sending from save_as function.

Created attachment 196636 [details] [review]
Avoid passing the webview low on the stack.

Created attachment 196637 [details] [review]
Interactive web application icons generation.

Current state: 
http://people.igalia.com/amazari/ephy-create-webapp-icon-svg.webm

Some note:

updated:
- broke the patch in severals bits
- small updates to the svg

todo:
- fix icons not showing in the shell. Is putting the icons in the right XDG icon dir enough ? Doesn't seem so :(
- hide most of the webview, showing only a 256x256 rectangle
- make the bg gray
- improve the dialog design with lapo's help

like-to-do:
- extract the webapp dialog into a separate file (window-commands.c) is too huge IMHO
- make web-app-utils a proper GObject WebApplication class.

Created attachment 197356 [details] [review]
Make web application persisting and deleteing atomic. Handle case where application is in incoherent state (basicaly deleting everything) Intruduce a service for handling web application Bubbling up GError from WebApplicationService Port Io to GIO

Created attachment 197690 [details] [review]
Overhaul web application service;

Make web application persisting and deleteing atomic.
Watch web application container dir.
Handle case where application is in incoherent state (basicaly deleting everything).
Introduce a service for handling web application Bubbling up GError from WebApplicationService.
Port Io to GIO

Additionally this patch adds test-cases for the newly introduced web application service.

*** Bug 660758 has been marked as a duplicate of this bug. ***

Review of attachment 196636 [details] [review]:

Just pushed this to master with a small change, thanks!

*** Bug 672982 has been marked as a duplicate of this bug. ***

Created attachment 210800 [details] [review]
window-commands: get app name only once

Avoid multiple calls to gtk_entry_get_text.

Signed-off-by: Diego Escalante Urrelo <diegoe@igalia.com>
--

Updated to apply to master.

Created attachment 210808 [details] [review]
e-web-app-utils: add ephy_web_application_exists

Signed-off-by: Diego Escalante Urrelo <diegoe@igalia.com>
--

Updated to master.

Review of attachment 210800 [details] [review]:

OK.

Review of attachment 210808 [details] [review]:

Looks good.

::: embed/ephy-web-app-utils.c
@@ +467,3 @@
+/**
+ * ephy_web_application_exists:
+ * @name: the name of the web application

Nitpick, but I'd say "the potential name".

@@ +469,3 @@
+ * @name: the name of the web application
+ *
+ * Returns: whether an application with the same @name exists.

"whether an application with @name exists".

Attachment 210800 [details] pushed as c07d767 - window-commands: get app name only once
Attachment 210808 [details] pushed as 2ef933e - e-web-app-utils: add ephy_web_application_exists

I think it would be good to have this, but by now the patches need to be updated to git master. So marking os obsolete, and hopefully we'll get at it soon.

This is a mass NEEDINFO of all Epiphany bugs with no activity in the past three years. I'm going to be automatically closing old bugs to help us focus on current problems. If you feel this bug is still relevant with Epiphany 3.26 or newer, then please leave any comment here so that I know not to close this one.

This is a mass-close of old bugs currently in the NEEDINFO state.

If you think this bug is still relevant, please leave a comment.